Closed Bug 804799 Opened 7 years ago Closed 7 years ago

Freeze when searching for content with ::after and font-size: 0

Categories

(Core :: Find Backend, defect, critical)

19 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox18 --- unaffected
firefox19 + verified

People

(Reporter: simon.lindholm10, Assigned: bzbarsky)

References

Details

(Keywords: hang, regression, testcase)

Attachments

(2 files)

Attached file testcase
User Agent: Mozilla/5.0 (X11; Linux i686; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121023030553

Steps to reproduce:

Open a page containing
<style>
.ellipsis { font-size: 0; }
.ellipsis::after { content: '...'; font-size: 13px; }
</style>
<span>abc<span class="ellipsis">def</span>ghi</span>
(attached)

Search for 'd' with the find bar.


Actual results:

Firefox froze. Stacks like
> ...
> #N-1 nsFind::Find(unsigned short const*, nsIDOMRange*, nsIDOMRange*, nsIDOMRange*, nsIDOMRange**) ()
> #N nsTypeAheadFind::FindItNow(nsIPresShell*, bool, bool, bool, unsigned short*) ()
and
> ...
> #N-1 nsTypeAheadFind::IsRangeVisible(nsIPresShell*, nsPresContext*, nsIDOMRange*, bool, bool, nsIDOMRange**, bool*) ()
> #N nsTypeAheadFind::FindItNow(nsIPresShell*, bool, bool, bool, unsigned short*) ()



Expected results:

Should have found the 'f' (without highlighting it). This is what happens in Firefox 18.
Comment on attachment 674412 [details]
testcase

><meta charset="utf-8">
><style>
>.ellipsis {
>    font-size: 0;
>}
>.ellipsis::after {
>    content: '...';
>    font-size: 13px;
>}
></style>
><span class="value">abc<span class="ellipsis">def</span>ghi</span>
Attachment #674412 - Attachment mime type: text/plain → text/html
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/78c16f313547
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121011122403
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2efb9e5762ce
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0 ID:20121011124703
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=78c16f313547&tochange=2efb9e5762ce
Suspected: bug 773247
Blocks: 773247
Severity: normal → critical
Status: UNCONFIRMED → NEW
Component: Untriaged → Find Backend
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
Blocks: 803303
Simon, thank you very much for the small testcase.  It made this very easy to debug!
Duplicate of this bug: 803579
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment on attachment 674488 [details] [diff] [review]
Make sure we keep advancing our current find point through the document even in cases when we optimize by moving it to the first visible range, so we don't get stuck in an infinite loop.

I feel this bug very strange. Does IsRangeVisible() returns invisible range for mStartPointRange? If so, I think we should fix this in IsRangeVisible(). However, nsTypeAheadFind.cpp is too messy... So, the change might break other behavior, though...

>diff --git a/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp b/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
>--- a/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
>+++ b/toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
>@@ -385,20 +385,52 @@ nsTypeAheadFind::FindItNow(nsIPresShell 
>       bool usesIndependentSelection;
>       if (!IsRangeVisible(presShell, presContext, returnRange,
>                           aIsFirstVisiblePreferred, false,
>                           getter_AddRefs(mStartPointRange), 
>                           &usesIndependentSelection) ||
>           (aIsLinksOnly && !isInsideLink) ||
>           (mStartLinksOnlyPref && aIsLinksOnly && !isStartingLink)) {
>         // ------ Failure ------
>-        // mStartPointRange got updated to the right thing already,
>-        // but we stil need to collapse it to the right end.
>-        mStartPointRange->Collapse(aFindPrev);
>-
>+        // At this point mStartPointRange got updated to the first
>+        // visible range in the viewport.  We _may_ be able to just
>+        // start there, if it's not taking us in the wrong direction.
>+        if (aFindPrev) {
>+          // We can continue at the end of mStartPointRange if its end
>+          // is before the start of returnRange.  Otherwise, we need
>+          // to continue at the start of returnRange.
>+          int16_t compareResult;
>+          nsresult rv =
>+            mStartPointRange->CompareBoundaryPoints(nsIDOMRange::START_TO_END,
>+                                                    returnRange, &compareResult);
>+          if (NS_SUCCEEDED(rv) && compareResult < 0) {

Shouldn't this be compareResult <= 0?

>+            // OK to start at the end of mStartPointRange
>+            mStartPointRange->Collapse(false);
>+          } else {
>+            // Start at the beginning of returnRange
>+            returnRange->CloneRange(getter_AddRefs(mStartPointRange));
>+            mStartPointRange->Collapse(true);
>+          }
>+        } else {
>+          // We can continue at the start of mStartPointRange if its
>+          // start is after the end of returnRange.  Otherwise, we
>+          // need to continue at the end of returnRange.
>+          int16_t compareResult;
>+          nsresult rv =
>+            mStartPointRange->CompareBoundaryPoints(nsIDOMRange::END_TO_START,
>+                                                    returnRange, &compareResult);
>+          if (NS_SUCCEEDED(rv) && compareResult > 0) {

compareResult >= 0?

>+            // OK to start at the start of mStartPointRange
>+            mStartPointRange->Collapse(true);
>+          } else {
>+            // Start at the end of returnRange
>+            returnRange->CloneRange(getter_AddRefs(mStartPointRange));
>+            mStartPointRange->Collapse(false);
>+          }
>+        }
>         continue;
>       }
Attachment #674488 - Flags: review?(masayuki) → review+
> Does IsRangeVisible() returns invisible range for mStartPointRange?

No, it returns a visible range.  But in the testcase in this bug it was returning a range that came completely before returnRange.  So we'd do another search and find the same returnRange over and over again, which is why we ended up in an infinite loop.

> Shouldn't this be compareResult <= 0?

Hmm.  I guess it could be; if they're equal then it doesn't matter which one we continue from and this would save us cloning a range.  Will do.

> compareResult >= 0?

Likewise.
http://hg.mozilla.org/integration/mozilla-inbound/rev/fdb09cf360f3
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla19
https://hg.mozilla.org/mozilla-central/rev/fdb09cf360f3
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 803303
Verified the freeze by using the testcase for 2012-10-23 Nightly 
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/19.0 Firefox/19.0
Build ID: 20121023030553

No issue found using the same testcase for Firefox 19.0 beta 3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130123083802
You need to log in before you can comment on or make changes to this bug.