1393791 - stylo: Assertion failure: aElement->GetComposedDoc()->GetServoRestyleRoot()

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes an assertion in m-c rev 20170825-2306e153fba9 with stylo enabled by pref.

Assertion failure: aElement->GetComposedDoc()->GetServoRestyleRoot(), at /home/worker/workspace/build/src/dom/base/Element.cpp:4315
0: NoteDirtyElement, at dom/base/Element.cpp:4301
1: style::gecko::wrapper::GeckoElement::maybe_restyle, at servo/components/style/gecko/wrapper.rs:712
2: core::option::Option<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>>::and_then<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>,&mut style::data::RestyleData,closure>, at src/libcore/option.rs:605
3: style::gecko::wrapper::GeckoElement::note_explicit_hints, at servo/components/style/gecko/wrapper.rs:736
4: geckoservo::glue::Servo_NoteExplicitHints, at servo/ports/geckolib/glue.rs:2856
5: mozilla::ServoRestyleManager::AttributeChanged, at layout/base/ServoRestyleManager.cpp:1338
6: mozilla::PresShell::AttributeChanged, at layout/base/RestyleManagerInlines.h:72
7: nsNodeUtils::AttributeChanged, at dom/base/nsNodeUtils.cpp:145
8: mozilla::dom::Element::SetAttrAndNotify, at dom/base/Element.cpp:2651
9: nsStyledElement::SetInlineStyleDeclaration, at dom/base/nsStyledElement.cpp:112
10: nsDOMCSSDeclaration::ParsePropertyValue, at layout/style/nsDOMCSSDeclaration.cpp:345
11: mozilla::dom::CSS2PropertiesBinding::set_height, at layout/style/nsCSSPropList.h:2284
12: mozilla::dom::GenericBindingSetter, at dom/bindings/BindingUtils.cpp:3014
13: js::CallJSNative, at js/src/jscntxtinlines.h:293

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Crashtest.

MozReview-Commit-ID: IWqnaFxC1Nx

Comment 4

[Bobby Holley (:bholley)]

Attachment: Stop unbinding native-anonymous content off a script runner. v1

The failure mode in the attached crashtest is an inconsistency in the flattened
tree. Specifically, we null out mVideoControls in an nsVideoFrame, but defer
the UnbindFromTree call on that NAC element, which measn that its mParent still
points to the nsVideoFrame's mContent. Because all this stuff runs off of script
runners, and the anonymous content destroyer is not guaranteed to run before
other potential script runners, we end up running arbitrary script while the
tree mismatch exists. This script calls back into ProcessPendingRestyles, which
causes trouble.

We could build a separate deferral mechanism, but it's not clear that we actually
need to defer the unbind anymore. The deferred unbind was added in bug 489008,
which predated a lot of simplifications in layout/dom interaction.

MozReview-Commit-ID: 1JYAhiXKVJC

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8901519 [details] [diff] [review]
Stop unbinding native-anonymous content off a script runner. v1

Review of attachment 8901519 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, modulo the SVG use bit. Please ask for review from Cam or someone else familiar with that.

::: dom/svg/SVGUseElement.cpp
@@ +319,5 @@
>  
>  void
>  SVGUseElement::DestroyAnonymousContent()
>  {
> +  if (mClone) {

I think this should be moved to nsSVGUseFrame::DestroyFrom instead, and use the same mechanism that the rest of the NAC. Or at least use that mechanism from there.

This gets also called from unlink code, in which case I think it's fine to just do this... But please check with someone more familiar than me with SVG.

Comment 6

[Bobby Holley (:bholley)]

With patches to move nsSVGUseElement::mClone to nsSVGUseFrame:

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

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 1 - Hoist nsSVGUseFrame into a header. v1

MozReview-Commit-ID: FcD2Nh7zcRO

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 2 - Move SVG Use anonymous content to the frame. v1

This brings it into alignment with what everything else does.

MozReview-Commit-ID: 2A9p8umHnKi

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop unbinding native-anonymous content off a script runner. r=emilio

MozReview-Commit-ID: 1JYAhiXKVJC

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8901610 [details] [diff] [review]
Part 1 - Hoist nsSVGUseFrame into a header. v1

On second thought, I think the svg:use stuff is straightforward enough that emilio can just review it, given that we discussed it together. I'll flag heycam for retro-feedback when he has time.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8901610 [details] [diff] [review]
Part 1 - Hoist nsSVGUseFrame into a header. v1

Review of attachment 8901610 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/svg/nsSVGUseFrame.h
@@ +7,5 @@
> +#define __NS_SVGUSEFRAME_H__
> +
> +// Keep in (case-insensitive) order:
> +#include "nsIAnonymousContentCreator.h"
> +#include "nsSVGEffects.h"

Is this used in the header? If so, let's move it to the cpp file.

@@ +9,5 @@
> +// Keep in (case-insensitive) order:
> +#include "nsIAnonymousContentCreator.h"
> +#include "nsSVGEffects.h"
> +#include "nsSVGGFrame.h"
> +#include "mozilla/dom/SVGUseElement.h"

Ditto.

@@ +10,5 @@
> +#include "nsIAnonymousContentCreator.h"
> +#include "nsSVGEffects.h"
> +#include "nsSVGGFrame.h"
> +#include "mozilla/dom/SVGUseElement.h"
> +#include "nsContentList.h"

Ditto,

@@ +36,5 @@
> +                    nsIFrame*         aPrevInFlow) override;
> +
> +  virtual nsresult  AttributeChanged(int32_t         aNameSpaceID,
> +                                     nsIAtom*        aAttribute,
> +                                     int32_t         aModType) override;

While you do this, feel free to clang-format this file, and remove all the extra `virtual`s (maybe in a different commit with rs=me).

@@ +59,5 @@
> +
> +private:
> +  bool mHasValidDimensions;
> +};
> +

nit: too many newlines.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8901611 [details] [diff] [review]
Part 2 - Move SVG Use anonymous content to the frame. v1

Review of attachment 8901611 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine to me, the old setup was really weird.

Maybe Cam has a bit more background on why it was that way, but this makes much more sense to me.

::: dom/svg/SVGUseElement.cpp
@@ +316,4 @@
>  bool
>  SVGUseElement::OurWidthAndHeightAreUsed() const
>  {
> +  auto frame = static_cast<nsSVGUseFrame*>(GetPrimaryFrame());

nit: I usually always use `auto*` for pointers, but I know not doing it isn't particularly error-prone in this case.

Also, maybe:

MOZ_ASSERT_IF(GetPrimaryFrame(), GetPrimaryFrame()->IsSVGUseFrame())?

(Seems easier to debug if someone messes up frame construction, and who knows who may do that in the future? ;))

@@ +339,2 @@
>    if (OurWidthAndHeightAreUsed()) {
> +    nsSVGElement *target = static_cast<nsSVGElement*>(clone);

nit: While at it, maybe move the star to the right or change to `auto*`?

::: layout/svg/nsSVGUseFrame.h
@@ +62,3 @@
>  private:
>    bool mHasValidDimensions;
> +  nsCOMPtr<nsIContent> mClone;

Maybe this member should be named mContentClone or something like that now? Ditto for the getter.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12)
> @@ +339,2 @@
> >    if (OurWidthAndHeightAreUsed()) {
> > +    nsSVGElement *target = static_cast<nsSVGElement*>(clone);
> 
> nit: While at it, maybe move the star to the right or change to `auto*`?

Err, to the left.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> ::: layout/svg/nsSVGUseFrame.h
> @@ +7,5 @@
> > +#define __NS_SVGUSEFRAME_H__
> > +
> > +// Keep in (case-insensitive) order:
> > +#include "nsIAnonymousContentCreator.h"
> > +#include "nsSVGEffects.h"
> 
> Is this used in the header? If so, let's move it to the cpp file.

And this is of course s/If so/if not/.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b564008830c4
Crashtest. r=me
https://hg.mozilla.org/integration/autoland/rev/69e1415e3ca0
Hoist nsSVGUseFrame into a header. r=emilio
https://hg.mozilla.org/integration/autoland/rev/34d0b762abdb
Move SVG Use anonymous content to the frame. r=emilio
https://hg.mozilla.org/integration/autoland/rev/589f8ecf173b
Stop unbinding native-anonymous content off a script runner. r=emilio

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8901611 [details] [diff] [review]
Part 2 - Move SVG Use anonymous content to the frame. v1

Review of attachment 8901611 [details] [diff] [review]:
-----------------------------------------------------------------

Moving this to the frame seems fine.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b564008830c4
https://hg.mozilla.org/mozilla-central/rev/69e1415e3ca0
https://hg.mozilla.org/mozilla-central/rev/34d0b762abdb
https://hg.mozilla.org/mozilla-central/rev/589f8ecf173b