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

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
critical
RESOLVED FIXED
9 months ago
8 months ago

People

(Reporter: zjz, Assigned: edgar)

Tracking

(Blocks 1 bug, {crash, testcase})

Trunk
mozilla65
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 disabled, firefox63 disabled, firefox64 fixed, firefox65 fixed)

Details

Attachments

(4 attachments)

Reporter

Description

9 months ago
Posted file test.html
Please see the uploaded html file.
Reporter

Updated

9 months ago
Blocks: 97284, shadowdom
Severity: normal → critical
This also crashes in opt, in the spellchecker code...
Reporter

Updated

9 months ago
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

Comment 12

8 months ago
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
Duplicate of this bug: 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.

Comment 17

8 months ago
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.

Updated

8 months ago
Duplicate of this bug: 1504414
Reporter

Comment 22

8 months ago
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.