Closed Bug 1370123 Opened 7 years ago Closed 7 years ago

stylo: Assertion failure: IsStyledByServo() when using getComputedStyle on an XHR doc

Categories

(Core :: CSS Parsing and Computation, enhancement, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(5 files, 4 obsolete files)

Bug 1370123 - Add tests for Element.animate when used on an element in a document without a browsing context;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1370123 - Make aContent parameter to nsComputedDOMStyle::GetPresShellForContent const;
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Bug 1370123 - Ignore animation restyle requests for elements in documents without a pres shell;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1370123 - Skip restyling elements in documents without a pres shell;
59 bytes, text/x-review-board-request
heycam
: review+
Details
Test for changing docs after requesting restyle
806 bytes, text/html
Details 
From bug 1355349 comment 37.

In dom/smil/tests/test_smilXHR.html we first do:

  var xhr = new XMLHttpRequest();
  xhr.open("GET", "smilXHR_helper.svg", false);
  xhr.send();
  var xdoc = xhr.responseXML

Then we pull out a circle element from the document:

  var circ = xdoc.getElementById("circ")

Then at the end we check the computed style of the circle:

  is(SMILUtil.getComputedStyleSimple(circ, "opacity"), "1",
     "animation of CSS property shouldn't be taking effect");

The trouble is, in nsComputedDOMStyle::DoGetStyleContextNoFlush we have the following check:

  if (ServoStyleSet* servoSet = styleSet->GetAsServo()) {
    StyleRuleInclusion rules = aStyleType == eDefaultOnly
                               ? StyleRuleInclusion::DefaultOnly
                               : StyleRuleInclusion::All;
    return servoSet->ResolveTransientStyle(aElement, aPseudo, type, rules);
  }

For whatever reason, styleSet->GetAsServo() returns a ServoStyleSet here (I haven't dug into exactly how we look up the styleSet there) so we go ahead and call ResolveTransientStyle.

From there we have the following call stack:

  ServoStyleSet::ResolveTransientStyle
  ServoStyleSet::ResolveTransientServoStyle
  ServoStyleSet::ResolveStyleLazily
  EffectCompositor::PreTraverse

In EffectCompositor::PreTraverse we have:

  bool flushThrottledRestyles = elementToRestyle &&
                                elementToRestyle->HasDirtyDescendantsForServo();

|elementToRestyle| here is the <circle> element but HasDirtyDescendantsForServo contains the assertion:

  MOZ_ASSERT(IsStyledByServo());

which calls:

  nsINode::IsStyledByServo() const
  {
    return OwnerDoc()->IsStyledByServo();
  }

and the OwnerDoc() here is the XHR doc, which is not styled by servo so the assertion fails.

So there's some kind of mismatch between consulting the element's owner doc to see if we're using Servo, and whatever we end up doing to get the style set for the element in DoGetStyleContextNoFlush.
Stability, P1.
Priority: -- → P1
getComputedStyle should resolve the given element based on the rules from the document it is called from, which means that it shouldn't matter whether the owner document of the element is styled by Stylo. If it matters, that indicates that we are probably checking something wrong.

So the question would be, what should happen in Gecko for this case? Do we need to apply animation / transition to an element from a different document when getting computed style? I have a feeling that we probably don't? How SMIL would interact in this case?

We probably should have a separate testcase to check this specific case that calling getComputedStyle on a XHR doc and/or another document in general, and ensure that works as expected.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
I'll put these up for review next week after:

* I check try is ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0905e3307d25d10d8c6a4d432ab2becc56ae9c6a
  [ A try of a similar patch was fine but it's possible I completely messed thing
    up when I updated: https://mail.google.com/mail/u/0/#label/Tryserver/15caaa7236ee55cb ]
* I go through all the comments in part 1 and tighten them up.
Attachment #8878382 - Attachment is obsolete: true
Attachment #8878383 - Attachment is obsolete: true
Comment on attachment 8878984 [details]
Bug 1370123 - Make aContent parameter to nsComputedDOMStyle::GetPresShellForContent const;

https://reviewboard.mozilla.org/r/150264/#review154944
Attachment #8878984 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8878983 [details]
Bug 1370123 - Add tests for Element.animate when used on an element in a document without a browsing context;

https://reviewboard.mozilla.org/r/150262/#review155378

I'm not very familiar with the Web Animations spec, but the new test looks fine as far as I can see.

::: testing/web-platform/tests/web-animations/interfaces/Animatable/animate-data-doc.html:65
(Diff revision 1)
> +    // There is no navigationStart moment for a data document so its default
> +    // document timeline will be inactive.

It is not clear to me from the spec. The Navigation Timing spec doesn't mention anything specifically about document fetched via XHR, and from a literal read of that spec, it seems such document really should have a "navigationStart" moment identical to "fetchStart" [1], although the value is never readable given it doesn't even have an associated Window object.

It is probably still correct that the timeline is inactive, since a document from XHR cannot be active itself. But that really is a different reason than not having the navigationStart moment.


[1] https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart
Attachment #8878983 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8878985 [details]
Bug 1370123 - Ignore animation restyle requests for elements in documents without a pres shell;

https://reviewboard.mozilla.org/r/150266/#review155380

::: dom/animation/EffectCompositor.cpp:972
(Diff revision 1)
> +  // If |aRoot| is null then presumably we are restyling the whole document
> +  // and are not in a data document.

I'm not quite sure about this statement. What would happen if an element in data document is added to `mElementsToRestyle` and we start a whole document restyling?

Should we instead check this for element of each restyle target in `getNeededRestyleTarget`? Or probably we should avoid adding such target into `mElementsToRestyle` at all? If such target isn't added at all, why do we really need this check?
Attachment #8878985 - Flags: review?(xidorn+moz)
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> Should we instead check this for element of each restyle target in
> `getNeededRestyleTarget`? Or probably we should avoid adding such target
> into `mElementsToRestyle` at all? If such target isn't added at all, why do
> we really need this check?

The idea being that if they later become eligible then we will probably trigger a further call to RequestRestyle anyway? Yeah, that's probably true and I guess the tests I added earlier will cover that.
(In reply to Brian Birtles (:birtles) from comment #11)
> (In reply to Xidorn Quan [:xidorn] UTC+10 from comment #10)
> > Should we instead check this for element of each restyle target in
> > `getNeededRestyleTarget`? Or probably we should avoid adding such target
> > into `mElementsToRestyle` at all? If such target isn't added at all, why do
> > we really need this check?
> 
> The idea being that if they later become eligible then we will probably
> trigger a further call to RequestRestyle anyway? Yeah, that's probably true
> and I guess the tests I added earlier will cover that.

Hmm, I think we'll still need to cover the case where an element is transferred from a display doc to a data doc after being added to mElementsToRestyle.
After rebasing this and running the tests I now hit the assertion added by bug 1373725. It seems like there's probably more (non-animation related) work to do for handling data documents.
Comment on attachment 8879839 [details]
Bug 1370123 - Clear styles resolved on documents without a browsing context;

https://reviewboard.mozilla.org/r/151238/#review156076

nsIDocument::GetShell will return null if the document's been put in the bfcache.  I guess if a document goes into the bfcache that we won't try to resolve styles on it, but I couldn't be sure.  And really I don't know how the bfcache works.  Can you find out whether it would be a problem?

The patch otherwise looks good.

::: servo/components/style/traversal.rs:589
(Diff revision 1)
> +    if !in_display_doc || display_none_root.is_some()
> +    {

Nit: "{" on previous line.
Yeah, this patch series has all gotten a bit muddled. I started off using the existence of a pres shell on the composed doc as the condition to check. That's because that's what nsComputedDOMStyle does and so do various other places like Gecko_UpdateAnimations. That also *seems* to me like what we want to check -- i.e. if we don't have a pres shell, then presumably we're not doing document styling (although we might do styling for getComputedStyle etc.)?

But then I've got all the terminology mixed up:

* "Data doc" -- I guess this term really corresponds to nsDocument.mLoadedAs(Interactive)Data == true? i.e. more of a security concern than styling concern?
* "XHR doc" -- I'm probably using this term correctly but I suspect it represents a subset of the cases we actually care about in this bug.
* "Document without a browsing context" -- I wonder if this term corresponds to nsIDocument::GetDocShell() being false as opposed to nsIDocument::GetShell() being false? Do documents in the bfcache have a docshell and no pres shell?

I'll have to look into this a bit more tomorrow but adding bz to CC for now in case he can point me in the right direction.
Attachment #8878985 - Flags: review?(xidorn+moz) → review?(cam)
Attachment #8879838 - Flags: review?(xidorn+moz) → review?(cam)
I'm not very confident on reviewing this, actually... So redirect to heycam.
Boris, I wonder if you could help with Cameron's question in comment 19. Specifically, I'm using the presence or absence of a pres shell as returned by nsIDocument::GetShell to determine if we are in a situation like an responseXML document where we have a document without a browsing context.

If we call getComputedStyle on such an element then we'll resolve the style for the element, but we then need to clear the ServoElementData immediately afterwards, much like we do for display:none subtrees (otherwise we'll fail to GC it since we never call ServoStyleSet::BeginShutdown() in that case).

Cameron's concern is that nsIDocument::GetShell returns null for a document in the bfcache. Is there a situation where that might be problematic? e.g. can we attempt to resolve style on an element in the bfcache, clear that style data, then restore the document from the bfcache and find some invariant about which elements have been styled has been violated?
Flags: needinfo?(bzbarsky)
There are various reasons GetShell() could return null for a document:

1)  It has no browsing context.
2)  It's not the current document in its browsing context (whether bfcached or not).
3)  It is the current document in its browsing context, but the <iframe> containing
    that browsing context has no nsIFrame (e.g. is display:none).

You presumably need to immediately clear the data in cases 1 and 3, because in those cases there is no ServoStyleSet around at all.  But you probably don't want to have your comments and commit messages and such claiming that this only applies to case 1.

For case 2, there probably shouldn't be "normal" restyling going on, but getComputedStyle could still be called for nodes in such a document.

OK, so as far as getComputedStyle behavior goes: it really shouldn't be caching any style information on elements, ever.  For example, say you use window1.getComputedStyle(node) where "node" comes from window2 and is "display:none".  One invariant stylo has, I think, is that the node needs to have style stored on it, and that style needs to correspond to the style rules of window2.  But the getComputedStyle call in this case will resolve style using the style rules for window1.  Caching that on the node sounds pretty odd.  Clearing it from the node afterward will violate the invariant stylo tries to maintain (that display:none subtree roots have style data), if I understand the invariant correctly.  Which is very much not a given.

> can we attempt to resolve style on an element in the bfcache, clear that style data,
> then restore the document from the bfcache

Yes.

> and find some invariant about which elements have been styled has been violated

I don't quite know, because I don't quite know what invariants stylo has here.  A brief skim of the "restore from bfcache" logic is not showing me any forced restyling in there.  bfcache stores the entire frame tree, so as things stand it would be pretty much like clearing style on some random element in a document while it's showing.

One thing you _could_ do to alleviate this problem is to just drop the cached document from the cache if someone tries to use computed style on a node in it.  We already do this when any DOM mutations occur in the bfcached doc.  See nsSHEntryShared::RemoveFromBFCacheAsync and its callers.

Does that help?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #28)
> There are various reasons GetShell() could return null for a document:
> 
> 1)  It has no browsing context.
> 2)  It's not the current document in its browsing context (whether bfcached
> or not).
> 3)  It is the current document in its browsing context, but the <iframe>
> containing
>     that browsing context has no nsIFrame (e.g. is display:none).
> 
> You presumably need to immediately clear the data in cases 1 and 3, because
> in those cases there is no ServoStyleSet around at all.  But you probably
> don't want to have your comments and commit messages and such claiming that
> this only applies to case 1.

At very least, it sounds like I need to replace a few references to "without a browsing context" to "without a pres shell".

> For case 2, there probably shouldn't be "normal" restyling going on, but
> getComputedStyle could still be called for nodes in such a document.
> 
> OK, so as far as getComputedStyle behavior goes: it really shouldn't be
> caching any style information on elements, ever.  For example, say you use
> window1.getComputedStyle(node) where "node" comes from window2 and is
> "display:none".  One invariant stylo has, I think, is that the node needs to
> have style stored on it, and that style needs to correspond to the style
> rules of window2.  But the getComputedStyle call in this case will resolve
> style using the style rules for window1.  Caching that on the node sounds
> pretty odd.  Clearing it from the node afterward will violate the invariant
> stylo tries to maintain (that display:none subtree roots have style data),
> if I understand the invariant correctly.  Which is very much not a given.

I'm not entirely sure I follow. If we're only talking about a display:none subtree, then we'll use window2's style rules since nsComputedDOMStyle always uses the node document's pres shell if it exists. And in that case we'll only clear style data up to the display:none subtree root due to the `if in_doc && curr == display_none_root.unwrap() { break; }` check in traversal.rs.

So I assume we're talking about case (3) from above where the whole iframe is display:none. In that case we'll use window1's style rules. When we're done we'll clear style up to the root as determined by `traversal_parent()` (i.e. Gecko_GetFlattenedTreeParentNode). Is that the potential problem?

> One thing you _could_ do to alleviate this problem is to just drop the
> cached document from the cache if someone tries to use computed style on a
> node in it.  We already do this when any DOM mutations occur in the bfcached
> doc.  See nsSHEntryShared::RemoveFromBFCacheAsync and its callers.
> 
> Does that help?

Yes, that's great, thank you! I'll have try to work up a patch to call RemoveFromBFCacheAsync when we go to do getComputedStyle for an element from a doc in the bfcache.
> then we'll use window2's style rules since nsComputedDOMStyle
> always uses the node document's pres shell if it exists.

No, it uses the presshell from the document stored in mDocumentWeak, which is the document of the window that was "this" when getComputedStyle was called.  In the situation I was describing, that would be the document in window1.  The node document of the node is the document in window2.  So if I understand the stylo side correctly we'll end up storing style based on rules in window1 on a node that lives in window2.  This could all be happening while window2 has a presshell and all that, so it's not one of the three "no pres shell" cases at all.
(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #30)
> > then we'll use window2's style rules since nsComputedDOMStyle
> > always uses the node document's pres shell if it exists.
> 
> No, it uses the presshell from the document stored in mDocumentWeak, which
> is the document of the window that was "this" when getComputedStyle was
> called.  In the situation I was describing, that would be the document in
> window1.  The node document of the node is the document in window2.  So if I
> understand the stylo side correctly we'll end up storing style based on
> rules in window1 on a node that lives in window2.  This could all be
> happening while window2 has a presshell and all that, so it's not one of the
> three "no pres shell" cases at all.

Followed up with bz on IRC about this. There was some confusion because we later added this code which makes us use the presshell for the element's document, if any.

  http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/layout/style/nsComputedDOMStyle.cpp#588

That's not what the spec says, however. But I'm not sure anyone actually follows the spec here anyway (at least they don't in my testing of XHR docs). Apparently there has been discussion about this before (I wasn't there) but it doesn't look like the spec has been updated since (the current spec text hasn't changed since at least 2013, perhaps longer.)

