264620 - crash at nsTableOuterFrame::OuterReflowChild line 1317

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

steps to reproduce:
1. load URL
2. hover over the images at the top of the page
3. TB1335249X, TB133187Z

Comment 1

[Bernd]

Attachment: stacktrace

Comment 2

[Bernd]

we crash at

  if (aNeedToReflowCaption && !*aNeedToReflowCaption &&
      mInnerTableFrame == aChildFrame) {
    nsHTMLReflowCommand* command = childRS.path->mReflowCommand;
    if (command) {
      nsReflowType type;
      command->GetType(type);
      *aNeedToReflowCaption = eReflowType_StyleChanged == type;
    }
  }
  childRS.path is 0x00000

Comment 3

[Bernd]

Attachment: patch

Only during the incr reflow the path should be dereferenced.

see
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsHTMLReflowState.cpp#177

Comment 4

[Bernd]

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

asking for r/sr and all approvals as this should also go into the branches. The
patch is low risk and does not regress the use case that this code covers (bug
222846)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

r+sr=bzbarsky

Comment 6

[Bernd]

fixed on trunk

Comment 7

[Asa Dotzler [:asa]]

bernd, please include a quick explanation of what the patch does, what kind of
testing it's had and what kinds of risks are associated with taking the patch.
doing that will help to expidite approval requests. thanks.

Comment 8

[Bernd]

Before the patch I assumed that we can always retrieve the reflow command from
the reflow path. This is wrong. The only situation where the path is non zero is
during an incr. reflow for the child frame see code snippet below.

177  if (reason == eReflowReason_Incremental) {
178     // If the child frame isn't along the reflow path, then convert
179     // the incremental reflow to a dirty reflow.
180     path = aParentReflowState.path->GetSubtreeFor(aFrame);
181     if (! path)
182       reason = eReflowReason_Dirty;
183   }
184   else
185     path = nsnull;

I tested the patch with files from the regression tests. 

I consider the risk low, as this is only a fixup when we need to reflow the
caption frame in addition to the table frame. The worst thing that could happen
is that we don't reflow the caption as previously. But I tested it specially
with the testcases from the bug where this code was introduced.

Comment 9

[Asa Dotzler [:asa]]

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

a=asa for aviary and 1.7 checkin.

Comment 10

[Bernd]

fix checked in into the branches