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

VERIFIED FIXED in Firefox 56

Status

()

Core
DOM: Core & HTML
P1
normal
VERIFIED FIXED
a month ago
13 days ago

People

(Reporter: darkspirit, Assigned: jchen)

Tracking

({hang, nightly-community, regression})

55 Branch
mozilla57
Unspecified
All
hang, nightly-community, regression
Points:
---

Firefox Tracking Flags

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

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

a month ago
(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!
(Reporter)

Updated

a month ago
Component: CSS Parsing and Computation → General
Product: Core → Firefox
(Reporter)

Updated

a month ago
Has STR: --- → yes
Keywords: crash, nightly-community
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.
Keywords: crash → hang, regressionwindow-wanted
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]

Comment 8

22 days ago
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.
Blocks: 1351170
Keywords: regressionwindow-wanted → regression
NI Jim according to regression window in comment 9
Flags: needinfo?(nchen)

Comment 11

21 days ago
Created attachment 8892407 [details] [diff] [review]
bug source

A patch to identify the offending code.
Flags: needinfo?(djripper)
(Assignee)

Updated

21 days ago
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Flags: needinfo?(nchen)
Comment hidden (mozreview-request)
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?
(Assignee)

Comment 15

20 days ago
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 16

19 days ago
mozreview-review
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+
(Assignee)

Comment 17

19 days ago
Yeah, I didn't see any obviously related failures in the try run [1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=05c1c24b7527df4bc25ed089a37fe124f79ca016

Comment 18

18 days ago
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f9b9ffb6fea5
Properly compare node to traversal range under different modes; r=smaug
(Assignee)

Comment 19

18 days ago
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
Last Resolved: 17 days ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Reporter)

Comment 21

17 days ago
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 :)
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(nchen)
Version: Trunk → 55 Branch
(Assignee)

Comment 23

14 days ago
Created attachment 8895195 [details] [diff] [review]
Patch for Beta

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+

Comment 25

13 days ago
uplift
https://hg.mozilla.org/releases/mozilla-beta/rev/21d266694e28
status-firefox56: affected → fixed
You need to log in before you can comment on or make changes to this bug.