157526 - mCBReflowState can be uninitialized in some cases (nsHTMLReflowState::InitCBReflowState never called)

Status: NEW

(Core :: Layout, defect)

Description

[Jochen Suckfuell]

There is a case where nsHTMLReflowState::InitCBReflowState doesn't set the
mCBReflowState member variable. This leads to a segfault later on. What's more,
even the constructors don't set this member to anything useful (like nsnull).
This would be important because on of the constructors might not call
Init-->InitCBReflowState .

Comment 1

[Boris Zbarsky [:bzbarsky]]

Jochen, what's the case in which nsHTMLReflowState::InitCBReflowState does not
set mCBReflowState?  I just looked at it, and every single early return sets
mCBReflowState before returning; the last statement in the function also sets
mCBReflowState.

Comment 2

[Jochen Suckfuell]

Boris, you're right. The current revision of the file sets the member in the
last line while assuming that the parentReflowState is set.

Yet there's still a case where InitCBReflowState isn't called from the one
constructor that takes a PRBool aInit to decide if Init() should happen. In this
case mCBReflowState should at least be initialized to nsnull.

The way I ran into the segfault was from a module I wrote for html2pdf
conversion. It uses the layout engine, but cannot use WebShell nor
DocumentViewer. It may well be that I initialized some classes differently than
the original mozilla.

Comment 3

[ly nguyen]

Attachment: stack trace

Comment 4

[ly nguyen]

we've had 96 crashes on the windows platform yesterday alone. since twister had
problems with symbols last week, the talkback team apologizes for not catching
this sooner. attached is the stack trace gathered from talkback.

Comment 5

[ly nguyen]

from the users who commented, everyone is having launch/startup problems. looks
like it dies before the profile manager even executes (at least visibly).

Comment 6

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

comment 3, comment 4, and comment 5 are about bug 161359, not this bug.  Please
don't add any more comments about that crash to this bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Just to clarify

This bug is about a possible situation in which
nsHTMLReflowState::InitCBReflowState is _never_called_, which leads to a reflow
state with an uninitialized mCBReflowState. The only way that could crash is if
a child reflow state that was not a CBReflowState itself then got initialized
with the "broken" reflow state as a parent.

The crash you are seeing is happening when the reflow state is constructed in  
 PresShell::InitialReflow.  This reflow state has no parents.  If it crashes on
construction, then almost certainly the root frame getting passed in is null or
bogus.

I think the bug you wanted is bug 161359.  Which was due to a bogus build.

Removing all the bogus markings.

Comment 8

[ly nguyen]

thx boris for the clarification =) i really appreciate it.

Comment 9

[Boris Zbarsky [:bzbarsky]]

.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'll look at this once I finish making the mCBReflowState changes in bug 135082

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, so it looks like the only case we have to worry about is when a reflow state
for a kid is constructed with aInit == PR_FALSE and then caller never calls Init().

In this case, not only mCBReflowState but also a whole slew of other pointer
members (eg all the style struct) members will be null.

At the moment, the "no init" constructor is called by at least nsBlockFrame and
the various table frames.  Tables just want to pass in border-collapsed border
and padding to Init(), which we could roll into the constructor args.  But
nsBlockFrame actually creates and inits the reflow state at rather distinct
points...  The block code has an XXX comment by dbaron that that computation
should just happen in the reflow state itself.  I think if we fixed that we
could just eliminate the no-init version of the constructor altogether
(replacing it with one that takes optional nsMargins for the border and
padding), and make Init() a protected method of nsHTMLReflowState, called from
every constructor.

Does that seem reasonable?