Closed Bug 1497480 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Spelling checker, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- disabled
firefox63 --- disabled
firefox64 --- fixed
firefox65 --- fixed

People

(Reporter: zjz, Assigned: edgar)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(4 files)

Attached file test.html
Please see the uploaded html file.
Blocks: 97284, shadowdom
Severity: normal → critical
This also crashes in opt, in the spellchecker code...
Summary: Assertion failure: 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 → 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
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?
Blocks: 1066965
Flags: needinfo?(bugs)
edgar, do you have spare cycles? If not, I'll take a look soon.
Flags: needinfo?(bugs) → needinfo?(echen)
I will take a look.
See Also: → 1446043
See Also: 1446043
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.
Assignee: nobody → echen
Flags: needinfo?(echen)
I'm not really expecting spell checking to cross shadow boundary.
Attachment #9019390 - Attachment description: Bug 1497480 - Part 1: Set the root for spelling checker to shadow root if the checking nodes are in shadow tree; → Bug 1497480 - Part 1: Set the root for spelling checker to shadow root if the contenteditable nodes are in the shadow DOM;
Component: DOM → Spelling checker
Pushed by echen@mozilla.com: 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
No longer blocks: 1446043
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
Flags: needinfo?(echen)
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.
Pushed by echen@mozilla.com: 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
Is this something we should consider for Beta backport or can it ride the trains?
Flags: needinfo?(echen)
Flags: in-testsuite+
IMO, we should consider for Beta backport.
I agree beta uplift. It causes crash, and the current beta is at the beginning of its cycle.
Edgar, could you ask approvals for beta?
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
Flags: needinfo?(echen)
Attachment #9019390 - Flags: approval-mozilla-beta?
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.
Attachment #9019391 - Flags: approval-mozilla-beta?
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.
Attachment #9020055 - Flags: approval-mozilla-beta?
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
Attachment #9019390 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9019391 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9020055 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: