Closed Bug 1387481 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: MayTraverseFrom(const_cast<Element*>(aRoot))

Categories

(Core :: CSS Parsing and Computation, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: jkratzer, Assigned: heycam)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached testcase causes an assertion in m-c rev 36ad88e6b7b2 with stylo enabled by pref.

Assertion failure: MayTraverseFrom(const_cast<Element*>(aRoot)), at /home/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:275
#01: mozilla::ServoStyleSet::StyleNewSubtree at layout/style/ServoStyleSet.cpp:805
#02: mozilla::HTMLEditor::CreateAnonymousElement at mfbt/RefPtr.h:284
#03: mozilla::HTMLEditor::CreateResizer at mfbt/AlreadyAddRefed.h:121
#04: mozilla::HTMLEditor::ShowResizersInner at editor/libeditor/HTMLEditorObjectResizer.cpp:307
#05: mozilla::HTMLEditor::ShowResizers at editor/libeditor/HTMLEditorObjectResizer.cpp:284
#06: mozilla::HTMLEditor::CheckSelectionStateForAnonymousButtons at editor/libeditor/HTMLAnonymousNodeEditor.cpp:465
#07: mozilla::HTMLEditor::EndUpdateViewBatch at editor/libeditor/HTMLEditor.cpp:4842
#08: mozilla::EditorBase::EndPlaceHolderTransaction at editor/libeditor/EditorBase.cpp:1027
#09: mozilla::AutoEditBatch::~AutoEditBatch at editor/libeditor/EditorUtils.h:167
#10: mozilla::HTMLEditor::RemoveInlinePropertyImpl at editor/libeditor/HTMLStyleEditor.cpp:1336
#11: RemoveOneProperty at editor/composer/nsComposerCommands.cpp:1494
#12: nsStyleUpdatingCommand::ToggleState at editor/composer/nsComposerCommands.cpp:240
#13: nsBaseStateUpdatingCommand::DoCommand at editor/composer/nsComposerCommands.cpp:92
#14: nsControllerCommandTable::DoCommand at dom/commandhandler/nsControllerCommandTable.cpp:147
#15: nsBaseCommandController::DoCommand at xpcom/base/nsCOMPtr.h:801
#16: nsCommandManager::DoCommand at dom/commandhandler/nsCommandManager.cpp:208
#17: nsHTMLDocument::ExecCommand at dom/bindings/ErrorResult.h:376
#18: mozilla::dom::HTMLDocumentBinding::execCommand at obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:877
Flags: in-testsuite?
No testcase is attached?
Attached file trigger.html
Priority: -- → P2
Nice! This is a test case of bug 1384526?  Today's nightly (buildid: 20170804193726) crashed when I reload the test case several times.

https://crash-stats.mozilla.com/report/index/90c766c5-0d48-4a45-a892-15a040170804

I think fixing this assertion also fixes bug 1384526.
Blocks: 1384526
Hmmm... this time it is creating anonymous element inside script element... 

Could this be a more general issue applying to any display:none element?
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #4)
> Could this be a more general issue applying to any display:none element?

Yeah, I am thinking the same.  It's odd that editor wants to append some native anonymous content to an element in a display:none subtree, but unless we want to fix that, it seems easy enough to just avoid styling the newly added NAC if its parent is unstyled or is display:none itself.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Comment on attachment 8894067 [details]
Bug 1387481 - Part 1: Don't eagerly style editor-created NAC if it's appended to an element in a display:none subtree.

https://reviewboard.mozilla.org/r/165166/#review170600
Attachment #8894067 - Flags: review?(hikezoe) → review+
Comment on attachment 8894068 [details]
Bug 1387481 - Part 2: Crashtest.

https://reviewboard.mozilla.org/r/165168/#review170602

Test case itsel looks good to me, but I don't quite understand what happened in crashtest.list.  I guess you modified it for reducing running time for crash tests?
Attachment #8894068 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> Test case itsel looks good to me, but I don't quite understand what happened
> in crashtest.list.  I guess you modified it for reducing running time for
> crash tests?

Oops, yes, will fix before landing.
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/791ed5440ffd
Part 1: Don't eagerly style editor-created NAC if it's appended to an element in a display:none subtree. r=hiro
https://hg.mozilla.org/integration/autoland/rev/99c06b6ebb4e
Part 2: Crashtest. r=hiro
https://hg.mozilla.org/mozilla-central/rev/791ed5440ffd
https://hg.mozilla.org/mozilla-central/rev/99c06b6ebb4e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Is there a user impact here that justifies backport to 56 in support of the Stylo experiments? If so, please nominate :)
Flags: needinfo?(cam)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8894067 [details]
Bug 1387481 - Part 1: Don't eagerly style editor-created NAC if it's appended to an element in a display:none subtree.

(Approval request applies to the crashtest too.)

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: When stylo is enabled, we can crash when certain editor-related content is involved.  We'd like to reduce the chance of crashes for our Beta stylo pref experiment.
[Is this code covered by automated tests?]: yes, a crashtest is in the bug
[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?]: the fix is pretty simple
[String changes made/needed]: none
Flags: needinfo?(cam)
Attachment #8894067 - Flags: approval-mozilla-beta?
Comment on attachment 8894067 [details]
Bug 1387481 - Part 1: Don't eagerly style editor-created NAC if it's appended to an element in a display:none subtree.

Fix a stylo crash and include a test. Beta56+. Should be in 56 beta 2.
Attachment #8894067 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Cameron McCormack (:heycam) from comment #17)
> [Is this code covered by automated tests?]: yes, a crashtest is in the bug
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Cameron's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.