Closed Bug 1331294 Opened 8 years ago Closed 8 years ago

stylo: value from getComputedStyle is not updated after selector matching changes

Categories

(Core :: DOM: CSS Object Model, defect)

Product:
Core
Component:
DOM: CSS Object Model
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: heycam)

References

Details

Attachments

(7 files, 1 obsolete file)

Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1331294 - Part 1: Make nsPresContext::HasPendingRestyleOrReflow check nsIDocument::mNeedStyleFlush instead of asking the RestyleManager for its status.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Make Servo_TraverseSubtree return whether it did any work.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1331294 - Part 3: Make ServoRestyleManager::PostRestyleEvent not check HasPendingRestyles() before returning early.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1331294 - Part 4: Make ServoStyleSet::StyleDocument call Servo_TraverseSubtree unconditionally, and return whether it did any work.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1331294 - Part 5: Make ServoRestyleManager::ProcessPendingRestyles use StyleDocument()'s return value to determine whether to return early.
59 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1331294 - Part 6: Remove RestyleManager::HasPendingRestyles and Servo_Element_ShouldTraverse.
59 bytes, text/x-review-board-request
bholley
: review+
Details 
See the following code:
<!DOCTYPE html>
<style>
a { color: red; }
a.x { color: green; }
</style>
<a>hello world</a>
<script>
let $a = document.querySelector('a');
alert(getComputedStyle($a).color);
$a.classList.add('x');
alert(getComputedStyle($a).color);
</script>

The two alerts both show the red color, while at the second time, it should really show the green color.

It only happens when you really get the computed value before. If the first getComputedStyle is removed, the second one would happily return the correct value.

(I thought it may be fixed by bug 1330874, but after applying the patch there, this bug does not disappear.)

This looks like something we should fix soonish, since some of the style system tests heavily rely on values returned from getComputedStyle.
(This is an issue revealled from /layout/style/test/test_bug387615.html)
At the moment, when we detect an attribute value change, we record the element snapshot, but we don't do anything to cause us to notice that restyles are pending.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Comment on attachment 8827018 [details]
Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.

https://reviewboard.mozilla.org/r/104842/#review105534

This shouldn't be happening. Servo_Element_GetSnapshot invokes maybe_restyle, which should propagate the dirty_descendants bit up the chain, which should cause us to notice a pending restyle. So we should debug and figure out what's going on with that.
Attachment #8827018 - Flags: review?(bobbyholley) → review-
I think then we have multiple bits of state recording whether we have pending restyles, the one that this patch used being nsIDocument::mNeedStyleFlush, which is set by the call in RestyleManagerBase::PostRestyleEventInternal.
(In reply to Cameron McCormack (:heycam) from comment #5)
> I think then we have multiple bits of state recording whether we have
> pending restyles, the one that this patch used being
> nsIDocument::mNeedStyleFlush, which is set by the call in
> RestyleManagerBase::PostRestyleEventInternal.

I guess we should refactor all that somehow such that any servo-backed document which asks that question will (also?) check the bits.
nsIDocument::SetNeedStyleFlush forwards that flag up to the parent document (I think that's for cases like printing), so I guess we might still need to call it.

I notice that there are some things which call SetNeedStyleFlush, which don't also post specific restyles, so perhaps the "restyle flushing" mechanism of the document is used for more than just processing pending restyles themselves.  Maybe at some point down the track we can eliminate mNeedStyleFlush and have it check RestyleManager::HasPendingRestyles() directly, if we work these out.

So I'm inclined to just use this PostRestyleEventInternal call for now.
(In reply to Cameron McCormack (:heycam) from comment #7)
> nsIDocument::SetNeedStyleFlush forwards that flag up to the parent document
> (I think that's for cases like printing), so I guess we might still need to
> call it.

The display document, not the parent document right? I don't know how that setup works, but can we teach the display document how to check the state of the documents it's displaying?

> I notice that there are some things which call SetNeedStyleFlush, which
> don't also post specific restyles, so perhaps the "restyle flushing"
> mechanism of the document is used for more than just processing pending
> restyles themselves.  Maybe at some point down the track we can eliminate
> mNeedStyleFlush and have it check RestyleManager::HasPendingRestyles()
> directly, if we work these out.
> 
> So I'm inclined to just use this PostRestyleEventInternal call for now.

That seems broken though. All the Servo code assumes that it can communicate the necessity of a traversal by bubbling the bits up the tree. If we decide that assumption doesn't hold, we need to do a wider audit.
Also, I think the display document stuff is for svg, not printing. And it looks like we can find all the sub-documents with EnumerateExternalResources, so we should be able to just delegate to the same accessor on any sub-documents (this accessor would presumably check mNeedsStyleFlush as well as ShouldTraverseForServo).
OK, let me look into replacing the use of mNeedStyleFlush with RestyleManager::HasPendingRestyles, for Servo-styled documents.
Ok, sounds good.

Doing a quick audit, I _think_ the only consumers that rely on this assumption are Servo_NoteExplicitHints and Servo_Element_GetSnapshot. So if the refactoring looks like too much of a distraction, I guess I'm ok with keeping the Gecko model for now and landing this patch. Up to you.
Comment on attachment 8827018 [details]
Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.

https://reviewboard.mozilla.org/r/104842/#review105548

Ok with this for now if making things more dynamic turns out to be too involved.
Attachment #8827018 - Flags: review- → review+
After looking at this again, I think we should do something different.  To change the "does the document need a style flush" check to look at the Servo state would mean:

* we need to keep mNeedStyleFlush anyway, since it is used for processing some style related things that don't necessarily involve traversing the tree to restyle
* the NeedStyleFlush() check would need to call the virtual EnumerateResourceDocuments, to poll those documents for style flush needs, which seems like overhead we don't need
* the check for whether traversal is needed actually means using a DocumentStyleRootIterator and look at each of those
* the check on each style root involves not just the bit check but also the Servo_Element_ShouldTraverse FFI call

So we go from a simple bit check to some complex processing.

Since we have bindgened nsIDocument, I think instead we should just teach the Servo side to set mNeedStyleFlush after it has gone up the tree setting bits.  We can also make it follow the mDisplayDocument chain set set mNeedStyleFlush up there too.
This patch uses an FFI call instead of going through the bindgened objects.
Comment on attachment 8827018 [details]
Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.

mozreview thinks this still has r+...
Attachment #8827018 - Flags: review+ → review?(bobbyholley)
Comment on attachment 8827018 [details]
Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.

https://reviewboard.mozilla.org/r/104842/#review106210

A few things:
* In general I'm ok with switching from pull to push. However, if we're doing this, then shouldn't we remove the Element::ShouldTraverseForServo stuff? It's not clear to me why we need two systems.
* Will something call SetNeedStyleFlush if the document is removed from the document and reinserted? That will drop the style data, and null mServoData on the root is supposed to mean "needs styling". Normally insertions are handled in nsCSSFrameConstructor::Content{Appended,Inserted}, but there's a special case at the top of ContentRangeInserted to handle the !aContainer case, so we may need to call SetNeedsStyleFlush there.
* I don't think having this on TElement or being handled in the servo layout code is really buying us anything. This setup doesn't mesh all that well with the way Servo does things, so I'd rather keep them separate.
* Given that the whole "dirty descendants" thing is something that we only set on Elements, it's a bit weird to talk about the document having dirty descendants. I'd rather just explicitly call Gecko_SetOwnerDocumentNeedsStyleFlush in maybe_restyle.

::: layout/style/ServoBindings.cpp:249
(Diff revision 2)
> +void
> +Gecko_SetOwnerDocumentNeedsStyleFlush(RawGeckoElementBorrowed aElement)
> +{
> +  aElement->OwnerDoc()->SetNeedStyleFlush();
> +}

Please assert main thread here.
Attachment #8827018 - Flags: review?(bobbyholley) → review-
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #17)
> * In general I'm ok with switching from pull to push. However, if we're
> doing this, then shouldn't we remove the Element::ShouldTraverseForServo
> stuff? It's not clear to me why we need two systems.

OK.  Then I think we should remove the error!() that's in traverse_subtree in glue.rs when it detects !token.should_traverse(), and just unconditionally call Servo_TraverseSubtree on all style roots inside ServoStyleSet::StyleDocument.

Since we have these uses of mNeedStyleFlush that don't correspond to "we must traverse the tree", and we will use mNeedStyleFlush to determine whether we call ServoStyleSet::StyleDocument, I think it makes sense for this not to be an error condition.

But we do also have this RestyleManager::HasPendingRestyles call, which is meant to represent the more specific case of "we must traverse the tree", and that is currently calling in to ShouldTraverseForServo on each of the style roots.  HasPendingRestyles is used for some specific things in the Font Loading API where we must wait to resolve some Promise objects if we have pending restyles (or layout).  Though I suppose it wouldn't be that bad to just make that look at mNeedStyleFlush.  I'll try that.

> * Will something call SetNeedStyleFlush if the document is removed from the
> document and reinserted? That will drop the style data, and null mServoData
> on the root is supposed to mean "needs styling". Normally insertions are
> handled in nsCSSFrameConstructor::Content{Appended,Inserted}, but there's a
> special case at the top of ContentRangeInserted to handle the !aContainer
> case, so we may need to call SetNeedsStyleFlush there.

I'll check, thanks.
(In reply to Cameron McCormack (:heycam) from comment #18)
> > * Will something call SetNeedStyleFlush if the document is removed from the
> > document and reinserted? That will drop the style data, and null mServoData
> > on the root is supposed to mean "needs styling". Normally insertions are
> > handled in nsCSSFrameConstructor::Content{Appended,Inserted}, but there's a
> > special case at the top of ContentRangeInserted to handle the !aContainer
> > case, so we may need to call SetNeedsStyleFlush there.
> 
> I'll check, thanks.

Looks like we don't need to add anything, since the StyleDocument() call we do soon after that will (with its new behaviour of always calling Servo_TraverseSubtree on all of the style roots) handle it.

There is a problem where the <style> elements that were reinserted along with the rest of the document don't affect the styles we get, but that seem to be a pre-existing issue.
Attachment #8827817 - Flags: review?(bobbyholley)
Comment on attachment 8827816 [details]
Bug 1331294 - Part 1: Make nsPresContext::HasPendingRestyleOrReflow check nsIDocument::mNeedStyleFlush instead of asking the RestyleManager for its status.

https://reviewboard.mozilla.org/r/105402/#review106406

I'm unfamiliar enough with the font loading stuff that I wouldn't stamp this coming from someone else, but I think
Attachment #8827816 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827018 [details]
Bug 1331294 - Part 2: Call nsIDocument::SetNeedStyleFlush after propagating dirty descendants bits up the tree.

https://reviewboard.mozilla.org/r/104842/#review106408

::: servo/ports/geckolib/glue.rs:941
(Diff revision 3)
>      while let Some(parent) = curr.parent_element() {
>          curr = parent;
>          if curr.has_dirty_descendants() { break; }
>          curr.set_dirty_descendants();
>      }
> +    Gecko_SetOwnerDocumentNeedsStyleFlush(element.0);

Nit: please just explicitly namespace this as bindings::Gecko_SetOwnerDocumentNeedsStyleFlush and drop the |use|.
Attachment #8827018 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827818 [details]
Bug 1331294 - Part 3: Make ServoRestyleManager::PostRestyleEvent not check HasPendingRestyles() before returning early.

https://reviewboard.mozilla.org/r/105406/#review106414

Looks like this was added in bug 1287951 to ensure that we tracked snapshots even when there was no explicit hint. That code has been refactored now, and is handled in maybe_restyle from servo. So I think we're good here.
Attachment #8827818 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827817 [details]
Make Servo_TraverseSubtree return whether it did any work.

https://reviewboard.mozilla.org/r/105404/#review106416

So, I think the abstraction this uses isn't quite right, which may make this misbehave in corner cases. Specifically, the concept of whether the traversal "does any work" is a bit under-defined, and I think the patch as-is will return |false| for a subtree with an explicit change hint on the root and nothing else (causing us to miss processing that change hint).

What we really want to know here is whether there might be any RestyleData to consume, or any lazy frame construction to do, or any ElementSnapshots to drop, and thus whether a post-traversal is required. The Servo infrastructure tracks this nicely with the dirty descendants bit, and we should leverage that here.

So I think we should handle this at the end of Servo_TraverseSubtree by returning |root.has_dirty_descendants() || root.mutate_data().unwrap().has_restyle()| [1]. And update the docs to match.

[1] We should mutate because it's faster than an immutable borrow - we should probably add an alias to mutate_data() called borrow_data_exclusive() for readability.
Attachment #8827817 - Flags: review?(bobbyholley) → review-
Comment on attachment 8827819 [details]
Bug 1331294 - Part 4: Make ServoStyleSet::StyleDocument call Servo_TraverseSubtree unconditionally, and return whether it did any work.

https://reviewboard.mozilla.org/r/105408/#review106424

r=me with those things addressed.

::: layout/style/ServoStyleSet.h:129
(Diff revision 1)
>    /**
>     * Performs a Servo traversal to compute style for all dirty nodes in the
> -   * document. The root element must be non-null.
> +   * document.  This will traverse all of the document's style roots (that
> +   * is, its document element, and the roots of the document-level native
> +   * anonymous content).  Returns true if it did any traversing, and false
> +   * if all of the style roots were clean.

Change this to "returns true if a post-traversal is required".

::: layout/style/ServoStyleSet.cpp:512
(Diff revision 1)
> -void
> +bool
>  ServoStyleSet::StyleDocument()
>  {
>    // Restyle the document from the root element and each of the document level
>    // NAC subtree roots.
> +  bool traversedAny = false;

Rename this variable appropriately.

::: layout/style/ServoStyleSet.cpp:527
(Diff revision 1)
>  
>  void
>  ServoStyleSet::StyleNewSubtree(Element* aRoot)
>  {
>    MOZ_ASSERT(!aRoot->HasServoData());
>    Servo_TraverseSubtree(aRoot, mRawSet.get(), TraversalRootBehavior::Normal);

We should assert that this returns false.

::: layout/style/ServoStyleSet.cpp:533
(Diff revision 1)
>  }
>  
>  void
>  ServoStyleSet::StyleNewChildren(Element* aParent)
>  {
>    Servo_TraverseSubtree(aParent, mRawSet.get(),

Comment that we probably can't assert that this returns false because the root may have other already-styled children that hypothetically need pending restyles, or the root might need restyling itself (which would be inefficient, because it might invalidate all the computation we're doing, but we probably don't have strong enough invarants to assert against it).
Attachment #8827819 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827820 [details]
Bug 1331294 - Part 5: Make ServoRestyleManager::ProcessPendingRestyles use StyleDocument()'s return value to determine whether to return early.

https://reviewboard.mozilla.org/r/105410/#review106428

::: layout/base/ServoRestyleManager.cpp:323
(Diff revision 1)
> -  // XXXbholley: Should this be while() per bug 1316247?
> -  if (HasPendingRestyles()) {
> -    mInStyleRefresh = true;
> +  mInStyleRefresh = true;
> -    styleSet->StyleDocument();
>  
> +  // XXXbholley: Should this be while() per bug 1316247?

I think this comment can go. We handle reentrant change hints below, and disallow reentrant restyle hints in ServoRestyleManager::PostPendingRestyle.

Please replace it with a comment that says something like:

// Perform the Servo traversal, and the post-traversal if required.
Attachment #8827820 - Flags: review?(bobbyholley) → review+
Comment on attachment 8827821 [details]
Bug 1331294 - Part 6: Remove RestyleManager::HasPendingRestyles and Servo_Element_ShouldTraverse.

https://reviewboard.mozilla.org/r/105412/#review106430
Attachment #8827821 - Flags: review?(bobbyholley) → review+
Blocks: 1333183
Boris, I tried moving the mNeedStyleFlush and mNeedLayoutFlush flags from nsIDocument to PresShell, but I regressed the test case in bug 709256.  So I figured I'd just leave them where they are and add some methods to clear them.
Comment on attachment 8827817 [details]
Make Servo_TraverseSubtree return whether it did any work.

https://reviewboard.mozilla.org/r/105404/#review108500
Attachment #8827817 - Flags: review?(bobbyholley) → review+
> but I regressed the test case in bug 709256

In terms of performance, you mean?

In your moved version, did you end up checking the booleans out of line after the virtual call and all that?  Or inline in the document code or via an inline API?

I'm really not a fan of having setters that shouldn't be called from elsewhere; it seems pretty fragile to me...  So if we can do this the right way, I would really prefer that.
Yes, in terms of the number that test case reported.  (Not by much, but a little.  Something like from 76 to 80 on my local opt, non-PGO linux build.)

I started by doing all of the checks about the flush type and the booleans that currently happen at the end of nsIDocument::FlushPendingNotifications inside the virtual nsIPresShell::FlushPendingNotifications (and getting numbers > 100), and then by adding an inline NeedFlush(FlushType) method to nsIPresShell to pull the checks out of the virtual call (getting the ~80 number).  The disassembly of nsIDocument::FlushPendingNotifications looked reasonable too, although I didn't compare it to the disassembly without my patch.
Hmm.  I guess maybe we just ended up with more cache misses... :(

I think the 76->80 hit is probably OK in the grand scheme of things if it makes the code cleaner: it corresponds to an extra 4ns, is all, right?  But it's not clear to me whether it made the code cleaner, given the manual calls to the inlined functions?

I guess I can live with the setters in the interests of expediency, but please file a followup to really look into moving the flags to presshell?
Comment on attachment 8830622 [details]
Bug 1331294 - Part 7: Clear "need style/layout" flags on nsIDocument from PresShell::FlushPendingNotifications.

https://reviewboard.mozilla.org/r/107366/#review108974

r=me
Attachment #8830622 - Flags: review?(bzbarsky) → review+
A concrete example why moving the flags might be better.  What happens when a new presshell is created?  Right now, we end up calling SetNeedLayoutFlush from PresShell::Initialize, via the FrameNeedsReflow(rootFrame, nsIPresShell::eResize, NS_FRAME_IS_DIRTY) call.  And we end up calling SetNeedStyleFlush from... nowhere really.  I guess maybe it ends up not mattering in practice because we _just_ constructed frames using up-to-date styles and all the places that care about style flushes are OK with up-to-date frames... but it's not obvious to me that this is the case.  It would be cleaner to have the flags on presshell and both starting as true when a presshell is created.
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #46)
> I think the 76->80 hit is probably OK in the grand scheme of things if it
> makes the code cleaner: it corresponds to an extra 4ns, is all, right?  But
> it's not clear to me whether it made the code cleaner, given the manual
> calls to the inlined functions?
> 
> I guess I can live with the setters in the interests of expediency, but
> please file a followup to really look into moving the flags to presshell?

Thanks.  I think I deleted my patch that tried moving the flags, but I'll recreate it next week and you can tell me if you think it's clearer. :-)  (I'm not too sure myself.)  Filed bug 1334735.
No longer blocks: 1333183
Unfortunately, the part 7 changes that clear mNeedStyleFlush from inside PresShell::FlushPendingNotifications cause the same couple of test failures that the more comprehensive flag moving patch I posted in bug 1334735 do.
The part 7 patch won't be enough.
Depends on: 1334735
Attachment #8830622 - Attachment is obsolete: true
Pushed by cmccormack@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5355f85bbfdd
Part 1: Make nsPresContext::HasPendingRestyleOrReflow check nsIPresShell::mNeedStyleFlush instead of asking the RestyleManager for its status. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/cba5e59e8885
Part 2: Call nsIPresShell::SetNeedStyleFlush after propagating dirty descendants bits up the tree. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/457ed7c4de58
Part 3: Make ServoRestyleManager::PostRestyleEvent not check HasPendingRestyles() before returning early. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a306e4eda7e1
Part 4: Make ServoStyleSet::StyleDocument call Servo_TraverseSubtree unconditionally, and return whether a post-traversal is required. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce456f7140a5
Part 5: Make ServoRestyleManager::ProcessPendingRestyles use StyleDocument()'s return value to determine whether to return early. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/04685513b540
Part 6: Remove RestyleManager::HasPendingRestyles and Servo_Element_ShouldTraverse. r=bholley
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: