1497480 - Assertion failure(and crash in opt): when typing <Enter> after the last character in the contenteditable node, which is also the last node in a shadow tree, assertion failure "aRoot not an ancestor of |this|?" happens

Status: RESOLVED FIXED

(Core :: Spelling checker, defect, P2)

Description

[張俊芝(ZHANG Junzhi)]

Attachment: test.html

Please see the uploaded html file.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

This also crashes in opt, in the spellchecker code...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So I just took a quick look, the issue is that spell-checking code accepts shadow-including descendants:

  https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/extensions/spellcheck/src/mozInlineSpellChecker.cpp#1367

But then the SetEnd call right below that line ends up looking for the next node in the light DOM using GetNextNode():

  https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp#143

Which crashes if they're not in the same light DOM subtree.

Furthermore all the offsets the spellchecking code maintains are referring to DOM children, not flattened tree children... So probably somebody needs to go ahead and figure out how this should really work with Shadow DOM...

If this kinda works right now we may want to just wallpaper the crash for now, but...

Olli, do you know who's familiar with this code and could help?

Comment 3

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

edgar, do you have spare cycles? If not, I'll take a look soon.

Comment 4

[Edgar Chen [:edgar]]

I will take a look.

Comment 5

[Edgar Chen [:edgar]]

I found other issues in spell-checking with Shadow DOM, too, for example,
- Full range checking doesn't check the text inside the shadow DOM.
- Navigate in/out Shadow DOM doesn't trigger spell-checking.
- I saw a lot of 

We could teach mozInlineSpellWordUtil about the flattened tree which could fix this crash, it doesn't make spell-checking be fully compatible with Shadow DOM. There is still some other part we need to figure out how to deal with Shadow DOM, for example, spell-checking code use nsRange and nsRange only deal with subtree. But at least, we don't crash.

Comment 6

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

I'm not really expecting spell checking to cross shadow boundary.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Bug 1497480 - Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM;

Comment 8

[Edgar Chen [:edgar]]

Attachment: Bug 1497480 - Part 2: Make spellchecking to work in ShadowDOM after anchor navigates away from it;

Comment 9

[Edgar Chen [:edgar]]

Attachment: Bug 1497480 - Part 3: Add tests for spell-checking in ShadowDOM;

Comment 10

[Edgar Chen [:edgar]]

 https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=456c717d09fdb9bd3760f7fcc0892d6113f2c672&group_state=expanded

Comment 11

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=25b0b6164161d96fc58de93d5370f1bb962de922&group_state=expanded

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31d9a680fb90
Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM; r=smaug
https://hg.mozilla.org/integration/autoland/rev/cf74ff09b141
Part 2: Make spellchecking to work in ShadowDOM after anchor navigates away from it; r=smaug
https://hg.mozilla.org/integration/autoland/rev/6c2a5ce9fad8
Part 3: Add tests for spell-checking in ShadowDOM; r=smaug

Comment 14

[Cosmin Sabou [:CosminS]]

Backed for causing Android mochitest failures on editor/libeditor/tests/test_bug

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=6c2a5ce9fad8b976bfa51fdc80908d1bbfd0e90f&tochange=4610b4c5befa0f89360ab554efcf0dbd9a1e0a07&searchStr=android%2C4.3%2Capi16&selectedJob=208933385

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=208933385&repo=autoland&lineNumber=1472

Backout link: https://hg.mozilla.org/integration/autoland/rev/e2897d099d2a6e78599312f386b8a80352c3c506

[task 2018-10-31T16:53:05.320Z] 16:53:05     INFO -  84 INFO TEST-START | editor/libeditor/tests/test_bug1497480.html
[task 2018-10-31T16:53:15.641Z] 16:53:15     INFO -  Buffered messages logged at 16:53:03
[task 2018-10-31T16:53:15.641Z] 16:53:15     INFO -  85 INFO AddTask.js | Entering test
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -  86 INFO AddTask.js | Leaving test
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -  87 INFO AddTask.js | Entering test
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -  Buffered messages finished
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -  88 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1497480.html | Correct number of misspellings and words. - got +0, expected 1
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -      isSpellingCheckOk@editor/libeditor/tests/spellcheck.js:6:3
[task 2018-10-31T16:53:15.642Z] 16:53:15     INFO -      @editor/libeditor/tests/test_bug1497480.html:69:10

Comment 15

[Edgar Chen [:edgar]]

Okay, I found other spell-checking tests are all disabled on android, maybe it is doesn't work well on android. Let's just disable this on android.

Comment 16

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&tier=1&revision=823cf71b7ce03bdf965259cab6fc443f73405f86&group_state=expanded

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d300e4c01708
Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM; r=smaug
https://hg.mozilla.org/integration/autoland/rev/c094ca2c2a1d
Part 2: Make spellchecking to work in ShadowDOM after anchor navigates away from it; r=smaug
https://hg.mozilla.org/integration/autoland/rev/742633251297
Part 3: Add tests for spell-checking in ShadowDOM; r=smaug

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d300e4c01708
https://hg.mozilla.org/mozilla-central/rev/c094ca2c2a1d
https://hg.mozilla.org/mozilla-central/rev/742633251297

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for Beta backport or can it ride the trains?

Comment 20

[Edgar Chen [:edgar]]

IMO, we should consider for Beta backport.

Comment 22

[張俊芝(ZHANG Junzhi)]

I agree beta uplift. It causes crash, and the current beta is at the beginning of its cycle.

Comment 23

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

Edgar, could you ask approvals for beta?

Comment 24

[Edgar Chen [:edgar]]

Comment on attachment 9019390 [details]
Bug 1497480 - Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Browser crash if user press enters in contenteditable elements which are in the shadow DOM.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk. Patches here is to add safe handling for shadow DOM to avoid spell-checker crossing shadow boundary.

String changes made/needed: None

Comment 25

[Edgar Chen [:edgar]]

Comment on attachment 9019391 [details]
Bug 1497480 - Part 2: Make spellchecking to work in ShadowDOM after anchor navigates away from it;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Browser crash if user press enters in contenteditable elements which are in the shadow DOM.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk. Patches here is to add safe handling for shadow DOM to avoid spell-checker crossing shadow boundary.

String changes made/needed: None.

Comment 26

[Edgar Chen [:edgar]]

Comment on attachment 9020055 [details]
Bug 1497480 - Part 3: Add tests for spell-checking in ShadowDOM;

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Browser crash if user press enters in contenteditable elements which are in the shadow DOM.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk. Patches here is to add safe handling for shadow DOM to avoid spell-checker crossing shadow boundary.

String changes made/needed: None.

Comment 27

[Julien Cristau [:jcristau]]

Comment on attachment 9019390 [details]
Bug 1497480 - Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM;

crash fix, regression in 64, approved for 64.0b7

Comment 28

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fa8dfa3aef26
https://hg.mozilla.org/releases/mozilla-beta/rev/ad24ecd2dd39
https://hg.mozilla.org/releases/mozilla-beta/rev/f9639e65bc99