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

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

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

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

No testcase is attached?

Comment 2

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Comment 3

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Hmmm... this time it is creating anonymous element inside script element... 

Could this be a more general issue applying to any display:none element?

Comment 5

[Cameron McCormack (:heycam)]

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

Comment 6

[Cameron McCormack (:heycam)]

Attachment: Bug 1387481 - Part 1: Don't eagerly style editor-created NAC if it's appended to an element in a display:none subtree.

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

Comment 7

[Cameron McCormack (:heycam)]

Attachment: Bug 1387481 - Part 2: Crashtest.

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

Comment 8

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a91052c0add571be07aacbaf4892bd947c9dcbd3

Comment 9

[Hiroyuki Ikezoe (:hiro)]

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

Comment 10

[Hiroyuki Ikezoe (:hiro)]

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?

Comment 11

[Cameron McCormack (:heycam)]

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

Comment 12

[Cameron McCormack (:heycam)]

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.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165166/diff/1-2/

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8894068 [details]
Bug 1387481 - Part 2: Crashtest.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165168/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/791ed5440ffd
https://hg.mozilla.org/mozilla-central/rev/99c06b6ebb4e

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact here that justifies backport to 56 in support of the Stylo experiments? If so, please nominate :)

Comment 17

[Cameron McCormack (:heycam)]

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

Comment 18

[Gerry Chang [:gchang]]

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.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b5eecee891c9
https://hg.mozilla.org/releases/mozilla-beta/rev/0280c6cbeb09

Comment 20

[Andrei Vaida [:avaida]]

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