So it seems that at least this particular case is still ok. I still need to drop the cached document if we compute styles on it while in the bfcache (and update various misleading commit messages, comments etc.).
Clearing review flags for this for now since I suspect we won't need the last patch once Bobby's patch for bug 1377010 lands.
Attachment #8879839 - Attachment is obsolete: true
Attachment #8883175 - Attachment is obsolete: true
It seems I don't need some of the extra patches in this bug in order to avoid assertions about rule nodes being GC'ed now that bug 1377010 has landed.

I've filed bug 1378284, however, since landing them still might be a good idea once I work out what some of the issues with them were.
Comment on attachment 8878985 [details]
Bug 1370123 - Ignore animation restyle requests for elements in documents without a pres shell;

https://reviewboard.mozilla.org/r/150266/#review159500

::: dom/animation/EffectCompositor.cpp:264
(Diff revision 4)
>      return;
>    }
>  
> -  // Ignore animations on orphaned elements.
> -  if (!aElement->IsInComposedDoc()) {
> +  // Ignore animations on orphaned elements and elements in documents without
> +  // a pres shell (e.g. XMLHttpRequest responseXML documents).
> +  if (!nsComputedDOMStyle::GetPresShellForContent(aElement)) {

This function really should move from nsComputedDOMStyle to somewhere more appropriate...
Attachment #8878985 - Flags: review?(cam) → review+
Comment on attachment 8879838 [details]
Bug 1370123 - Skip restyling elements in documents without a pres shell;

https://reviewboard.mozilla.org/r/151236/#review159504

::: dom/animation/EffectCompositor.cpp:1003
(Diff revision 3)
> +    if (!nsComputedDOMStyle::GetPresShellForContent(target.mElement)) {
> +      return returnTarget;
> +    }

Is the only problem with not having this check here that we would end up calling PostRestyleEvent for an element that is now in another document, and that element's ElementData could (if it's a non-displayed document) just hang around indefinitely, or maybe cause problems if moved into yet another document?

If so, then I wonder if instead we should check in ServoRestyleManager::PostRestyleEvent that the element is in the document that restyle manager is for.  (Although maybe we should do that, and warn/assert, and still have the pres shell check here.)

::: dom/animation/EffectCompositor.cpp:1088
(Diff revision 3)
> +    // If this is a full document restyle, then unconditionally clear
> +    // elementSet in case there are any elements that didn't match above
> +    // because they were moved to a document without a pres shell after
> +    // posting an animation restyle.
> +    if (!aRoot && flushThrottledRestyles) {
> +      elementSet.Clear();
> -  }
> +    }

