Closed Bug 575336 Opened 10 years ago Closed 9 years ago

Style flushes can auto-convert to layout flushes on same document

Categories

(Core :: Layout, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b4

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(6 files, 2 obsolete files)

Consider a situation in which document A is a subframe of document B and we do a style flush on A.  The code added to nsDocument::FlushPendingNotifications to handle media queries will convert this to a layout flush on document B.  This will trigger a layout flush of B's presshell.  If that triggers any invalidates, the viewmanager will be told to end the update view batch as NS_VMREFRESH_NO_SYNC (instead of the NS_VMREFRESH_DEFERRED we use for non-layout flushes) and this will proceed to immediately call WillPaint and flush layout on all presshells in the viewmanager tree, including the presshell for document A.

Possible solutions:

1)  Don't promote style flushes to layout ones unless we know we have media
    queries (the matcher could flag this on the style context).
2)  Always use NS_VMREFRESH_DEFERRED for the update batch in question.  Have to
    make sure we don't get weird frame skips.  
3)  Both of the above.

roc, thoughts?

This is costing us about 20% on at least one of the Zimbra preference tab tests, fwiw.
Does nsViewManager::FlushPendingInvalidates need to call WillPaintOnObservers? We added that in bug 244366. But is it necessary? The WillPaint in NS_PAINT handling will perform flushes and let us coalesce any painting resulting from those flushes.
> But is it necessary?

An excellent question....

Reading over bug 244366, this was an attempt to reduce the number of platform invalidate things we ended up doing... We should try taking this out and seeing whether anything breaks.  Nothing jumps out at me.

Do we want to just switch presshell to always doing a NO_SYNC update batch then, even in cases when reflows aren't being flushed?
It looks like we were trying to coalesce the ProcessPendingUpdates calls.  Maybe we don't need to worry about that, though.

Can we call ProcessPendingUpdates off the refresh driver?  That's probably just fodder for a separate bug...
I filed bug 576159 on restricting the cases in which we promote to a layout flush (#1 from comment 0).
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #455328 - Flags: review?(roc)
We should just take views out of the picture by making nsFrame::InvalidateRoot always store to a dirty region belonging to the display root frame (topmost nsViewportFrame or popup frame), and painting that periodically and synchornously off the refresh driver. There's some trickiness since refresh driver is per-doc and we need to paint cross doc. No more DEFERRED/NO_SYNC, no more FlushPendingInvalidates.
> There's some trickiness since refresh driver is per-doc and we need to paint
> cross doc.

Why is this tricky?  We'd only drive painting off the root refresh driver, right?  Or do we need to paint once per document?

Want to file a bug on doing that if we don't have one?
We should probably have fewer refresh drivers (though we probably want to keep the extra at the frame boundaries that are going to become process boundaries).
(In reply to comment #8)
> > There's some trickiness since refresh driver is per-doc and we need to paint
> > cross doc.
> 
> Why is this tricky?  We'd only drive painting off the root refresh driver,
> right?

I guess that would work.
I tried landing the above patch, and it caused test failures; I backed it out.  Specific failing tests:

M3 (debug+opt):
7462 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/base/tests/test_selection_move_commands.xul | cmd_scrollBottom - 0 should equal -300

M4 (debug+opt):
5871 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (width: 157px) should apply
5874 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (height: 182px) should apply
5878 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (width) should not apply
5879 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (height) should not apply
5880 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (width) should not apply
5883 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_bug453896_deck.html | all and (height) should not apply

Moth (debug+opt):
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (width: 157px) should apply
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (height: 182px) should apply
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (width) should not apply
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (height) should not apply
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (width) should not apply
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/layout/style/test/browser_bug453896.js | all and (height) should not apply

Reftest (debug+opt, intermittent):
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-debug-unittest-reftest/build/reftest/tests/layout/reftests/bugs/482659-1c.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/mozilla-central-win32-opt-unittest-reftest/build/reftest/tests/layout/reftests/bugs/482659-1d.html |
I _think_ what's going on here at least for the media query tests is that some of those run before paint suppression stops.  So resizing the iframe in the parent doesn't necessarily resize the presshell of the child (due to nsViewManager::SetWindowDimensions sticking the new size into mDelayedResize instead).  So the layout flush we do on the parent doesn't actually trigger a ResizeReflow on the kid, and then we don't end up updating media queries in the kid, and lose.  It used to be the reflow flush on the parent flushed reflow on the kid, so we'd do the FlushDelayedResize there, before unwinding to the child style flush, and then the child would have the right styles.

Making the FlushDelayedResize call happen for style-only flushes right before FlushPendingMediaFeatureValuesChanged fixes the media-query tests and the two reftest failures.

As for the editor test failure....  That test fails for me no matter what if I load it standalone.  If I load it in an iframe, it passes without this patch, fails with.  The change to FlushDelayedResize on its own doesn't make that test pass...

roc, any idea what's up there?
I have to note, I'm not sure why that delayed resize change fixes the reftests in question....
I think the problem with test_selection_move_commands.xul is that nothing flushes layout along that editor command path!

I suspect the DoCommand methods in nsEditorCommands should all be flushing...
Priority: -- → P1
Attachment #455328 - Attachment description: Ok, so let's try this → Part 4: Ok, so let's try this
This fixes the media query tests.  The idea is that on style flush we want to make sure our prescontext's size is up to date.  I considered just flushing the pending resize, period, but that would do the resize reflow... which I'm not sure we want to do here.  I could switch to doing that if you prefer, though.
Attachment #463075 - Flags: review?(roc)
Attached patch Part 2: editor needs to flush (obsolete) — Splinter Review
This fixes the editor mochitest.  I didn't flush up front because I'm not sure we want to do it in general for typing and arrowing.... so I tried to limit it to things that clearly need layout information.  Let me know what you think.
Attachment #463076 - Flags: review?(roc)
This fixes the remaining reftest failures, I think.  The reftest harness does some flushing, but was only flushing the root window, not subframes.  This changes it to also flush subframes.
Attachment #463077 - Flags: review?(roc)
Summary: Style flushes can auto-convert to layout flushes on same document → [FIX]Style flushes can auto-convert to layout flushes on same document
I don't see any reason to treat movement commands differently. Surely they should either all flush, or none?
Summary: [FIX]Style flushes can auto-convert to layout flushes on same document → Style flushes can auto-convert to layout flushes on same document
Whiteboard: [need review]
Well, the movement commands don't obviously depend on up-to-date layout (unlike things that need to make sure that the scrollport is the right size or that we have the right scrolled frame size).

Ehsan, do we use this stuff internally, or would it be safe to just flush for all of these?
I think scrolling by line at least should need flushing, since the scrollport has to be the right size
OK, I think I buy that...
Attachment #463076 - Attachment is obsolete: true
Attachment #463092 - Flags: review?(roc)
Attachment #463092 - Flags: review?(ehsan)
Attachment #463076 - Flags: review?(roc)
Comment on attachment 463092 [details] [diff] [review]
Part 2: now with more flushing

    // Most of the commands below (possibly all of them) need layoutto

fix typo
Attachment #463092 - Flags: review?(roc) → review+
Comment on attachment 463092 [details] [diff] [review]
Part 2: now with more flushing

All of these commands that I've ever looked at should need an up to date layout, since we use the frames for all caret movement computations.  I'm fairly positive that I've looked at all of these commands, so I think this is the right thing to do here.
Attachment #463092 - Flags: review?(ehsan) → review+
> fix typo

Fixed.
This test used to pass... but it's clearly not right now; this patch fixes.
Attachment #463183 - Flags: review?(roc)
Comment on attachment 463184 [details] [diff] [review]
Roll-up patch for approval only; DO NOT CHECK IN

a2.0=dbaron, but, boy, this patch sure doesn't look like the rollup patch for this bug
Attachment #463184 - Flags: approval2.0? → approval2.0+
Oops.  This should be better.  ;)
Attachment #463184 - Attachment is obsolete: true
Attachment #463210 - Flags: approval2.0+
Whiteboard: [need review] → [need landing]
Depends on: 617076
Depends on: 715261
You need to log in before you can comment on or make changes to this bug.