377119 - [FIX]printing reftest only shows first page on Mac

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

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

Bug 370439 caused a Mac-only regression with the printing reftest (layout/reftests/printing/272830-1.html).  It only shows the first page when showing the test, but it shows both pages for the reference.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Here's roughly what's happening:

1) During the initial reflow, gfxScrollFrameInner posts a reflow callback to update the scrollbar attributes.
2) After we've finished processing the reflow callback, presshell flushes any new layout notifications.
3) There is a pending restyle of a scrollbar button which didn't have frames but now will. I believe this is because the scrollbar XBL constructor has run which calls initScrollbars
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/scrollbar.xml#40
which changes the visible buttons from the standard Windows layout to the Mac layout. I'm not sure when the XBL constructor ran or why this restyle wasn't flushed before the initial reflow (Boris?).
4) The scrollbar's nsBoxFrame::InsertFrames asks for the scrollbar frame to be reflowed again:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsBoxFrame.cpp#1038
5) The scrollbar is a reflow root, but its parent isn't, so we have to reflow the document itself to handle this.
http://lxr.mozilla.org/seamonkey/source/layout/base/nsPresShell.cpp#3409
6) We reflow the document again. But tables don't know how to do incremental reflow in the context of pagination, so the table continuation on the second page is basically just deleted. Oops.

I think steps 4+5 are wrong --- we shouldn't need to reflow the whole document given that the scrollbar is a reflow root and we're just messing with its children. But I'm not sure which of step 4 or 5 is wrong in this case. David?

I think step 3 is also a problem. Constructing a frame for the up-top scrollbar button and then later deleting the frame and constructing a new frame for the bottom-up button is stupid extra overhead on Mac. I guess we don't want to run the XBL constructor before we construct frames for the default children because that would mean running script during frame construction. Is there a way to have the initial hidden attributes be set differently for Mac, short of cloning scrollbar.xml? If not, maybe we should just do that.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could probably also fix this bug by ensuring that after frame construction but before reflow, all XBL constructors have run and all their pending restyles have been flushed.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could also fix this by fixing table incremental reflow in paginated contexts, of course, but that's not something I want anyone to do for 1.9.

Comment 4

[Colin Barrett [:cbarrett]]

Wouldn't this test also fail on Linux in a case like bug 263444? The commented out throw in scrollbar.xml is a bit misleading.

Comment 5

[Eli Friedman]

(In reply to comment #2)
> We could probably also fix this bug by ensuring that after frame construction
> but before reflow, all XBL constructors have run and all their pending restyles
> have been flushed.

So, adding something like 

mDocument->BeginUpdate(UPDATE_ALL);
mDocument->EndUpdate(UPDATE_ALL);

to nsPresShell::InitialReflow()?  That could work.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsGfxScrollFrame.cpp&rev=3.301&mark=434-435#434 might have something to do with this problem; could someone try removing that bit of code and seeing if it fixes the problem?  (It's a shot in the dark, but it could work... the justification is that the page sequence returns early from reflow if it isn't dirty.)

(I can't do any coding at the moment because my hard drive failed.  I should have that resolved by this weekend.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I'm not sure when the XBL constructor ran or why this restyle wasn't flushed
> before the initial reflow (Boris?).

See http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsPresShell.cpp&rev=3.984&mark=2627,2628#2627

It do think it might make sense to move that to right after we construct the frame tree, so make the logic in InitialReflow something like:

1)  Construct the frame tree
2)  Process XBL constructors
4)  Make sure we're not dead
5)  Flush pending restyles
6)  Probably make sure we're not dead again, since step 5 can run XBL ctors too
7)  Do the actual reflow

sicking, what do you think?

> 5) The scrollbar is a reflow root, but its parent isn't

I'd think that for a eTreeChange targeted at a reflow root we could just reflow the reflow root, since eTreeChange means something _under_ the root changed, right?  dbaron would know for sure.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: The frame constructor changes per comment 6

Comment 8

[Boris Zbarsky [:bzbarsky]]

Filed bug 378784 on steps 4 and 5 from comment 1.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 261285 [details] [diff] [review]
The frame constructor changes per comment 6

r/sr=me. Smaugs patch is going to fix the shell iteration, right?

Comment 10

[Eli Friedman]

Comment on attachment 261285 [details] [diff] [review]
The frame constructor changes per comment 6

>+    // Now reget the root frame, since all that script might have
>+    // affected it somehow.

That's impossible; the viewport frame gets created once, in InitialReflow, and doesn't get destroyed until the presshell get destroyed.

>+  // XXXbz do we need the extra grip and the NS_ENSURE_STATE?  Or is
>+  // aPO->mPresShell guaranteed to not go away?

aPO->mPresShell is guaranteed not to go away; all the print objects are owned by the print engine, and the print engine can't get destroyed while the presentation is being created.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> That's impossible; the viewport frame gets created once,

I'd really rather not start relying on that....  I'll document that more clearly.

> aPO->mPresShell is guaranteed not to go away

Great!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: With better comments (and most of the print engine change removed)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 14

[Peter van der Woude [:Peter6]]

This seems to have  broken the Tab Sidebar extension (sidebar remains empty) on Windows (don't know about Mac or Linux).

Comment 15

[Boris Zbarsky [:bzbarsky]]

Seems to work for me on Linux, just fine.  I would expect it to work everywhere, unless the extension is doing something _really_ screwy.

Peter, do you build?  If so, can you do some experiments for me?

Comment 16

[Peter van der Woude [:Peter6]]

(In reply to comment #15)
> Peter, do you build?  If so, can you do some experiments for me?
No I don't build, unfortunately. 

Comment 17

[Boris Zbarsky [:bzbarsky]]

Do you need any particular settings for the extension?  Or does it show up with the vanilla install settings?

Comment 18

[Peter van der Woude [:Peter6]]

It does show up with vanilla settings Boris.
Dave Townsend (Mossop) is going to have a closer look at it later, if he can find the time. 

Comment 19

[Peter van der Woude [:Peter6]]

The issue with Tab Sidebar was an extension problem which is fixed now.