1334735 - move mNeedStyleRefresh and mNeedLayoutRefresh flags from nsIDocument to nsIPresShell

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Cameron McCormack (:heycam)]

From bug 1331294.

The flushing of style and layout is done by the pres shell, so it should make more sense to have the mNeedStyleRefresh and mNeedLayoutRefresh and flags on nsIPresShell rather than nsIDocument.

Comment 1

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Per bug 1331294 comment 49, I recreated the flag moving patch that I had.  I haven't re-tested it against the bug 709256 test case for performance, but I did to a try run and got a couple of failures:

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

You need to start the flags off as "true" in presshell.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1334735 - Part 1: Move need style/flush flags from document to pres shell.

Review commit: https://reviewboard.mozilla.org/r/110040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110040/

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Bug 1334735 - Part 2: Add separate flag to track need to flush throttled animations.

Review commit: https://reviewboard.mozilla.org/r/110042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110042/

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2)
> You need to start the flags off as "true" in presshell.

Thanks.  With these current patches I've got all test failures fixed except gfx/layers/apz/test/mochitest/test_interrupted_reflow.html.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hrm.  I'm not sure what's going on there.  The failure is on this line, right?

     is(elm.scrollTop, maxScroll, "Main-thread scroll position was restored");

If you compare the before/after cases, what do the flush booleans look like?

Comment 7

[Cameron McCormack (:heycam)]

That is the line.  The mNeedLayoutFlush boolean is false, with my patches applied, yet we still have the document's nsViewportFrame in mDirtyRoots (put there when reflow was interrupted).  The issue appears to be that we now clear mNeedLayoutFlush when we skip doing interruptible layout inside PresShell::FlushPendingNotifications when mSuppressInterruptibleReflows is true.  I think the condition that we check before calling SetNeedLayoutFlush() at the end of PresShell::FlushPendingNotifications is wrong.

Comment 8

[Cameron McCormack (:heycam)]

With the new patch queue, I'm getting approximately the same timing on the bug 709256 test case as without the patches, ~105 vs ~103.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Bug 1334735 - Part 3: Really set mNeedLayoutFlush when skipping suppressed interruptible reflows.

Review commit: https://reviewboard.mozilla.org/r/110294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110294/

Comment 10

[Cameron McCormack (:heycam)]

Comment on attachment 8833915 [details]
Bug 1334735 - Part 1: Move need style/flush flags from document to pres shell.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110040/diff/1-2/

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8833916 [details]
Bug 1334735 - Part 2: Add separate flag to track need to flush throttled animations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110042/diff/1-2/

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8833916 [details]
Bug 1334735 - Part 2: Add separate flag to track need to flush throttled animations.

https://reviewboard.mozilla.org/r/110042/#review111848

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8833915 [details]
Bug 1334735 - Part 1: Move need style/flush flags from document to pres shell.

https://reviewboard.mozilla.org/r/110040/#review112128

::: dom/base/nsDocument.cpp:257
(Diff revision 2)
>  #include "mozilla/StyleSheet.h"
>  #include "mozilla/StyleSheetInlines.h"
>  #include "mozilla/dom/SVGSVGElement.h"
>  #include "mozilla/dom/DocGroup.h"
>  #include "mozilla/dom/TabGroup.h"
> +#include "nsIPresShell.h"

You mean nsIPresShellInlines.h?  This file already includes mozilla/PresShell.h, which includes nsIPresShell.h....

::: dom/smil/nsSMILAnimationController.cpp:19
(Diff revision 2)
>  #include "mozilla/dom/SVGAnimationElement.h"
>  #include "nsSMILTimedElement.h"
>  #include <algorithm>
>  #include "mozilla/AutoRestore.h"
>  #include "RestyleTracker.h"
> +#include "nsIPresShell.h"

nsIPresShellInlines?

::: layout/base/nsCSSFrameConstructor.h:26
(Diff revision 2)
>  #include "nsQuoteList.h"
>  #include "nsCounterManager.h"
>  #include "nsIAnonymousContentCreator.h"
>  #include "nsFrameManager.h"
>  #include "ScrollbarStyles.h"
> +#include "nsIPresShellInlines.h"

Why is this needed, if you took QuotesDirty/CountersDirty out of line?

::: layout/base/nsCSSFrameConstructor.cpp:12929
(Diff revision 2)
> +void
> +nsCSSFrameConstructor::QuotesDirty()
> +{
> +  NS_PRECONDITION(mUpdateCount != 0, "Instant quote updates are bad news");
> +  mQuotesDirty = true;
> +  if (nsIPresShell* shell = mDocument->GetShell()) {

Why not just use mPresShell?  I _think_ it shouldn't even ever end up null if this is getting called.  But could null-check just in case, I guess.

::: layout/base/nsCSSFrameConstructor.cpp:12939
(Diff revision 2)
> +void
> +nsCSSFrameConstructor::CountersDirty()
> +{
> +  NS_PRECONDITION(mUpdateCount != 0, "Instant counter updates are bad news");
> +  mCountersDirty = true;
> +  if (nsIPresShell* shell = mDocument->GetShell()) {

Again, mPresShell.

::: layout/base/nsIPresShell.h:602
(Diff revision 2)
> +    // We check mInFlush to handle re-entrant calls to FlushPendingNotifications
> +    // by reporting that we always need a flush in that case.  Otherwise,
> +    // we could end up missing needed flushes, since we clear mNeedStyleFlush
> +    // and mNeedLayoutFlush at the top of FlushPendingNotifications.
> +    //
> +    // XXXheycam Could we just clear those flags after the work has been

No, because doing the work can trigger things (e.g. script execution!) that schedule new restyles or layouts, so we can't assume the flags are false after the work is done.

::: layout/base/nsIPresShellInlines.h:10
(Diff revision 2)
> +
> +void
> +nsIPresShell::SetNeedLayoutFlush()
> +{
> +  mNeedLayoutFlush = true;
> +  if (nsIDocument* doc = mDocument->GetDisplayDocument()) {

So we actually have a subtle problem here.

Say we have a situation where a document (call it D) has no presshell but D->GetDisplayDocument() does or vice versa.  Then trying to schedule a flush on D's presshell would no-op (because there isn't one) and we would not schedule a flush on D->GetDisplayDocument()'s presshell either, because we would never reach this code.  Then if D got a presshell it would have the flush flags set to true, but D->GetDisplayDocument()'s presshell would have them set to false, and since all flushes _start_ at D->GetDisplayDocument(), we would not flush D as needed.

This mostly doesn't come up because the presshell lifetimes for D and D->GetDisplayDocument() are synced.  But there is one failing case: when D as a whole is created after D->GetDisplayDocument()->GetShell() already exists.  In that case, we end up with precisely the failure mode I descibed above: D's presshell has "true" for the flush flags, but D->GetDisplayDocument()->GetShell() knows nothing about this, so doesn't know it needs to do any flushing.

I think we should solve this by having the nsIPresShell ctor call SetNeedLayoutFlush() and SetNeedStyleFlush(), with a nice comment explaining why, instead of just setting the members to true.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8833916 [details]
Bug 1334735 - Part 2: Add separate flag to track need to flush throttled animations.

https://reviewboard.mozilla.org/r/110042/#review112142

::: layout/base/nsIPresShell.h:593
(Diff revision 2)
>    /**
>     * Whether we might need a flush for the given flush type.  If this
>     * function returns false, we definitely don't need to flush.
>     *
>     * @param aFlushType The flush type to check.  This must be
> -   *   >= FlushType::Style.
> +   *   >= FlushType::Style.  We assume that throttled animations are to be

I don't follow this comment.  Do you mean that we assume that all flush types >= Style flush throttled animations?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8834327 [details]
Bug 1334735 - Part 3: Really set mNeedLayoutFlush when skipping suppressed interruptible reflows.

https://reviewboard.mozilla.org/r/110294/#review112152

::: layout/base/PresShell.cpp:4251
(Diff revision 1)
>      if (aFlush.mFlushAnimations) {
>        SetNeedThrottledAnimationFlush();
>      }
>    }
>  
> -  if (!didLayoutFlush && !mIsDestroying &&
> +  if (!didLayoutFlush && flushType >= FlushType::InterruptibleLayout &&

Hah.  I _thought_ this code looked unfamiliar.  This looks like a regression that was introduced in bug 1029982.

Before that, the logic looked like this, effectively:

    if (!mIsDestroying &&
        mSuppressInterruptibleReflows && 
        flushType == Flush_InterruptibleLayout) {
      mDocument->SetNeedLayoutFlush()
    }
    
but the new condition clearly lost the correct behavior for the mSuppressInterruptibleReflows && flushType == Flush_InterruptibleLayout case.  Too bad we apparently don't have very good test coverage.  :(

On the other hand, I'm not sure how one would have triggered this situation before; one would need a flush of InterruptibleLayout on the _document_, which I'm not sure anything does.

In any case, good catch!

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> This mostly doesn't come up because the presshell lifetimes for D and
> D->GetDisplayDocument() are synced.  But there is one failing case: when D
> as a whole is created after D->GetDisplayDocument()->GetShell() already
> exists.  In that case, we end up with precisely the failure mode I descibed
> above: D's presshell has "true" for the flush flags, but
> D->GetDisplayDocument()->GetShell() knows nothing about this, so doesn't
> know it needs to do any flushing.

D being created after D->GetDisplayDocument()->GetShell() is what would usually happen, I think, since the display document would already need to be being displayed for any SVG resource document loads to be kicked off.

In practice I don't know if the display document not knowing about the pending restyle in the resource document matters, since we should get to nsReferencedElement::DocumentLoadNotification::Observe once the nsExternalResourceMap determines that the resource document is ready, which should then post any restyles according to the specific type of reference it was (e.g. filter, clip-path, or whatever).  But I agree we should keep them in sync, since that's easier to reason about.

> I think we should solve this by having the nsIPresShell ctor call
> SetNeedLayoutFlush() and SetNeedStyleFlush(), with a nice comment explaining
> why, instead of just setting the members to true.

mDocument isn't set in the nsIPresShell constructor.  Maybe we should be doing this in (or just after) nsIDocument::SetDisplayDocument instead.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> mDocument isn't set in the nsIPresShell constructor.

Then we should make the calls at the point when mDocument gets set, I think.  That's pretty guaranteed to happen before the presshell does anything... ;)

Comment 18

[Cameron McCormack (:heycam)]

Yeah, looks like SetDisplayDocument is the wrong place; the pres shell isn't set there.  PresShell::Init should be the right place then.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c76949a95eeb
Part 1: Move need style/flush flags from document to pres shell. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a27478b613d
Part 2: Add separate flag to track need to flush throttled animations. r=bz,birtles
https://hg.mozilla.org/integration/mozilla-inbound/rev/0195a0a47d89
Part 3: Really set mNeedLayoutFlush when skipping suppressed interruptible reflows. r=bz

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c76949a95eeb
https://hg.mozilla.org/mozilla-central/rev/4a27478b613d
https://hg.mozilla.org/mozilla-central/rev/0195a0a47d89