An alternative would be to ensure the element gets removed the set in Element::UnbindFromTree.  We already do a bunch of animation-related content property removals in UnbindFromTree.  Would that work?
(In reply to Cameron McCormack (:heycam) from comment #40)
> Comment on attachment 8879838 [details]
> Bug 1370123 - Skip restyling elements in documents without a pres shell;
> 
> https://reviewboard.mozilla.org/r/151236/#review159504
> 
> ::: dom/animation/EffectCompositor.cpp:1003
> (Diff revision 3)
> > +    if (!nsComputedDOMStyle::GetPresShellForContent(target.mElement)) {
> > +      return returnTarget;
> > +    }
> 
> Is the only problem with not having this check here that we would end up
> calling PostRestyleEvent for an element that is now in another document, and
> that element's ElementData could (if it's a non-displayed document) just
> hang around indefinitely, or maybe cause problems if moved into yet another
> document?

I wasn't really thinking about ElementData so much. From memory, I was just thinking that as of the third patch in this series, we ignore such elements if they are passed to RequestRestyle and so the most consistent this to do is to also ignore them here, if they happen to become elements in non-displayed documents in between calling RequestRestyle and PreTraverseInSubtree.

> If so, then I wonder if instead we should check in
> ServoRestyleManager::PostRestyleEvent that the element is in the document
> that restyle manager is for.  (Although maybe we should do that, and
> warn/assert, and still have the pres shell check here.)

