842080 - nsContainerFrame unnecessarily initializes the nsReflowStatus outparam that it passes to ReflowChild

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

We have this pattern in a few places in layout:
> 1148       nsReflowStatus frameStatus = NS_FRAME_COMPLETE;
> 1149 
> 1150       // Reflow
> 1151       rv = ReflowChild(frame, aPresContext, desiredSize, frameState,
> 1152                        prevRect.x, 0, aFlags, frameStatus, &tracker);
> 1153       NS_ENSURE_SUCCESS(rv, rv);
https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsContainerFrame.cpp#1148

As indicated by the reflow documentation in nsIFrame.h...
 https://mxr.mozilla.org/mozilla-central/source/layout/generic/nsIFrame.h#1871
... the nsReflowStatus is an *outparam* for reflow -- so there's no reason that we need to initialize it to NS_FRAME_COMPLETE before passing it to Reflow() or ReflowChild().

(We *do* need to make sure that we check whether reflow succeeded before we use the nsReflowStatus, of course. NS_ENSURE_SUCCESS takes care of that above.)

Filing this bug on removing unnecessary initializations of nsReflowStatus outparams here and elsewhere.

Comment 1

[Daniel Holbert [:dholbert]]

Actually, nsContainerFrame is the only place where we have this problem, AFAICT.

The other chunks of code that I initially thought were like this turned out to all be invocations of ReflowAbsoluteFrames & related methods, which expect to be merging their reflow-status into an already-initialized reflow-status.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This fixes the issue in nsContainerFrame and adds some documentation to nsFrame::ReflowAbsoluteFrames and nsAbsoluteContainingBlock::Reflow to indicate that *their* nsReflowStatus args *are* expected to be already-initialized.

(I also added XXXdholbert comments in 2 other spots where we should be able to leave a reflow status un-initialized, except that we don't check for reflow failure, so we can't depend on the status variable to be initialized in the code after the reflow call.)

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 714842 [details] [diff] [review]
fix v1

>+++ b/layout/generic/nsAbsoluteContainingBlock.h
>   // @param aForceReflow if this is false, reflow for some absolutely
>   //        positioned frames may be skipped based on whether they use
>   //        placeholders for positioning and on whether the containing block
>   //        width or height changed.
>+  // @param aReflowStatus This is assumed to be already-initialized, e.g. with
>+  //        the status of the delegating frame's main reflow. We'll merge in
>+  //        the statuses of the absolutely positioned children's reflows.
>+
>   nsresult Reflow(nsContainerFrame*        aDelegatingFrame,
>                   nsPresContext*           aPresContext,
>                   const nsHTMLReflowState& aReflowState,
>                   nsReflowStatus&          aReflowStatus,
>                   nscoord                  aContainingBlockWidth,
>                   nscoord                  aContainingBlockHeight,
>                   bool                     aConstrainHeight,
>                   bool                     aCBWidthChanged,

Actually, I just noticed that the existing parameter-documentation there, for 'aForceReflow', is stale, because that parameter was removed as part of the reflow branch (bug 300030) in 2006. :)

For reference, here's that CVS diff, removing the arg but leaving the documentation behind:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAbsoluteContainingBlock.h&branch=&root=/cvsroot&subdir=mozilla/layout/generic&command=DIFF_FRAMESET&rev1=1.31&rev2=1.32

(In the corresponding .cpp file, aForceReflow was replaced w/ a call to aReflowState.ShouldReflowAllKids(), FWIW.) 

So, I've just removed the 'aForceReflow' documentation in a '(no bug)' DONTBUILD cset...
 https://hg.mozilla.org/integration/mozilla-inbound/rev/ab16cbe1bf11
...and I'll rebase this patch on top of that change.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: fix v2 (rebased on top of comment tweak)

Rebased on top of the comment-tweak mentioned in prev. comment, and tweaked that chunk to no longer use "@param" notation, to match local style.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 714880 [details] [diff] [review]
fix v2 (rebased on top of comment tweak)

>layout/generic/nsContainerFrame.cpp
>-      nsReflowStatus frameStatus = NS_FRAME_COMPLETE;
>+      nsReflowStatus frameStatus;

Would it be possible to assert that the reflow does assign it?  E.g.
#define NS_FRAME_ILLEGAL_REFLOW_STATUS 0x80  (in nsIFrame.h)
nsReflowStatus frameStatus = NS_FRAME_ILLEGAL_REFLOW_STATUS;
and then MOZ_ASSERT(!(frameStatus & NS_FRAME_ILLEGAL_REFLOW_STATUS));
after the NS_ENSURE_SUCCESS.

>layout/generic/nsFrame.cpp
>+    // XXXdholbert Why aren't we checking the rv of the various
>+    // reflow methods here?

I think we should make all reflow methods return |void| instead.
AFAIK, the only failures in the past were OOM (all allocations during
reflow should be infallible now), and some MathML markup error checks
that we should just fix (preferably by moving those checks to frame
construction time).  If there any other they should probably be
MOZ_ASSERTs instead.

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren [:mats] from comment #5)
> >layout/generic/nsContainerFrame.cpp
> >-      nsReflowStatus frameStatus = NS_FRAME_COMPLETE;
> >+      nsReflowStatus frameStatus;
> 
> Would it be possible to assert that the reflow does assign it?  E.g.
> #define NS_FRAME_ILLEGAL_REFLOW_STATUS 0x80  (in nsIFrame.h)
> nsReflowStatus frameStatus = NS_FRAME_ILLEGAL_REFLOW_STATUS;
> and then MOZ_ASSERT(!(frameStatus & NS_FRAME_ILLEGAL_REFLOW_STATUS));
> after the NS_ENSURE_SUCCESS.

I guess we could... That'd be an extra couple lines of boilerplate for each reflow, but maybe that's fine.

> >layout/generic/nsFrame.cpp
> >+    // XXXdholbert Why aren't we checking the rv of the various
> >+    // reflow methods here?
> 
> I think we should make all reflow methods return |void| instead.

That sounds like a significantly larger change. :)

My intention with the two code-snippets you quoted were to clean up one inconsistency (leave outparam uninitialized) and to annotate another one where we technically can't clean up the inconsistency because we can't be sure that our outparam is being set.  Are you OK w/ leaving this bug to cover that (minor) stuff, if I file followups on performing your suggested larger enhancement in separate bugs?

Comment 7

[Mats Palmgren (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> I guess we could... That'd be an extra couple lines of boilerplate for each
> reflow, but maybe that's fine.

Adding it to the one place in nsContainerFrame.cpp is enough.
I was mostly interested in knowing if such a thing would pass
a debug-build Try run -- just to double check we don't miss anything...

> > I think we should make all reflow methods return |void| instead.
> 
> That sounds like a significantly larger change. :)

Yes, that should definitely be a separate bug :-)
And it should probably be discussed and investigated a bit deeper --
others may different opinions.
I just wanted to point out that adding comments and/or code to deal
with reflow errors probably isn't worth doing.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren [:mats] from comment #7)
> (In reply to Daniel Holbert [:dholbert] from comment #6)
> > I guess we could... That'd be an extra couple lines of boilerplate for each
> > reflow, but maybe that's fine.
> 
> Adding it to the one place in nsContainerFrame.cpp is enough.
> I was mostly interested in knowing if such a thing would pass
> a debug-build Try run -- just to double check we don't miss anything...

Try run:
 https://tbpl.mozilla.org/?tree=Try&rev=2aec6816394e
w/ the attached patch:
  https://hg.mozilla.org/try/rev/875884832685
and a patch along the lines of what you suggested (creating a bogus reflowStatus bit, which we set in nsContainerFrame::ReflowChild() just before calling Reflow, and which we then check to make sure it's gone after Reflow completes):
  https://hg.mozilla.org/try/rev/fa419aff3321

That's gotten a full (green) Try run on OS X 10.8 debug -- woot.

Comment 9

[Daniel Holbert [:dholbert]]

(I just started one more try push w/ the bogus reflow status also being set up one level, at the chunk of nsContainerFrame that this patch modifies, in case that's what you had in mind: https://tbpl.mozilla.org/?tree=Try&rev=786b9d40936c )

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: fix v3 (XXXdholbert comments removed)

Both try pushes look good. Posting updated patch, w/ "we should be checking reflow return-value" comments removed, per end of comment 7.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 716393 [details] [diff] [review]
fix v3 (XXXdholbert comments removed)

Thanks for testing.  r=mats

Comment 12

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8bf2dfc825

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ef8bf2dfc825