562955 - opt test mochitests-4/5: /tests/layout/base/tests/test_bug458898.html | innerHeight: 0 >= 200 ?

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[alice nodelman [:alice] [:anode]]

From unit tests run on talos win7 machines.

562 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug458898.html | innerHeight: 0 >= 200 ?

Comment 1

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

That sounds bad.  Is the window actually opening as the wrong size?

Comment 2

[Rob Arnold [:robarnold]]

I wonder if this is related to bug 558479 that I've been seeing for some time on trunk/win7 now. Can you get a screenshot of the window/desktop when this happens?

Comment 3

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

Does this test fail every time on Win7, or just some of the time?

Comment 4

[alice nodelman [:alice] [:anode]]

This is consistently orange on windows 7 for unit test on talos currently being staged.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

dbaron: can you take a look?

Comment 6

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

I think this is most likely a windows widget code bug, and that jimm or robarnold would be the best people to look into it.

Comment 7

[David Mandelin [:dmandelin]]

I get this reproducibly on Windows 7 in a JM build. docShellWin->GetSize() returns (117, 0), so it seems this is the same bug.

Comment 9

[[e-mail] (:bhearsum)]

This permanently fails on 64-bit win7, too. Can someone take a look at this, we don't have other 64-bit test platforms on Windows.

Comment 10

[Armen [:armenzg]]

We are now running the Windows 7 unit tests on production but hidden.

This is one of the 4 test suites left to greenize on Windows 7 and we want to
make this green as soon as it is possible to help us move load from the Win2k3
machines to the Win7 machines (it will really help us). Any help that you can
give us will be really appreciate it since this block us to roll this goal out.

You can find logs for it under:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&noignore=1
as:
* Rev3 WINNT 6.1 mozilla-central opt test mochitests-4/5

Comment 11

[Armen [:armenzg]]

This bug is still valid and there is no assigned owner.
This blocks us from switching from running unit tests on the builders to the minis and therefore improve wait times.

Comment 12

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

You can also find the current logs by going to:
http://tests.themasta.com/tinderboxpushlog/?noignore=1
and looking for the "Mo (4)" build on the Win7 line.

Comment 13

[Armen [:armenzg]]

Hi Jim, what is the status update in here?
Anyone else that could help you with this?

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> Hi Jim, what is the status update in here?
> Anyone else that could help you with this?

Been busy and haven't had a chance to look at this. I'm headed into pto, I'll be back 8/18. I might be able to get to this after that, but I can't promise anything.

Comment 15

[Robert Sayre]

I disabled this test.

http://hg.mozilla.org/tracemonkey/rev/ad7f95eb5fd5

This will get merged to m-c soon. Tracemonkey shows the few remaining Windows 7 failures, so I'm landing them there to make sure I've correctly disabled the tests.

Comment 16

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #15)
> I disabled this test.
> 
> http://hg.mozilla.org/tracemonkey/rev/ad7f95eb5fd5
> 
> This will get merged to m-c soon. Tracemonkey shows the few remaining Windows 7
> failures, so I'm landing them there to make sure I've correctly disabled the
> tests.

Ping - any ETA? (Sorry for the pester, but we're eager to disable those tests-on-builders, and improve wait times.)

Comment 17

[Phil Ringnalda (:philor)]

Merged, and Win7 Mo4 unhidden on mozilla-central and e10s.

Comment 18

[Jim Mathies [:jimm]]

Hmm, sizeToContent appears broken. We're not getting the right size values. This appears broken on all windows platforms, not just win7.

Comment 19

[Jim Mathies [:jimm]]

Attachment: stand alone test

Comment 20

[Jim Mathies [:jimm]]

Attachment: fix

waiting on a try run to confirm this fix.

Comment 21

[Jim Mathies [:jimm]]

So this bug is triggered by dumb luck. In nsHTMLReflowState's InitResizeFlags, we set a horizontal reflow flag based on a set of checks:

362 mFlags.mHResize = !(frame->GetStateBits() & NS_FRAME_IS_DIRTY) &&
363 frame->GetSize().width !=
364 mComputedWidth + mComputedBorderPadding.LeftRight(); 

Coincidentally the default window width on Win7 (132 pixels) results in an inner frame width of 100px, which is the same width this test sets. So mFlags.mHResize ends up being false.

http://hg.mozilla.org/mozilla-central/annotate/f5603541696d/layout/generic/nsHTMLReflowState.cpp#l360

In ViewportFrame's Reflow, we use the result of this via ShouldReflowAllKids() to decide if we should reflow. ShouldReflowAllKids returns false, so the visible area of the window never gets updated, and that in turn causes SizeToContent to resize the window to the default (which it already is).

This bug can be triggered on XP, you just have to adjust the width in the test down 1px (99px in the test). I suppose you could do the same on other platforms. This is also reproducible in 3.6.

I'm unsure of how to fix this. The width check appears to be unreliable. It also seems odd that NS_FRAME_IS_DIRTY isn't set on the frame. I'm still trying to sort that out.

(Moving this over to layout since it's not a widget bug.)

Comment 22

[Boris Zbarsky [:bzbarsky]]

> It also seems odd that NS_FRAME_IS_DIRTY isn't set on the frame.

Indeed.  For the first reflow it should be set... and for later ones we should have already reflowed at GetSize().width and hence not need any updating if the width hasn't changed.

And in fact, you're saying that frame->GetSize().width is nonzero, right?  So we have already been laid out at this width once.  What's the issue?

Or maybe more precisely.... how is the stuff that you think should be reflowed but isn't related to |frame| in the above code?  Is |frame| above a viewport frame?

Comment 23

[Jim Mathies [:jimm]]

(In reply to comment #22)
> > It also seems odd that NS_FRAME_IS_DIRTY isn't set on the frame.
> 
> Indeed.  For the first reflow it should be set... and for later ones we should
> have already reflowed at GetSize().width and hence not need any updating if the
> width hasn't changed.
> 
> And in fact, you're saying that frame->GetSize().width is nonzero, right?  So
> we have already been laid out at this width once.  What's the issue?
> 
> Or maybe more precisely.... how is the stuff that you think should be reflowed
> but isn't related to |frame| in the above code?  Is |frame| above a viewport
> frame?


The value for frame->GetSize().width apparently propagates up from the widget, it's the default width of the window. mFlags.mHResize ends up being false, which causes mFlags.mVResize to be false, and this prevents us from calculating the proper height of the content. That in turn messes up SizeToContent which pulls dims from pres context's GetVisibleArea(). 

I got side tracked today but will be looking at this more tomorrow.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> The value for frame->GetSize().width apparently propagates up from the widget,
> it's the default width of the window.

Sure, but the only way it can "propagate" is via reflow, right?  That's where we set frame sizes.  Or is someone doing a guerilla SetSize() or SetRect() on a frame?

So is the issue here that we reflow the document once at the default size, then try to SizeToContent?  We should be guaranteeing that SizeToContent either sets both resize flags on the root or more simply just does a dirty reflow, imo.  But why do we have a reflow before the SizeToContent one?  Is that a bug or just life?

Comment 25

[Jim Mathies [:jimm]]

(In reply to comment #24)
> > The value for frame->GetSize().width apparently propagates up from the widget,
> > it's the default width of the window.
> 
> Sure, but the only way it can "propagate" is via reflow, right?  That's where
> we set frame sizes.  Or is someone doing a guerilla SetSize() or SetRect() on a
> frame?
> 
> So is the issue here that we reflow the document once at the default size, then
> try to SizeToContent?

No doesn't look like it. This seems to be happening on the first reflow which is triggered by nsDocumentViewer's SizeToContent calling ResizeReflow.

In DoReflow, we setup reflowState, which gets passed to target->Reflow. That in turn returns a desiredSize. If mComputedWidth matches up to the frame width (which is already set to the default window width), desiredSize.height is set to zero. At the end of DoReflow, we set the frame height to the desired height (0). Then in the tail end of ResizeReflow, we set the visible area height to the root frame height (also 0).

> We should be guaranteeing that SizeToContent either sets
> both resize flags on the root or more simply just does a dirty reflow, imo.

Getting the reflowState flags set right from SizeToContent looks tricky. These two are abstracted from each other by a number of generic calls. However a call in nsDocumentViewer's SizeToContent to preshell->StyleChangeReflow() prior to the presshell->ResizeReflow() fixes this.

Would that be an acceptable solution? (I don't want to totally bork something else or kill performance somehow.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> (which is already set to the default window width)

Why did that happen?  Who set it?

I'd need to think a bit before I can answer the questions at the end there.

Comment 27

[Jim Mathies [:jimm]]

(In reply to comment #26)
> > (which is already set to the default window width)
> 
> Why did that happen?  Who set it?
> 
> I'd need to think a bit before I can answer the questions at the end there.

It gets passed up when the window is created:

> xul.dll!nsDocShell::SetPositionAndSize
xul.dll!nsDocShell::InitWindow
xul.dll!nsWebShellWindow::Initialize
xul.dll!nsAppShellService::JustCreateTopWindow
xul.dll!nsAppShellService::CreateTopLevelWindow

That sets mBounds, which is then used in DocumentViewerImpl's InitPresentationStuff to set the initial visible area on the pres context.

Comment 28

[Jim Mathies [:jimm]]

Attachment: patch

This targets the specific case we're interested in without effecting any other situation.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> That sets mBounds, which is then used in DocumentViewerImpl's
> InitPresentationStuff to set the initial visible area on the pres context.

Yes, but what sets that size on the _frame_?

Comment 30

[Jim Mathies [:jimm]]

Attachment: stacks and code

(In reply to comment #29)
> > That sets mBounds, which is then used in DocumentViewerImpl's
> > InitPresentationStuff to set the initial visible area on the pres context.
> 
> Yes, but what sets that size on the _frame_?

Ok, this value is initialized in PresShell::DoReflow on the initial refresh timer.

Attached stacks show how it gets sets.

Comment 31

[Jim Mathies [:jimm]]

Attachment: stacks and code

with the right second stack

Comment 32

[Boris Zbarsky [:bzbarsky]]

OK, so a reflow before the SizeToContent attempt, yes.  I believe that!

Comment 33

[Boris Zbarsky [:bzbarsky]]

So another option is to change PresShell::DoReflow such that if |root == rootFrame| and size.height is unconstrained we just force the mVResize flag to true.  That should only happen in SizeToContent cases, I believe, and will avoid the extra work that dirty reflows entail.   Run that idea by dbaron, though, just to make sure I'm not on crack?

Comment 34

[Jim Mathies [:jimm]]

Attachment: mVResize patch

(In reply to comment #33)
> So another option is to change PresShell::DoReflow such that if |root ==
> rootFrame| and size.height is unconstrained we just force the mVResize flag to
> true.  That should only happen in SizeToContent cases, I believe, and will
> avoid the extra work that dirty reflows entail.   Run that idea by dbaron,
> though, just to make sure I'm not on crack?

This seems to hit the case we're trying to catch. This is within a block where |mComputedHeight == NS_AUTOHEIGHT|, and |parentReflowState == nsnull| on root reflows.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 478659 [details] [diff] [review]
mVResize patch

I'd really rather do that in DoReflow, as I said.  parentReflowState can be null for non-root-frame reflows (specifically, for any reflow root), so this patch isn't right.  But in DoReflow we know whether we're really reflowing the root.

Comment 36

[Jim Mathies [:jimm]]

Attachment: DoReflow patch

Seems a little odd accessing these flags from outside InitResizeFlags. I could also hand down a flag in a new constructor if that seems more appropriate.

Comment 37

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

(In reply to comment #33)
> So another option is to change PresShell::DoReflow such that if |root ==
> rootFrame| and size.height is unconstrained we just force the mVResize flag to
> true.  That should only happen in SizeToContent cases, I believe, and will
> avoid the extra work that dirty reflows entail.   Run that idea by dbaron,
> though, just to make sure I'm not on crack?

The basic idea sounds good.

But I'm wondering if we need to force mVResize to true again on the way back (i.e., when switching from unconstrained height to fixed height) as well, when we reflow a second time with the size from the first time.  Aren't there percentage heights that will behave differently there?

Comment 38

[Jim Mathies [:jimm]]

(In reply to comment #37)
> (In reply to comment #33)
> > So another option is to change PresShell::DoReflow such that if |root ==
> > rootFrame| and size.height is unconstrained we just force the mVResize flag to
> > true.  That should only happen in SizeToContent cases, I believe, and will
> > avoid the extra work that dirty reflows entail.   Run that idea by dbaron,
> > though, just to make sure I'm not on crack?
> 
> The basic idea sounds good.
> 
> But I'm wondering if we need to force mVResize to true again on the way back
> (i.e., when switching from unconstrained height to fixed height) as well, when
> we reflow a second time with the size from the first time.  Aren't there
> percentage heights that will behave differently there?

A percentage height in this test case has no effect on sizeToContent. I'm not sure how it could, layout doesn't have a value to calculate a percentage of.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> Aren't there percentage heights that will behave differently there?

Hmm.... In quirks mode there almost certainly are.  Not sure about standards mode.

On the followup reflow, are we doing a ResizeReflow for which the old size is unconstrained and the new size is whatever the size was computed to be?

Comment 40

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

(In reply to comment #39)
> > Aren't there percentage heights that will behave differently there?
> 
> Hmm.... In quirks mode there almost certainly are.  Not sure about standards
> mode.
> 
> On the followup reflow, are we doing a ResizeReflow for which the old size is
> unconstrained and the new size is whatever the size was computed to be?

Well, on the followup, we don't even necessarily *do* a reflow if it doesn't involve resizing the widget, which seems rather bad.  (See nsXULWindow::SizeShellTo, called from DocumentViewerImpl::SizeToContent.)  I guess I won't worry about that bit right now.

If it weren't expensive in quirks mode, I'd say we should just set mVResize to true on the root element all the time.  (In quirks mode, it'll propagate to all non-fixed-height elements.)

As is, though, I think we should probably add a boolean to the pres shell (defaulting to false) called something like mLastRootReflowUnconstrainedHeight, and set and use it appropriately in this code (so we set mVResize if either the previous or the current reflow was unconstrained height).

Comment 41

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

Attachment: patch

jimm asked if I could write the patch I was thinking of; here it is.

I'll send this to try server shortly.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 479159 [details] [diff] [review]
patch

r=me

Comment 43

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

Landed: http://hg.mozilla.org/mozilla-central/rev/769faecf13a9

Thanks for all the work on this, Jim, and sorry for blaming the widget code on this one.  I clearly got that wrong.

Comment 44

[Jim Mathies [:jimm]]

(In reply to comment #43)
> Landed: http://hg.mozilla.org/mozilla-central/rev/769faecf13a9
> 
> Thanks for all the work on this, Jim, and sorry for blaming the widget code on
> this one.  I clearly got that wrong.

NP, thanks for putting together the final patch. (..and thanks for the help and patience bz!)