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

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Darkspirit]

(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!

Comment 1

[Markus Stange [:mstange]]

It would be good to find out if this is a regression.

Comment 2

[(no longer active)]

Sounds like an infinite loop?

Comment 3

[Andrew Overholt [:overholt]]

Need a regression window here. Maybe Jet knows someone who knows about ranges/selections?

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think I might be hitting this too: https://perfht.ml/2u2YBeZ

Was clicking/selecting text inside a github PR.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hanging for 18 seconds inside text selection code.

Comment 6

[(no longer active)]

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?

Comment 7

[(no longer active)]

Nevermind, trying to make a selection on the attached test case is enough to reproduce!

Comment 8

[Jet Villegas (inactive)]

Yikes! This one's quite awful: We can get nsIContentIterator->Next() stuck on a single node. Hopefully, an easy fix? 

Kato-san: WDYT?

Comment 9

[Samael Wang [:freesamael] (away for now)]

Tried mozregression since it's quite easy to reproduce, and it gave me bug 1351170. Local build shows the regression commit is part 1.

Comment 10

[Hsin-Yi Tsai (she/her) [:hsinyi]]

NI Jim according to regression window in comment 9

Comment 11

[Jet Villegas (inactive)]

Attachment: bug source

A patch to identify the offending code.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1383242 - Properly compare node to traversal range under different modes;

When the node borders one of the range bounds, `NodeIsInTraversalRange`
should return different results depending on whether it's in pre mode or
not.

>  <div><br></div>
>       \__/

In this pre mode example, the node <br> is within the range, and the
node position (which is at the start of the node in pre mode) and the
start bound are both (<div>, 0). Therefore, it shows the start bound
should be inclusive in pre mode.

>  <div><br></div>
>  \___/

In this pre mode example, the node <br> is outside of the range, yet the
node position and the end bound are both (<div>, 0). Therefore, it shows
the end bound should be exclusive in pre mode.

>  <div><br></div>
>           \____/

in this post mode example, the node <br> is outside of the range, yet
the node position (which is at the end of the node in post mode) and the
start bound are both (<div>, 1). Therefore, it shows the start bound
should be exclusive in post mode.

>  <div><br></div>
>       \__/

In this post mode example, the node <br> is within the range, and the
node position and the end bound are both (<div>, 1). Therefore, it shows
the end bound should be inclusive in post mode.

In summary, the correct pre mode bound check is `start <= node < end`,
and the correct post mode bound check is `start < node <= end`. This
patch fixes `NodeIsInTraversalRange` to have the correct bounds check.

Review commit: https://reviewboard.mozilla.org/r/164018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164018/

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Very scary looking change. Changing the behavior we have had from 2002.
I need to think about this.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Could you explain why your patch caused the regression?

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

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

[Olli Pettay [:smaug][[e-mail]]]

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-

Comment 17

[(inactive) Jim Chen [:jchen] [:darchons]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9b9ffb6fea5
Properly compare node to traversal range under different modes; r=smaug

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Considering the risks involved with this bug, we probably want to back out parts of bug 1351170 from 56 Beta

Comment 20

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f9b9ffb6fea5

Comment 21

[Darkspirit]

You've crushed the bug.
Verified fixed in Nightly 57 x64 20170805100334 @ Debian Testing.
Thank you!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Please attach a Beta patch here that takes care of comment 19 and request approval :)

Comment 23

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: 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

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8895195 [details] [diff] [review]
Patch for Beta

Partial backout to fix a hang. Let's land this for beta 2.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/21d266694e28