Closed Bug 366012 Opened 18 years ago Closed 17 years ago

Crash [@ nsHTMLFramesetFrame::Reflow] with <frameset> in <msub>

Categories

(Core :: MathML, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(3 files, 2 obsolete files)

Loading this testcase crashes Firefox [@ nsHTMLFramesetFrame::Reflow].  Firefox appears to deref null.
The problem is that MathML frames does not implement the reflow protocol
(WillReflow/Reflow/DidReflow) properly. This results in child frames being
reflowed twice with NS_FRAME_FIRST_REFLOW set, which causes the crash in
this case because nsHTMLFramesetFrame depends on NS_FRAME_FIRST_REFLOW
only being set once.
Assignee: rbs → mats.palmgren
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
Attached patch Patch rev. 1 (obsolete) — Splinter Review
This patch adds the missing WillReflow/DidReflow calls under layout/mathml/.
Attachment #250866 - Flags: review?(rbs)
>The problem is that MathML frames does not implement the reflow protocol
>(WillReflow/Reflow/DidReflow) properly.

The WillReflow/DidReflow calls are already there (unless by bug/omission). To see, try tracing nsHTMLContainerFrame::ReflowChild (which is a helper that nsMathMLContainerFrame::ReflowChild also uses) and nsMathMLContainerFrame::FinalizeReflow. So the extra calls you are adding don't seem necessary.
To be precise, the DidReflow ultimately happens in nsHTMLContainerFrame::FinishReflowChild -- the helper that nsMathMLContainerFrame::Place() calls when the reflow of MathML frames is really finished (i.e., when we finally know where to place them and the argument aPlaceOrigin is set -- it can be a delayed process -- with Place() called more than once with aPlaceOrigin false, e.g., when we are trying to get the size needed for stretching -- in a kind of two-way dependency).

[BTW, is the bug happening outside the reflow branch? As the MathML code hasn't yet been fully migrated, some things might be in flux.]
Attached patch Patch rev. 2 (obsolete) — Splinter Review
This patch is the same as rev. 1 but without the WillReflow() call.

(In reply to comment #5)
> The WillReflow/DidReflow calls are already there (unless by bug/omission).

You're right about WillReflow(). Sorry, should have checked more carefully
before adding that at the last minute...

The DidReflow() calls are missing though, when errors occur, see for
example the two marked blocks in the URL in comment 2 (which is why we
crash on the testcase here incidentally) - we never reach the
FinishReflowChild() call at the end that would do the DidReflow() call.

(In reply to comment #6)
> To be precise, the DidReflow ultimately happens in
> nsHTMLContainerFrame::FinishReflowChild 

Sure, *if* we reach it. If not then it's taken care of by the
hunk after the call to Place() (note the NS_FRAME_IN_REFLOW test).

It's not reached for sure if one of the ReflowChild() calls fails
in the other three hunks. There you have a number of children that
has been successfully reflowed that needs a DidReflow() before
returning.

> [BTW, is the bug happening outside the reflow branch?]

Yes, this crash also happens on the 1.8 branch for example.
Attachment #250866 - Attachment is obsolete: true
Attachment #251591 - Flags: review?(rbs)
Attachment #250866 - Flags: review?(rbs)
Flags: blocking1.9?
Flags: blocking1.8.1.3?
Blocks: 370884
mats, I didn't forget you. I made another conference trip and came back just as the semester was beginning and getting in full swing here. But I can get back to my scary bug list now.

I have been wondering whether there isn't any other elegant solution to this bug. But nothing has flashed to my mind so far. I wonder though if @@ -461,24 +461,36 @@ is necessary unconditionally. Looks like it has to be inside the next |if (NS_MATHML_HAS_ERROR(mPresentationData.flags))|.

Also, there probably needs to be a little helper function since the same code is repeated.
(In reply to comment #8)
> Looks like it has to be inside the
> next |if (NS_MATHML_HAS_ERROR(mPresentationData.flags))|.

I think that could miss some cases. FinishReflowChild() is not called
when aPlaceOrigin is false. A third case is if the Place() call fails,
there are a couple of those (where ReflowError() is not called), e.g.
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/mathml/base/src/nsMathMLmsubFrame.cpp&rev=1.41&root=/cvsroot&mark=127#115

I think this would cover it though:
  rv = Place(...);
  if (!placeOrigin ||
      NS_MATHML_HAS_ERROR(mPresentationData.flags) ||
      NS_FAILED(rv))


> Also, there probably needs to be a little helper function

Sure, the last three at least, how about:
nsMathMLContainerFrame::DidReflowSomeChildren(nsIFrame* aFirst,
                                              nsIFrame* aStop)
 // call DidReflow on aFirst and all its siblings up to, but
 // not including, aStop. aStop may be null.


BTW, do you mind if I add assertions where the QI fails, like
the one above, it seems like that should not happen?
Let's go for this (i.e., let's make it to work this way)
  rv = Place(...);
  if (NS_MATHML_HAS_ERROR(mPresentationData.flags) ||
      NS_FAILED(rv))

(Otherwise, it would mean that the DidReflow() can be called twice -- which would then beg the question why bother with the conditional checks at the other call sites.)

The slightly shorter
nsMathMLContainerFrame::DidReflowChildren(nsIFrame* aFirst,
                                          nsIFrame* aStop=null)
is okay and equally clear (with aStop=null meaning all).

> BTW, do you mind if I add assertions where the QI fails, like
> the one above, it seems like that should not happen?

Yeah the QI shouldn't fail. So you can indeed assert there.
Attached patch Patch rev. 3Splinter Review
(In reply to comment #10)
> Otherwise, it would mean that the DidReflow() can be called twice

Ok, this patch should fix that.

> Yeah the QI shouldn't fail.

I eliminated the QI by changing the param type to nsMathMLContainerFrame*.
nsIFrame* + QI to nsIMathMLFrame* is more generic, but it seems like
overkill for these methods...
Attachment #251591 - Attachment is obsolete: true
Attachment #257004 - Flags: review?(rbs)
Attachment #251591 - Flags: review?(rbs)
Comment on attachment 257004 [details] [diff] [review]
Patch rev. 3

r=rbs
Attachment #257004 - Flags: review?(rbs) → review+
Attachment #257004 - Flags: superreview?(bzbarsky)
Mats, I'm not going to be able to review that in any sort of expeditious manner.  Most likely, I won't be able to review it at all until mid-April.  :(
You can change the param to be nsMathMLFrame*, BTW.

Patch looks safe to me. I can give you the r+sr= as well -- at least that will redeem me from the time you have been on hold.
Attachment #257004 - Flags: superreview?(bzbarsky) → superreview?(rbs)
Comment on attachment 257004 [details] [diff] [review]
Patch rev. 3

r+sr=rbs
Attachment #257004 - Flags: superreview?(rbs) → superreview+
(In reply to comment #14)
> You can change the param to be nsMathMLFrame*, BTW.

In that case I would have to QI to nsIFrame* again?
nsMathMLmsupFrame.cpp:135: error: ‘class nsMathMLFrame’ has no member named ‘GetFirstChild’

Given that PlaceSuperScript(), PlaceSubSupScript() and PlaceSubScript()
operates on child frames I think nsMathMLContainerFrame* makes sense.
Comment on attachment 257004 [details] [diff] [review]
Patch rev. 3

Checked in to trunk at 2007-03-02 02:55 PST.

-> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Flags: blocking1.8.0.11?
Flags: blocking1.9?
Flags: in-testsuite?
Depends on: 372483
Depends on: 373472
Why is this a branch blocker? Does not appear to be a security problem and the fix caused at least two regression bugs (fixed, but not nominated for the branch). Is this a high-frequency crash or test blocker?
1 regression.  Bug 373472 occurs also before this bug, but this patch
allowed us to see it more clearly because of the added assertion.

I agree it's likely not a security problem and I doubt it's a high-frequency
crash. I don't know whether it's a test blocker.  Jesse?
This crash came up a lot in my testing on trunk.  But I don't really test branch, so it wouldn't bother me if this were not fixed on branch.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
See also bug 375562, a similar crash that still happens on trunk.
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsHTMLFramesetFrame::Reflow]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: