Closed Bug 1393791 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

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

People

(Reporter: truber, Assigned: bholley)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 1 obsolete file)

Attached file 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
Flags: in-testsuite?
Assignee: nobody → bobbyholley
Priority: -- → P2
Attached patch Crashtest.Splinter Review
MozReview-Commit-ID: IWqnaFxC1Nx
Attachment #8901518 - Flags: review+
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
Attachment #8901519 - Flags: review?(emilio+bugs)
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.
Attachment #8901519 - Flags: review?(emilio+bugs) → review+
With patches to move nsSVGUseElement::mClone to nsSVGUseFrame:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=611822288aa777c1576e435ce856c7ba146b544a
MozReview-Commit-ID: FcD2Nh7zcRO
Attachment #8901610 - Flags: review?(cam)
This brings it into alignment with what everything else does.

MozReview-Commit-ID: 2A9p8umHnKi
Attachment #8901611 - Flags: review?(cam)
MozReview-Commit-ID: 1JYAhiXKVJC
Attachment #8901612 - Flags: review+
Attachment #8901519 - Attachment is obsolete: true
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.
Attachment #8901610 - Flags: review?(cam) → review?(emilio)
Attachment #8901611 - Flags: review?(emilio)
Attachment #8901611 - Flags: review?(cam)
Attachment #8901611 - Flags: feedback?(cam)
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.
Attachment #8901610 - Flags: review?(emilio) → review+
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.
Attachment #8901611 - Flags: review?(emilio) → review+
(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.
(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/.
Pushed by bholley@mozilla.com:
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 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.
Attachment #8901611 - Flags: feedback?(cam) → feedback+
Flags: in-testsuite? → in-testsuite+
Depends on: 1396041
You need to log in before you can comment on or make changes to this bug.