Hang [@ nsRange::ExcludeNonSelectableNodes] when doing text selection in this testcase

VERIFIED FIXED in Firefox 56

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: darkspirit, Assigned: jchen)

Tracking

({hang, nightly-community, regression})

55 Branch
mozilla57
Unspecified
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [qf:p1], crash signature, )

Attachments

(3 attachments)

(Darkspirit from bug 1377690 comment 5)
> (In reply to Markus Stange [:mstange] from bug 1377690 comment 0)
> > Created attachment 8882833 [details]
> > testcase
> 
> In Nightly 56 x64 20170721100159 @ Debian Testing (Linux 4.11.0-1-amd64, Radeon RX480)
> (with a fresh profile with default settings, so without webrender/stylo)
> my tab freezes/crashes and I get
> bp-90011b08-9336-47ff-98ea-9c61f0170721 [@ IPCError-browser | ShutDownKill ]
> with your attachment,
> after I do a mousedown on the square (div), hold it, move it to the center of the window and release my mouse:
> If I then do a rightclick, I don't get a context menu, or if I change to another tab and switch back, there is a loading circle until the tab crashes.
> I am unsure where should I report this. It looks crazy to me that such a simple div could crash my Nightly.
> Can someone reproduce this?

(Markus Stange [:mstange] from bug 1377690 comment 6)
> I can reproduce the content process hang on Mac with these steps to reproduce. Can you please file a new bug? Thanks!
Component: CSS Parsing and Computation → General
Product: Core → Firefox
Has STR: --- → yes
Component: General → DOM: Core & HTML
Product: Firefox → Core
Summary: tab crash: Mousedown on a bordered div and release it in the center of the window → Hang [@ nsRange::ExcludeNonSelectableNodes] when doing text selection in this testcase
It would be good to find out if this is a regression.
Sounds like an infinite loop?
Need a regression window here. Maybe Jet knows someone who knows about ranges/selections?
Flags: needinfo?(bugs)
Priority: -- → P1
I think I might be hitting this too: https://perfht.ml/2u2YBeZ

Was clicking/selecting text inside a github PR.
Hanging for 18 seconds inside text selection code.
Yes, thank you this profile is really helpful.  Also, wow!  This function can take 18 seconds.  :-(

Any chance you can see if you can try to come up with STRs?  Or a regression range please?
Whiteboard: [qf]
Nevermind, trying to make a selection on the attached test case is enough to reproduce!
Whiteboard: [qf] → [qf:p1]
Yikes! This one's quite awful: We can get nsIContentIterator->Next() stuck on a single node. Hopefully, an easy fix? 

Kato-san: WDYT?
Flags: needinfo?(bugs) → needinfo?(djripper)
Tried mozregression since it's quite easy to reproduce, and it gave me bug 1351170. Local build shows the regression commit is part 1.
NI Jim according to regression window in comment 9
Flags: needinfo?(nchen)
Posted patch bug sourceSplinter Review
A patch to identify the offending code.
Flags: needinfo?(djripper)
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
Very scary looking change. Changing the behavior we have had from 2002.
I need to think about this.
Could you explain why your patch caused the regression?
In a nutshell, I think we never ran into this before because `NodeIsInTraversalRange` was not called often in practice. There are cases where we adjust the start of the range, but the new start would be after the end of the range, and `NodeIsInTraversalRange` was designed to catch these "degenerate" cases. However, before bug 1351170, we didn't adjust the range start properly, so in turn `NodeIsInTraversalRange` was not exercised often. With bug 1351170, we now regularly (and correctly) adjust the range start, but `NodeIsInTraversalRange` is failing to catch the "degenerate" cases, resulting in this regression.
Comment on attachment 8893018 [details]
Bug 1383242 - Properly compare node to traversal range under different modes;

https://reviewboard.mozilla.org/r/164018/#review170042

This is so über-scary patch that at least push to try before landing.
The method does get called in many places via ::Init and ::PositionAt-
Attachment #8893018 - Flags: review?(bugs) → review+
Yeah, I didn't see any obviously related failures in the try run [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c1c24b7527df4bc25ed089a37fe124f79ca016
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9b9ffb6fea5
Properly compare node to traversal range under different modes; r=smaug
Considering the risks involved with this bug, we probably want to back out parts of bug 1351170 from 56 Beta
https://hg.mozilla.org/mozilla-central/rev/f9b9ffb6fea5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You've crushed the bug.
Verified fixed in Nightly 57 x64 20170805100334 @ Debian Testing.
Thank you!
Status: RESOLVED → VERIFIED
Please attach a Beta patch here that takes care of comment 19 and request approval :)
Flags: needinfo?(nchen)
Version: Trunk → 55 Branch
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1351170
[User impact if declined]: Hang when selecting text on certain pages.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Patch is a simple backout of the offending commit
[String changes made/needed]: None
Flags: needinfo?(nchen)
Attachment #8895195 - Flags: review+
Attachment #8895195 - Flags: approval-mozilla-beta?
Comment on attachment 8895195 [details] [diff] [review]
Patch for Beta

Partial backout to fix a hang. Let's land this for beta 2.
Attachment #8895195 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.