Bug 1510485 - Properly handle errors in nsFind when returning a range. r=bzbarsky, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 11 Dec 2018 01:06:32 +0000
changeset 508972 3a8a5011f5bf8bd3dcfe84af64c459fef20d49a2
parent 508971 663b56166037bd6b1b066e8e95f9818d8746fd18
child 508973 60444ade289159d488b7ab144e28e7091946a43c
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbzbarsky, RyanVM
bugs1510485, 1505887, 1442466
milestone65.0
Bug 1510485 - Properly handle errors in nsFind when returning a range. r=bzbarsky, a=RyanVM Bug 1505887 changed the behavior here from content calling into nsFind via window.find(), by making the SetStart and SetEnd calls here fail instead of succeed for NAC (like the text in textareas). This patch makes us handle that error gracefully moving on to the next match, instead of trying to preserve the previous behavior. This means that we'll fail to highlight textarea content and such from window.find, which Chromium does, looks like. Though Chromium doesn't expose the ranges as selection either. In any case I don't think that this is a very common thing given bugs like bug 1442466, which this bug fixes. I haven't found anything close to a spec for what window.find() should do... If we decide to go with this patch then I'll add a crashtest for this and a test for bug 1442466 as well. Otherwise I'll add a way to skip the security check in nsFind somehow for NAC, or relax the security restrictions in SetStart / SetEnd, I guess. Differential Revision: https://phabricator.services.mozilla.com/D14013
layout/base/crashtests/1510485.html
layout/base/crashtests/crashtests.list
toolkit/components/find/nsFind.cpp
new file mode 100644
--- /dev/null
+++ b/layout/base/crashtests/1510485.html
@@ -0,0 +1,7 @@
+<script>
+onload = function(){
+  window.find('e')
+}
+</script>
+<textarea>
+  <!-- E -->
--- a/layout/base/crashtests/crashtests.list
+++ b/layout/base/crashtests/crashtests.list
@@ -546,8 +546,9 @@ load 1477847.html
 load 1489149.html
 load 1490037.html
 load 1494332.html
 load 1494030.html
 load 1505420.html
 pref(layout.css.column-span.enabled,true) load 1506163.html
 pref(layout.css.column-span.enabled,true) load 1506204.html
 load 1510080.html
+load 1510485.html
--- a/toolkit/components/find/nsFind.cpp
+++ b/toolkit/components/find/nsFind.cpp
@@ -752,30 +752,30 @@ nsFind::Find(const char16_t* aPatText, n
           matchStartOffset = findex;
           matchEndOffset = mao;
         } else {
           startParent = matchAnchorNode;
           endParent = current;
           matchStartOffset = mao;
           matchEndOffset = findex + 1;
         }
+
         if (startParent && endParent && IsVisibleNode(startParent) &&
             IsVisibleNode(endParent)) {
-          range->SetStart(*startParent, matchStartOffset, IgnoreErrors());
-          range->SetEnd(*endParent, matchEndOffset, IgnoreErrors());
-          *aRangeRet = range.get();
-          NS_ADDREF(*aRangeRet);
-        } else {
-          // This match is no good -- invisible or bad range
-          startParent = nullptr;
+          IgnoredErrorResult rv;
+          range->SetStart(*startParent, matchStartOffset, rv);
+          if (!rv.Failed()) {
+            range->SetEnd(*endParent, matchEndOffset, rv);
+          }
+          if (!rv.Failed()) {
+            range.forget(aRangeRet);
+            return NS_OK;
+          }
         }
 
-        if (startParent) {
-          return NS_OK;
-        }
         // This match is no good, continue on in document
         matchAnchorNode = nullptr;
       }
 
       if (matchAnchorNode) {
         // Not done, but still matching. Advance and loop around for the next
         // characters. But don't advance from a space to a non-space:
         if (!inWhitespace || DONE_WITH_PINDEX ||