Closed Bug 794693 Opened 7 years ago Closed 6 years ago

crash in nsHTMLReflowState::ComputeContainingBlockRectangle

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: martijn.martijn, Assigned: jwir3)

References

Details

(Keywords: crash, testcase, Whiteboard: [native-crash][font-inflation])

Crash Data

Attachments

(3 files, 5 obsolete files)

This bug was filed from the Socorro interface and is 
report bp-fdb0296e-82f9-4d7c-9bc2-607a42120927 .
============================================================= 
0 	xul.dll 	nsHTMLReflowState::ComputeContainingBlockRectangle 	layout/generic/nsHTMLReflowState.cpp:1708
1 	xul.dll 	nsHTMLReflowState::InitConstraints 	layout/generic/nsHTMLReflowState.cpp:1848
2 	xul.dll 	nsHTMLReflowState::Init 	layout/generic/nsHTMLReflowState.cpp:314
3 	xul.dll 	nsHTMLReflowState::nsHTMLReflowState 	layout/generic/nsHTMLReflowState.cpp:185
4 	xul.dll 	ComputeDescendantWidth 	layout/generic/nsFontInflationData.cpp:162
5 	xul.dll 	nsFontInflationData::UpdateWidth 	layout/generic/nsFontInflationData.cpp:208
6 	xul.dll 	nsFontInflationData::UpdateFontInflationDataWidthFor 	layout/generic/nsFontInflationData.cpp:62
7 	xul.dll 	nsHTMLReflowState::InitResizeFlags 	layout/generic/nsHTMLReflowState.cpp:440
8 	xul.dll 	nsHTMLReflowState::Init 	layout/generic/nsHTMLReflowState.cpp:316
9 	xul.dll 	nsHTMLReflowState::nsHTMLReflowState 	layout/generic/nsHTMLReflowState.cpp:78
10 	xul.dll 	nsFrame::BoxReflow 	layout/generic/nsFrame.cpp:7989

This crash happens when font inflation is turned on, e.g. when the value of the font.size.inflation.minTwips preference is something else than the default.
Still occurs in a Firefox trunk debug build on Linux64.
OS: Windows NT → All
Hardware: x86 → All
Whiteboard: [font-inflation]
Looks like it occurs also in release builds, bp-ebce00dc-13c4-4b4e-b417-1d7062130423
Scott, can you take a look?
Flags: needinfo?(sjohnson)
Attached patch b794693 (obsolete) — Splinter Review
Sorry for the late response on this one. Basically, the logic in nsHTMLReflowState::InitResizeFlags() needs to be reordered. We already check to see if this crash would exist (i.e. if we have a dummy parentRS), but we do this after the initial call to UpdateFontInflationDataWidthFor().

So, this patch does exactly that and adds a crashtest with the testcase attached to this bug inside of it.
Assignee: nobody → sjohnson
Attachment #752554 - Flags: review?(matspal)
Flags: needinfo?(sjohnson)
Attached patch b794693 (v2) (obsolete) — Splinter Review
Needed to make the crashtest a reftest instead, so that I can modify the pref.
Attachment #752554 - Attachment is obsolete: true
Attachment #752554 - Flags: review?(matspal)
Attachment #752562 - Flags: review?(matspal)
Comment on attachment 752554 [details] [diff] [review]
b794693

> Needed to make the crashtest a reftest instead, so that I can modify the
> pref.

