Closed
Bug 1334735
Opened 8 years ago
Closed 8 years ago
move mNeedStyleRefresh and mNeedLayoutRefresh flags from nsIDocument to nsIPresShell
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(3 files, 1 obsolete file)
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.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
You need to start the flags off as "true" in presshell.
Assignee | ||
Updated•8 years ago
|
Attachment #8833603 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•8 years ago
|
||
(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•8 years ago
|
||
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?
Assignee | ||
Comment 7•8 years ago
|
||
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.
Assignee | ||
Comment 8•8 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
mozreview-review |
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
Attachment #8833916 -
Flags: review?(bbirtles) → review+
![]() |
||
Comment 13•8 years ago
|
||
mozreview-review |
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.
Attachment #8833915 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 14•8 years ago
|
||
mozreview-review |
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?
Attachment #8833916 -
Flags: review?(bzbarsky) → review+
![]() |
||
Comment 15•8 years ago
|
||
mozreview-review |
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!
Attachment #8834327 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 16•8 years ago
|
||
(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•8 years ago
|
||
> 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... ;)
Assignee | ||
Comment 18•8 years ago
|
||
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•8 years ago
|
||
Pushed by cmccormack@mozilla.com: 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c76949a95eeb https://hg.mozilla.org/mozilla-central/rev/4a27478b613d https://hg.mozilla.org/mozilla-central/rev/0195a0a47d89
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•8 years ago
|
Assignee: nobody → cam
You need to log in
before you can comment on or make changes to this bug.
Description
•