1443483 - The FlushTarget stuff doesn't seem to make sense.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Emilio Cobos Álvarez (:emilio)]

The only point of flushing the parent document is that media query updates could affect this document. If we end up not flushing the document, then all that is useless work.

Comment 1

[Phabricator Automation]

Attachment: Bug 1443483: FlushTarget doesn't really make sense. r=xidorn

Comment 2

[Boris Zbarsky [:bzbarsky]]

> The only point of flushing the parent document is that media query updates could affect this document. 

In general, for a layout flush, that's not true: resizes of our viewport could just change layout even if there are no media queries.

That said, flushing the parent but then _not_ flushing ourselves doesn't seem like it would pick this part up either.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> > The only point of flushing the parent document is that media query updates could affect this document. 
> 
> In general, for a layout flush, that's not true: resizes of our viewport
> could just change layout even if there are no media queries.

Well, sure, this is only used from nsComputedDOMStyle though.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, but nsComputedDOMStyle does look at layout bits (for the properties that it returns used values for).

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Yes, but nsComputedDOMStyle does look at layout bits (for the properties
> that it returns used values for).

Ok, fair. Though we flush unconditionally for those as of right now :)

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

Consider this testcase. I suppose this would be broken with the patch here.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Actually, it is wrong in Gecko currently :/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #7)
> Actually, it is wrong in Gecko currently :/

Yeah, the patch here shouldn't make us more wrong in any way. Your test isn't caught by any of our current checks (nice test-case btw!)

That's why I'd like to get rid of the FlushTarget stuff, and fix this in a followup bug tomorrow if you don't mind.

Comment 9

[Phabricator Automation]

Comment on attachment 8956433 [details]
Bug 1443483: FlushTarget doesn't really make sense. r=xidorn

Xidorn Quan [:xidorn] UTC+10 has approved the revision.

https://phabricator.services.mozilla.com/D682

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/610e3fc18116
FlushTarget doesn't really make sense. r=xidorn

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e180161959e4
followup - Fix lint warning about virtual function keywords.

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/610e3fc18116
https://hg.mozilla.org/mozilla-central/rev/e180161959e4

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm a little concerned about the overall trend here (although not necessarily this patch in particular):  it seems like nsComputedDOMStyle::DocumentNeedsRestyle has written essentially its own version of FlushPendingNotifications that doesn't check all the things that FlushPendingNotifications does.  Are we sure that none of the other things in FlushPendingNotifications can affect computed style?  (For example, flushing the content sink?   Flushing the user font set?  SMIL resample requests?  XBL constructors?)  I wouldn't necessarily expect all of these to be well-tested in our test suite; we might have added tests for the flushing but those tests might not have used computed style flushing.  It also seems like a bit of a risk for future maintenance as well.  Is DocumentNeedsRestyle really good enough?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

So, DocumentNeedsRestyle is clearly not good enough, given bug 1443676. If only, we always need to do a full layout flush on the parent document, if there are viewport-dependent media queries at least.

Now, regarding to whether avoiding the style flush in the document we're calling getComputedStyle on is worth the complexity... I think it might be. Per bug 1358495 this is probably a real problem. That being said... I agree this is a fair amount of complexity. Probably :wpan is the only person that would have measurements regarding how often this is hit or not :/.

If we measure it and it turns out indicating that the optimization isn't really worth it, or isn't hit enough for it to be worth it, I'd be fine for removing it. I'll file a bug for that.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #13)
> Are we sure that none of the other things
> in FlushPendingNotifications can affect computed style?  (For example,
> flushing the content sink?   Flushing the user font set?  SMIL resample
> requests?  XBL constructors?)

I don't think flushing the content sink should have any effect here, though I'm not totally familiar with all the implications here.

I suspect running script should at least flush the content sink, right? Otherwise, I guess we'd need something like a content sink flush in all the DOM-exposed methods, like querySelector, etc.

Re., SMIL resample requests, seems like they can reenter into FlushPendingNotifications[1], which looks somewhat scary, but in any case should be caught by the existing animations check (if any ancestor has any animation running we do a full flush), so I think we're good.

Re. the user font set, I sort of agree[2]. It can affect ex/ch units if a font has finished loading already but we still haven't flushed it, but I think it's not observable (the font could as well not have loaded, and we trigger the load events from the flush, so it can't be observed I think).

If XBL constructors can run script, which they can, this is broken in that case. But that doesn't worry me a lot, I think, given it's not exposed to web content.

[1]: https://searchfox.org/mozilla-central/rev/40b1baaab5aba21223590ae1029e916dfd5ff7cd/dom/smil/nsSMILAnimationController.cpp#440
[2]: https://searchfox.org/mozilla-central/rev/40b1baaab5aba21223590ae1029e916dfd5ff7cd/layout/style/nsComputedDOMStyle.cpp#165

Comment 15

[Boris Zbarsky [:bzbarsky]]

> For example, flushing the content sink?

This can init the presshell if we're flushing at FlushType::Style or above.  Apart from that, it does nothing for HTML.  For XML, it can trigger sink notifications, but I don't think those can affect computed style of other elements.  The element whose style is being queried _could_ be affected to the extent that we get different behavior for the "with frame" and "without frame" cases.

> I suspect running script should at least flush the content sink, right?

Running a <script> does.  Running random JS e.g. off a timer does not.  Sink flush (in XML) would not affect querySelector, because that doesn't depend on sink notifications.

I agree that in general DocumentNeedsRestyle is pretty fishy...