That shouldn't be necessary, just add the pref() before "load" in this
patch instead.  As far as I know, reftest/crashtest manifest files
supports the exact same features and execute in the same way so the
"reftest" vs. "crashtest" division is merely convention.
Attachment #752554 - Attachment is obsolete: false
Attachment #752554 - Flags: review+
Attachment #752562 - Attachment is obsolete: true
Attachment #752562 - Flags: review?(matspal)
(In reply to Mats Palmgren [:mats] from comment #5)
> Comment on attachment 752554 [details] [diff] [review]
> b794693
> 
> > Needed to make the crashtest a reftest instead, so that I can modify the
> > pref.
> 
> That shouldn't be necessary, just add the pref() before "load" in this
> patch instead.  As far as I know, reftest/crashtest manifest files
> supports the exact same features and execute in the same way so the
> "reftest" vs. "crashtest" division is merely convention.

Ah, yes. I added a space between the pref and the value causing a manifest error. I assumed it was because I couldn't use test-pref in crashtests.list, but that was an error.

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89814d2461af
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/89814d2461af
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 877160
This patch seems wrong since (as the dependency shows), we need to call UpdateFontInflationDataWidth (which exists primarily for its side-effects, but returns whether anything changed) for anything that's a flow root; otherwise it won't have a FontInflationData or that object will be out-of-date.  It's only the |dirty| condition that was intended to be different.
(In reply to David Baron [:dbaron] (in W3C meetings through June 11) (don't cc:, use needinfo? instead) from comment #8)
> This patch seems wrong since (as the dependency shows), we need to call
> UpdateFontInflationDataWidth (which exists primarily for its side-effects,
> but returns whether anything changed) for anything that's a flow root;
> otherwise it won't have a FontInflationData or that object will be
> out-of-date.  It's only the |dirty| condition that was intended to be
> different.

I agree with this assessment. My original thinking here was as follows:

What was occurring was that we had some frame, say frame X, which had descendants frames Y and Z. During the UpdateWidth(reflow state of X), we computed our nearest common ancestor of frames Y and Z to be some frame T, where T != X. Then, inside of ComputeDescendantWidth(reflow state of X, frame T), we create a list of frames from T to X up the tree, and construct a reflow state for each of these.

At some point, when creating the nsHTMLReflowState for one of these frames, we encounter a situation where the frame does not have a parentReflowState. Thus, inside of InitCBReflowState(), we set mCBReflowState to nullptr, and then return. When we call InitConstraints(), it then tries to get the ContainingBlockRectangle using this null reflow state.

My assertion was that parentReflowState was null inside of InitCBReflowState() iff mDummyParentReflowState is set to true. Even if this were true (which it isn't), changing the if statement in the patch above doesn't fix the bug, since we're not calling UpdateFontInflationDataWidthFor() on the descendant reflow state. 

I'll prepare a backout patch and post it to the dependency bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Ok, I added a backout for this patch, as it doesn't correctly fix the problem. One note, though, I think once we land an actual fix for this, we should also land a test that tests for the behavior in the testcase for the dependency. 

None of the current font-inflation tests exhibited the behavior of the regression.
I think I see the underlying problem. What happens is that we are computing font inflation data, and constructing reflow states for the list of frames mentioned in comment 9. During this, we construct a reflow state for a table frame, T1, such that T1's parent reflow state is T0. The problem is that T0 doesn't have a containing block reflow state. Or, more specifically, because T0 does not have a parent reflow state, it's mCBReflowState variable is intentionally set to nullptr. 

Because we do special handling for inner table frames in:
mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#386

we end up setting the mCBReflowState to nullptr for T1. However, because T1's parentReflowState is not nullptr, this is unexpected when we get to:
mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.cpp#386

There's a couple of different ways we could handle this, but I'm not sure how the structure _should_ look in this situation. At least, I think we need to assert in InitConstraints() that we expect either A) the parentReflowState to be nullptr or B) BOTH the parentReflowState and mCBReflowState are not null. The problem is that we're checking to see if parentReflowState is null, but then assuming that if parentReflowState is non-null, mCBReflowState must be non-null as well. 

Alternatively, inside of InitCBReflowState(), we could change:
>     if (frame->GetType() == nsGkAtoms::tableFrame) {
to:
>     if (frame->GetType() == nsGkAtoms::tableFrame && parentReflowState->mCBReflowState) {

which would effectively fail safely by making our parentReflowState our containing block reflow state, if the parent reflow state doesn't have another containing block reflow state for an inner table frame. 

We've already checked up above for whether the containing block frame is actually the parent reflow state's frame, so I think this is the right thing to do to fix the bug, but I wasn't absolutely sure how this structure should look for table frames, so I thought I would ask first before posting a patch.
Flags: needinfo?(dbaron)
(In reply to Scott Johnson (:jwir3) from comment #11)
> I think I see the underlying problem. What happens is that we are computing
> font inflation data, and constructing reflow states for the list of frames
> mentioned in comment 9. During this, we construct a reflow state for a table
> frame, T1, such that T1's parent reflow state is T0. The problem is that T0
> doesn't have a containing block reflow state. Or, more specifically, because
> T0 does not have a parent reflow state, it's mCBReflowState variable is
> intentionally set to nullptr. 
> 
> Because we do special handling for inner table frames in:
> mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.
> cpp#386
> 
> we end up setting the mCBReflowState to nullptr for T1. However, because
> T1's parentReflowState is not nullptr, this is unexpected when we get to:
> mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.
> cpp#386

Hmmm.  So the second link here is the same as the first rather than what you meant.  But that's not a big deal.

> There's a couple of different ways we could handle this, but I'm not sure
> how the structure _should_ look in this situation. At least, I think we need
> to assert in InitConstraints() that we expect either A) the
> parentReflowState to be nullptr or B) BOTH the parentReflowState and
> mCBReflowState are not null. The problem is that we're checking to see if
> parentReflowState is null, but then assuming that if parentReflowState is
> non-null, mCBReflowState must be non-null as well. 

It's not clear to me how asserting would fix the crash -- you'd need to make that assertion true.

More importantly, though, it's not clear to me why we're *not* crashing without font inflation.  I'd have expected that we would construct the same chain of reflow states in reflow that we are in ComputeDescendantWidth -- which I would think would lead to the same crash.  But apparently we're not.  So I think the interesting question is what ComputeDescendantWidth is doing differently, since I think the easiest fix would be to fix that.

> Alternatively, inside of InitCBReflowState(), we could change:
> >     if (frame->GetType() == nsGkAtoms::tableFrame) {
> to:
> >     if (frame->GetType() == nsGkAtoms::tableFrame && parentReflowState->mCBReflowState) {
> 
> which would effectively fail safely by making our parentReflowState our
> containing block reflow state, if the parent reflow state doesn't have
> another containing block reflow state for an inner table frame. 

This seems incorrect; it would break the rules about what mCBReflowState means.
Flags: needinfo?(dbaron)
Whiteboard: [font-inflation] → [native-crash][font-inflation]
Attached patch b794693 (v3) (obsolete) — Splinter Review
When we're at the box-block interface in nsFrame::BoxReflow, we set up a dummy parent structure. Unfortunately, this causes problems when we're constructing font inflation data for table frames at this interface. Instead of doing this for frames in this special case, this patch makes the code wait to perform the font inflation data initialization until after the correct parent hierarchy is set up.
Attachment #752554 - Attachment is obsolete: true
Attachment #781229 - Flags: review?(dbaron)
Comment on attachment 781229 [details] [diff] [review]
b794693 (v3)

This seems like basically the right idea, but it seems like it makes future modifications to this code even more dangerous than they already are, both in terms of other users of the constructor of nsHTMLReflowState, and in terms of being able to maintain the nsHTMLReflowState code in a sane way (in terms of what gets initialized before what else).

Is it possible we could instead construct |reflowState| using the constructor that takes a parent reflow state, and do the which-parent-reflow-state-to-set calculation beforehand?  If that's possible, I think we should.  If not, I think you should instead use the caller-will-call-Init concept (currently the aInit parameter), which would require extending that concept to the other constructor.  (The caller-will-call-Init business that we already have is much less scary because it means moving everything after a certain point into the constructor, rather than taking a piece out of the middle.)

Either way, I think you should convert (probably in a separate patch, first) the aInit parameter to the with-aParentReflowState constructor into a aFlags parameter, and make it a CALLER_WILL_INIT flag (with the opposite boolean sense).

Then you'd either (for the first approach) need to add support for DUMMY_PARENT_REFLOW_STATE to the with-aParentReflowState constructor or (for the second approach) need to add support for CALLER_WILL_INIT to the without-aParentReflowState constructor.

Or something like that.

I think you also need to look at the other user of DUMMY_PARENT_REFLOW_STATE.
Attachment #781229 - Flags: review?(dbaron) → review-
Attached patch b794693-part1 (obsolete) — Splinter Review
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #15)
> Either way, I think you should convert (probably in a separate patch, first)
> the aInit parameter to the with-aParentReflowState constructor into a aFlags
> parameter, and make it a CALLER_WILL_INIT flag (with the opposite boolean
> sense).

Performed this modification as part of patch for part1.
Attachment #781229 - Attachment is obsolete: true
Attachment #786346 - Flags: review?(dbaron)
Attached patch b794693-part2 (obsolete) — Splinter Review
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #15)
> Comment on attachment 781229 [details] [diff] [review]
> b794693 (v3)
> 
> This seems like basically the right idea, but it seems like it makes future
> modifications to this code even more dangerous than they already are, both
> in terms of other users of the constructor of nsHTMLReflowState, and in
> terms of being able to maintain the nsHTMLReflowState code in a sane way (in
> terms of what gets initialized before what else).
> 
> Is it possible we could instead construct |reflowState| using the
> constructor that takes a parent reflow state, and do the
> which-parent-reflow-state-to-set calculation beforehand?  If that's
> possible, I think we should.  

This patch, part 2, takes this approach. I was wary of doing this initially, because I wasn't entirely sure what it would cause problems with in terms of calculating widths/heights during the initialization of nsHTMLReflowState for the child.

The other thing I wasn't entirely sure about was that the containing block is computed, but not passed in as a parameter. That is, it's set after the reflow state is constructed, but this throws away the containing block reflow state that was found during InitCBReflowState(). The way I've written the patch, this fixes the crash but doesn't change the behavior of what mCBReflowState is prior to actually performing reflow within nsFrame::BoxReflow().

> I think you also need to look at the other user of DUMMY_PARENT_REFLOW_STATE.

So, I've looked at this, and it looks like there are three users of DUMMY_PARENT_REFLOW_STATE:

http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7719
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7826
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#7847

The second and third of these are in BoxReflow(), and are essentially setting the parentReflowState's parent to a dummy value, as well as the child (unless we detect that we have an actual parent).

The other usage is in ReflowAbsoluteFrames() (or, rather, right before ReflowAbsoluteFrames() is called). I'm not sure if anything has to change here - both this usage and the usage in BoxReflow() that sets up the parent reflow state to have a dummy parent use the non-parent constructor, so I don't think the behavior is going to change (i.e. we're not asserting in this case that we have a viable parent, so I don't think this changes the behavior).

It's also possible that I am misunderstanding what you're saying here.
Attachment #786353 - Flags: review?(dbaron)
Comment on attachment 786353 [details] [diff] [review]
b794693-part2

Accidentally uploaded the wrong patch. Stand by.
Attachment #786353 - Flags: review?(dbaron)
Comment on attachment 786353 [details] [diff] [review]
b794693-part2

On second thought, this is the correct one. I just got mixed up. :)
Attachment #786353 - Flags: review?(dbaron)
Comment on attachment 786353 [details] [diff] [review]
b794693-part2

I noticed a failure on try server that I want to get a better handle on before this goes through final review.
Attachment #786353 - Flags: review?(dbaron)
Comment on attachment 786346 [details] [diff] [review]
b794693-part1

Review of attachment 786346 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/tables/nsTableRowFrame.cpp
@@ +29,5 @@
>                           nsIFrame*                aFrame,
>                           const nsSize&            aAvailableSpace,
>                           bool                     aInit = true)
>      : nsHTMLReflowState(aPresContext, aParentReflowState, aFrame,
> +                        aAvailableSpace, -1, -1, nsHTMLReflowState::CALLER_WILL_INIT)

The last parameter here is supposed to be: aInit ? 0 : nsHTMLReflowState::CALLER_WILL_INIT
Comment on attachment 786346 [details] [diff] [review]
b794693-part1

Sorry for the bugmail spam. I'm trying to figure out what's causing an xpcshell test failure, and I will resubmit for review after determining the issue.
Attachment #786346 - Flags: review?(dbaron)
Attached patch b794693-part1Splinter Review
After chasing what appears to have been a red herring on try-server, I'm now re-requesting review for these two patches. They are the same as before, but there are a couple of stylistic changes (I put the nsHTMLReflowState constructors closer together for clarity, and adjusted a couple of spacing issues I found near the code I touched), as well as a fix described in comment 21.
Attachment #786346 - Attachment is obsolete: true
Attachment #786353 - Attachment is obsolete: true
Attachment #790325 - Flags: review?(dbaron)
Attached patch b794693-part2Splinter Review
Attachment #790388 - Flags: review?(dbaron)
dbaron: review ping
Comment on attachment 790325 [details] [diff] [review]
b794693-part1

Please don't reorder the functions, and particularly don't do it in a
patch that makes other substantive changes.  (If you wanted to do it
it should be in its own patch, but it has the disadvantage of messing
up blame, which doesn't seem worth it here.)  (Fixing the indentation
of aRenderingContext is fine, though.)

There's no need to add mCallerWillInit to mFlags.  Just check
aFlags & CALLER_WILL_INIT in the constructors, since that's the
only place you need to check it.

You should document in the defintion of the flag enum that the
DUMMY_PARENT_REFLOW_STATE flag is only supported by the constructor for
root reflow states.


>+   * Initialize a ROOT reflow state with a rendering context. Typically, this
>+   * is used for measuring things.

Move the "with a rendering context. Typically, this is used for measuring
things." bit to the definition of aRenderingContext (though perhaps replacing
it with what I comment there).

>+   *
>+   * @param aPresContext The presentation context in which the frame for which
>+   *        the reflow state is being constructed is a part.

I'd just say "must be equal to aFrame->PresContext()"

(twice, once for each constructor)

>+   * @param aFrame A pointer to the frame for which this reflow state is being
>+   *        constructed.

No need for "A pointer to".  And maybe "for which" -> "for whose reflow".

(twice, once for each constructor)

>+   * @param aRenderingContext A pointer to the rendering context for which this
>+   *        reflow state is being constructed within. This is used for
>+   *        performing rendering measurements.

Again, drop "A pointer to".

"constructed within" doesn't make sense here; rendering contexts are
often temporary.  How about just "Rendering context to use for measurements".

>+   * @param aAvailableSpace The available width and height for the frame for
>+   *        which this reflow state is being constructed. Can be (and often is)
>+   *        NS_UNCONSTRAINEDSIZE.

Your comment about NS_UNCONSTRAINEDSIZE isn't right for the width; I'd
also prefer to have this documented in just one place.  So instead of this,
just say "see comments for availableWidth and availableHeight members".

(twice, once for each constructor)

>+   * @param aFlags A set of ReflowStateFlags passed to the constructor to
>+   *        indicate special handling of this reflow state.

ReflowStateFlags isn't a name used in the code.  How about just
"flags for additional boolean parameters (see below)"

(twice, once for each constructor)

You also need to fix more callers than just the two you fixed:
  nsTableFrame::SetupHeaderFooterChild
  nsTableFrame::PlaceRepeatedFooter
  nsTableFrame::ReflowChildren
  nsTableOuterFrame::GetChildMargin
  nsTableRowGroupFrame::ReflowChildren
  nsTableRowGroupFrame::SplitSpanningCells
  nsTableRowGroupFrame::SplitRowGroup
  nsHTMLScrollFrame::ReflowScrolledFrame

Once you think you've fixed all the callers, you should test that you've
gotten it right by declaring (but not defining) an additional
constructor with the same signature as the old one and making it
*private*, so that any uses of it will be compilation errors, making
sure that such a constructor causes a compilation error if you don't fix
a callsite, and then making sure you don't get any such compilation
errors.  Don't check that in, though.

I think it would also be better to convert TableCellReflowState's
constructor to take the same aFlags, and convert the callers.

r=dbaron with that
Attachment #790325 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (needinfo? me) from comment #26)
> You should document in the defintion of the flag enum that the
> DUMMY_PARENT_REFLOW_STATE flag is only supported by the constructor for
> root reflow states.

Er, scratch that; this patch fixed it so that's not the case anymore.
Comment on attachment 790388 [details] [diff] [review]
b794693-part2

Could you move this chunk of code:
>     // XXX Is it OK that this reflow state has no parent reflow state?
>     // (It used to have a bogus parent, skipping all the boxes).
>     nsSize availSize(aWidth, NS_INTRINSICSIZE);
down so it stays next to the lines that were below it?

(Probably also worth changing "no parent" to "only one ancestor".)

>+    nsHTMLReflowState *parentRS = nullptr;
>+    nsHTMLReflowState *containingBlockRS = nullptr;

Rather than const_cast, just declare these as "const nsHTMLReflowState*".

(Also, heycam would want me to tell you to write "* " rather than " *".)

Also, no need to initialize to nullptr; they're obviously initialized
in all codepaths.

Also, it seems silly to have two variables when one could do.  I think
just use |parentRS|.

>+    reflowState.mCBReflowState = containingBlockRS;

I think it's worth adding a comment that this is a little fishy; if it's
actually changing things (which it might be), then it would be good to
make it happen correctly the first time around.

>+    reflowState.parentReflowState = parentRS;

This should no longer be needed since we passed it in the constructor.

>+/**
>+ * Check to make sure that the next-in-flow of a given frame is the same as a
>+ * parent frame's next-in-flow.
>+ *
>+ * @param aFrame A frame for which to check parenthood.
>+ * @param aParent A parent frame to check aFrame against (likely aFrame's
>+ *        parent frame).
>+ *
>+ * @returns true, if aFrame has a next-in-flow, aParent has a next-in-flow, and
>+ *          the parent of aFrame's next-in-flow is the same as aParent's
>+ *          next-in-flow.
>+ */

This is way too verbose a comment for a 3 line function.  It's easier to
read the function than the comment.  I think it's best to just remove
this change from the patch.

r=dbaron with those things fixed
Attachment #790388 - Flags: review?(dbaron) → review+
Comment on attachment 790388 [details] [diff] [review]
b794693-part2

Also, please add the crashtest to the manifest!
(and remember that it needs font-inflation prefs set in the manifest -- please test that he way you put it in the manifest crashes without the patch)
(In reply to David Baron [:dbaron] (needinfo? me) from comment #26)
> Please don't reorder the functions, and particularly don't do it in a
> patch that makes other substantive changes.  (If you wanted to do it
> it should be in its own patch, but it has the disadvantage of messing
> up blame, which doesn't seem worth it here.)  (Fixing the indentation
> of aRenderingContext is fine, though.)

Fixed.

> There's no need to add mCallerWillInit to mFlags.  Just check
> aFlags & CALLER_WILL_INIT in the constructors, since that's the
> only place you need to check it.

Fixed.

> You should document in the defintion of the flag enum that the
> DUMMY_PARENT_REFLOW_STATE flag is only supported by the constructor for
> root reflow states.

Fixed, but see comment above about how this is changed in this patch.

> >+   * Initialize a ROOT reflow state with a rendering context. Typically, this
> >+   * is used for measuring things.
> 
> Move the "with a rendering context. Typically, this is used for measuring
> things." bit to the definition of aRenderingContext (though perhaps replacing
> it with what I comment there).

Fixed.
 
> >+   *
> >+   * @param aPresContext The presentation context in which the frame for which
> >+   *        the reflow state is being constructed is a part.
> 
> I'd just say "must be equal to aFrame->PresContext()"

Fixed.

> >+   * @param aFrame A pointer to the frame for which this reflow state is being
> >+   *        constructed.
> 
> No need for "A pointer to".  And maybe "for which" -> "for whose reflow".

Fixed.

> >+   * @param aRenderingContext A pointer to the rendering context for which this
> >+   *        reflow state is being constructed within. This is used for
> >+   *        performing rendering measurements.
> 
> Again, drop "A pointer to".

Fixed.

> "constructed within" doesn't make sense here; rendering contexts are
> often temporary.  How about just "Rendering context to use for measurements".

Fixed.

> >+   * @param aAvailableSpace The available width and height for the frame for
> >+   *        which this reflow state is being constructed. Can be (and often is)
> >+   *        NS_UNCONSTRAINEDSIZE.
> 
> Your comment about NS_UNCONSTRAINEDSIZE isn't right for the width; I'd
> also prefer to have this documented in just one place.  So instead of this,
> just say "see comments for availableWidth and availableHeight members".

Fixed.

> >+   * @param aFlags A set of ReflowStateFlags passed to the constructor to
> >+   *        indicate special handling of this reflow state.
> 
> ReflowStateFlags isn't a name used in the code.  How about just
> "flags for additional boolean parameters (see below)"

Well, ReflowStateFlags is the tag name given to the struct used to define mFlags. That said, I changed the comment to reflect what you're saying here.

> You also need to fix more callers than just the two you fixed:
>   nsTableFrame::SetupHeaderFooterChild
>   nsTableFrame::PlaceRepeatedFooter
>   nsTableFrame::ReflowChildren
>   nsTableOuterFrame::GetChildMargin
>   nsTableRowGroupFrame::ReflowChildren
>   nsTableRowGroupFrame::SplitSpanningCells
>   nsTableRowGroupFrame::SplitRowGroup
>   nsHTMLScrollFrame::ReflowScrolledFrame
> 
> Once you think you've fixed all the callers, you should test that you've
> gotten it right by declaring (but not defining) an additional
> constructor with the same signature as the old one and making it
> *private*, so that any uses of it will be compilation errors, making
> sure that such a constructor causes a compilation error if you don't fix
> a callsite, and then making sure you don't get any such compilation
> errors.  Don't check that in, though.

Fixed. I've verified using the method you mentioned to enlist the compiler to help me catch these. In addition to the ones you mentioned, there were some in Flexbox code (nsFlexContainerFrame.cpp) and MathML frames (nsMathMLContainerFrame.cpp).

> I think it would also be better to convert TableCellReflowState's
> constructor to take the same aFlags, and convert the callers.

Fixed.
(In reply to David Baron [:dbaron] (needinfo? me) from comment #28)
> Comment on attachment 790388 [details] [diff] [review]
> b794693-part2
> 
> Could you move this chunk of code:
> >     // XXX Is it OK that this reflow state has no parent reflow state?
> >     // (It used to have a bogus parent, skipping all the boxes).
> >     nsSize availSize(aWidth, NS_INTRINSICSIZE);
> down so it stays next to the lines that were below it?
> 
> (Probably also worth changing "no parent" to "only one ancestor".)

Fixed.

> 
> >+    nsHTMLReflowState *parentRS = nullptr;
> >+    nsHTMLReflowState *containingBlockRS = nullptr;
> 
> Rather than const_cast, just declare these as "const nsHTMLReflowState*".

Fixed.

> (Also, heycam would want me to tell you to write "* " rather than " *".)

Fixed.

> Also, no need to initialize to nullptr; they're obviously initialized
> in all codepaths.

Fixed.

> Also, it seems silly to have two variables when one could do.  I think
> just use |parentRS|.

I removed the other pointer.

> >+    reflowState.mCBReflowState = containingBlockRS;
> I think it's worth adding a comment that this is a little fishy; if it's
> actually changing things (which it might be), then it would be good to
> make it happen correctly the first time around.

Comment added.

> >+    reflowState.parentReflowState = parentRS;
> This should no longer be needed since we passed it in the constructor.

Removed.

> This is way too verbose a comment for a 3 line function.  It's easier to
> read the function than the comment.  I think it's best to just remove
> this change from the patch.

Yeah, this was kind of silly. I thought it would be useful at the time I was working with this code. I've removed it.
(In reply to David Baron [:dbaron] (needinfo? me) from comment #29)
> Comment on attachment 790388 [details] [diff] [review]
> b794693-part2
> 
> Also, please add the crashtest to the manifest!

Done.

(In reply to David Baron [:dbaron] (needinfo? me) from comment #30)
> (and remember that it needs font-inflation prefs set in the manifest --
> please test that he way you put it in the manifest crashes without the patch)

Done. I've verified this by adding the crashtest before the patches, verifying that the crashtest fails, and then re-merging the code for the crashtest into this patch.
https://hg.mozilla.org/mozilla-central/rev/ee3ddbc1a590
https://hg.mozilla.org/mozilla-central/rev/ec56bbf4c57c
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.