I think that's probably a useful additional check to add. I don't know that it's 100% overlapping though. I think in EffectCompositor we can easily check for elements in non-display documents but checking that the composed document matches the restyle manager's document would require exposing some extra information from the restyle manager and is probably best checked by the restyle manager itself.

So, my current thinking is:

a) Keep the check for elements in non-display documents in EffectCompositor (in order to be consistent with the handling when such elements are passed to RequestRestyle)
b) Add a further check that an element's composed doc matches the ServoRestyleManager's mPresContext->Document() to ServoRestyleManager::PostRestyleEventForAnimations and silently skip such requests if they don't match.

Presumably if we ever hit (b) it's because we have moved the element to a different document and in doing that we should have generated a restyle request on the EffectCompositor associated with the new document's pres context (via UpdateProperties at least).

> ::: dom/animation/EffectCompositor.cpp:1088
> (Diff revision 3)
> > +    // If this is a full document restyle, then unconditionally clear
> > +    // elementSet in case there are any elements that didn't match above
> > +    // because they were moved to a document without a pres shell after
> > +    // posting an animation restyle.
> > +    if (!aRoot && flushThrottledRestyles) {
> > +      elementSet.Clear();
> > -  }
> > +    }
> 
> An alternative would be to ensure the element gets removed the set in
> Element::UnbindFromTree.  We already do a bunch of animation-related content
> property removals in UnbindFromTree.  Would that work?

I'm concerned that doing that would mean a lot of hashmap lookups when doing document surgery. By clearing it lazily here or as part of iterating over the hashmap we should be able to avoid additional lookups.
(In reply to Brian Birtles (:birtles) from comment #41)
> I think that's probably a useful additional check to add. I don't know that
> it's 100% overlapping though. I think in EffectCompositor we can easily
> check for elements in non-display documents but checking that the composed
> document matches the restyle manager's document would require exposing some
> extra information from the restyle manager and is probably best checked by
> the restyle manager itself.

Then again, we could always compare against EffectCompositor.mPresContext->Document()? That might be simpler.
(In reply to Brian Birtles (:birtles) from comment #41)
> So, my current thinking is:
> 
> a) Keep the check for elements in non-display documents in EffectCompositor
> (in order to be consistent with the handling when such elements are passed
> to RequestRestyle)
> b) Add a further check that an element's composed doc matches the
> ServoRestyleManager's mPresContext->Document() to
> ServoRestyleManager::PostRestyleEventForAnimations and silently skip such
> requests if they don't match.

Thinking about this a little further, I'm not sure if this is needed. There is some unclear spec text about being able to resolve styles for an element using the styles of a different document. No one implements that, but I wonder if there's really any value in adding this check. I'm not aware of us hitting any problems in this area so I'm inclined to just keep the check for non-display documents, i.e. (a) from above, and not add (b) until we find we need it.
Here's a test for changing documents after requesting an animation restyle on the previous document. Nothing blows up and everything appears to work the same as with the Gecko style system.
In the end, all I've done here is rebase and update the comment in the last patch to make it a little more clear why we skip elements in non-display documents in EffectCompositor::PreTraverseInSubtree.
Comment on attachment 8879838 [details]
Bug 1370123 - Skip restyling elements in documents without a pres shell;

https://reviewboard.mozilla.org/r/151236/#review161516

Thanks for working through the alternatives.
Attachment #8879838 - Flags: review?(cam) → review+
Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9805d5b89d7
Add tests for Element.animate when used on an element in a document without a browsing context; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/cd8e522d2798
Make aContent parameter to nsComputedDOMStyle::GetPresShellForContent const; r=xidorn
https://hg.mozilla.org/integration/autoland/rev/9858b929fbb6
Ignore animation restyle requests for elements in documents without a pres shell; r=heycam
https://hg.mozilla.org/integration/autoland/rev/f8610fc5aa2f
Skip restyling elements in documents without a pres shell; r=heycam
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: