Jesse Schwartzentruber (:truber) Reporter
	Description • 7 years ago

The attached testcase causes an assertion in m-c rev 20170831-fb22415719a9 with stylo enabled by pref.

Assertion failure: MayTraverseFrom(const_cast<Element*>(root)), at /builds/worker/workspace/build/src/layout/style/ServoStyleSet.cpp:898
#0 mozilla::ServoStyleSet::StyleDocument, at layout/style/ServoStyleSet.cpp:898
#1 mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1100
#2 mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4191
#3 nsRefreshDriver::Tick, at layout/base/nsRefreshDriver.cpp:1921

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 1 • 7 years ago

root is a <mathml>, and its parent is <area> which is display:none, and thus MayTraverseFrom returns false.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 2 • 7 years ago

The restyle root is set from NoteDirtyElement in Element.cpp. There is an if-statement in that function that:
> if (!parent->GetPrimaryFrame() && Servo_Element_IsDisplayNone(parent)) {
>   return;
> }

However, <area> element always has nsImageFrame as its primary frame when there is <img> element using the corresponding <map>, and thus this condition doesn't protect us from setting the invisible <mathml> as a restyle root.

	Xidorn Quan [:xidorn] UTC+11 Assignee
	Comment 3 • 7 years ago

That said, majority of the code in the testcase isn't necessary to trigger this assertion.

What really important here is that, we have a valid <img> which uses a <map> which has <area>, and we dynamically insert a new element into the <area> element.

	Emilio Cobos Álvarez (:emilio)
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8903453 [details]
Bug 1395725 - Don't add children of <area> as servo restyle root.

https://reviewboard.mozilla.org/r/175300/#review180434

::: dom/base/Element.cpp:4341
(Diff revision 1)
>  
>      // If the parent is styled but is display:none, we're done.
>      //
>      // We check for a frame to reduce the cases where we need the FFI call.
> -    if (!parent->GetPrimaryFrame() && Servo_Element_IsDisplayNone(parent)) {
> +    // We need to additionally check that it is not an <area> element because
> +    // <area> may have primary frame from <img>, but itself is display:none.

Please mention bug 135040 so I don't forget to revert this when I get to finish it.

	Bobby Holley (:bholley)
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8903453 [details]
Bug 1395725 - Don't add children of <area> as servo restyle root.

https://reviewboard.mozilla.org/r/175300/#review180558

::: dom/base/Element.cpp:4339
(Diff revision 1)
>      // We check for a frame to reduce the cases where we need the FFI call.
> -    if (!parent->GetPrimaryFrame() && Servo_Element_IsDisplayNone(parent)) {
> +    // We need to additionally check that it is not an <area> element because
> +    // <area> may have primary frame from <img>, but itself is display:none.
> +    if ((!parent->GetPrimaryFrame() && Servo_Element_IsDisplayNone(parent)) ||
> +        parent->IsHTMLElement(nsGkAtoms::area)) {
>        return;
>      }

Can you separate out the area check into a separate if (...) { return } below? I think it would make the code easier to follow.

	Mozilla Autoland
	Comment 8 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 099a87c95a58 -d 512562477a1e: rebasing 417502:099a87c95a58 "Bug 1395725 - Don't add children of <area> as servo restyle root. r=emilio" (tip)
merging dom/base/Element.cpp
merging layout/style/crashtests/crashtests.list
warning: conflicts while merging layout/style/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 9 • 7 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/feec5f3a726c
Don't add children of <area> as servo restyle root. r=emilio

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/feec5f3a726c