Closed Bug 348492 Opened 18 years ago Closed 18 years ago

Frames not destroyed with <mmultiscripts> and <mtd>

Categories

(Core :: MathML, defect)

PowerPC
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: rbs)

References

Details

(4 keywords, Whiteboard: [sg:critical])

Attachments

(3 files, 1 obsolete file)

Upon loading the testcase:

###!!! ASSERTION: unexpected child list: 'PR_FALSE', file /Users/admin/trunk/mozilla/layout/tables/nsTableOuterFrame.cpp, line 243
(this assertion also fires in bug 348126, fwiw)

Upon reloading:

###!!! ASSERTION: Some frame destructors were not called: 'mFrameCount == 0', file /Users/admin/trunk/mozilla/layout/base/nsPresShell.cpp, line 625
(if you have the patch from bug 334514 that adds this assertion)

Crash dereferencing 0xdadadaf6
(if you use the crashing testcase, and you're using a debug build)
Whiteboard: [sg:critical]
(This bug shows up because the CSS rule to disable 'mtd' isn't being applied. As I explained in bug 347355 comment 33, this has been a blessing in disguise. With that CSS rule in effect, the bug doesn't show up.)

After much debugging, this bug turns out to be a case of mistaken identity. What is happening is that the content model moves from
1)
<mmultiscripts>
  <mtd/>

to 
2)
<mmultiscripts>
  <mtd/>
  <img/>

And here is the scenario that leads to the crash:

In 1), the lone <mtd/> causes the frame construction code to generate pseudo frames to make a complete table hierarchy.

In 2), when <img/> is appended, the frame construction code searches the frame that should handle the append. This rightly returns the MathML's mmultiscripts frame.

Now, here is where the drama unfolds (it is a bit technical here...). The frame construction code has different handlings depending on the 'type' of the frame. So it calls frame->GetType(), and for our particular context with frame=mmultiscripts, frame->GetType() returns a bogus 'outerTable', and because of this type, it passes the handling to frame->FirstChild(), that is, mmultiscripts->FirstChild(), which is the pseudo outerTable frame!

Then the unintended outerTable frame is given the task to append the <img/>. Since it can't do that, it therefore fires the assertion 
###!!! ASSERTION: unexpected child list: 'PR_FALSE'

The img frame is left unattached to the frame tree. And at tear down, it becomes a zombie -- we are back to what whas happening in bug 347355.

========================
On the whole, the crux of the matter boild down to why does mmultiscripts->GetType() return outerTable ?!?

The answer to that turns out to be a dangerous overload of GetType(). I include the function full below:

// For MathML, the 'type' will be used to determine the spacing between frames
// Subclasses can override this method to return a 'type' that will give
// them a particular spacing
nsIAtom*
nsMathMLContainerFrame::GetType() const
{
  // see if this is an embellished operator (mapped to 'Op' in TeX)
  if (mEmbellishData.coreFrame) {
    return mEmbellishData.coreFrame->GetType();
  }

  // if it has a prescribed base, just fetch the type from there
  if (mPresentationData.baseFrame) {
HERE>>HERE>>>>>>>>>>>>>>>>>>>>>>
    In the case of is bug, baseFrame is the outeTable!!!
>>>>>>>>>>>>>>>>>>>>>>
    return mPresentationData.baseFrame->GetType();
  }

  // everything else is treated as ordinary (mapped to 'Ord' in TeX)
  return nsMathMLAtoms::ordinaryMathMLFrame;
}

================
The fix will consist therefore in not overloading GetType() for internal MathML purposes! I will have to introduce another separate function. This might give a large patch, but I hope the explanation/necessity above will ease the review.
Attached patch patchSplinter Review
This patch prevents the crash in the testcase here. But in a sense, it is a continuation of bug 347355. It is needed to really nail down bug 347355 with C++.

As I hinted in my previous comment, it is a chnage of API and does not really change the functionality. The nsIFrame's GetType() is now left alone, and what it was overloaded for has been passed on to a newly added GetMathMLFrameType().
Attachment #233698 - Flags: superreview?(bzbarsky)
Attachment #233698 - Flags: review?(bzbarsky)
Comment on attachment 233698 [details] [diff] [review]
patch

>Index: layout/mathml/base/src/nsMathMLContainerFrame.cpp
>+    aFrame->QueryInterface(NS_GET_IID(nsIMathMLFrame), (void**)&mathMLFrame);

CallQueryInterface(aFrame, &mathMLFrame);

>Index: layout/mathml/base/src/nsMathMLContainerFrame.h
>+                                 eMathMLFrameType*    aMathMLFrameType = nsnull);

Document this argument, please.

>Index: layout/mathml/base/src/nsMathMLFrame.cpp
>@@ -179,11 +194,13 @@ nsMathMLFrame::GetPresentationDataFrom(n
>+      frame->QueryInterface(NS_GET_IID(nsIMathMLFrame), (void**)&mathMLFrame);

CallQI.

>Index: layout/mathml/base/src/nsMathMLFrame.h
>+      aFrame->QueryInterface(NS_GET_IID(nsIMathMLFrame), (void**)&mathMLFrame);

CallQI.

r+sr=bzbarsky
Attachment #233698 - Flags: superreview?(bzbarsky)
Attachment #233698 - Flags: superreview+
Attachment #233698 - Flags: review?(bzbarsky)
Attachment #233698 - Flags: review+
Checked in, with the suggested review changes.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
On FF2 I get "invalid markup" from the testcase and don't crash. Can we do without this fix on the old branches?
The reason it doesn't show up on the branch is because the CSS rule to disable 'mtd' is applied there, C.f. comment #3.

So the question boils down to whether it is good enough to have prevented the issue via that CSS (as opposed to having this C++ patch).
Given XBL, probably not....  Unless the MathML frame construction enforces that its nodes are really MathML-namespaced before application of XBL?
See bug 347355 comment 15 for more reasons not to rely on mathml.css rules for memory safety.
Per comments 8 through 10, requesting blocking on the branches.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Comment on attachment 233698 [details] [diff] [review]
patch

If security eyes are okay with this, I guess we can put it in the 1.8 branch.
Attachment #233698 - Flags: approval1.8.1.4?
Attachment #233698 - Flags: approval1.8.0.12?
Comment on attachment 233698 [details] [diff] [review]
patch

This patch doesn't appear to apply to the branch(es). Please provide a merged patch, and if significantly different it might need a re-review.
Attachment #233698 - Flags: approval1.8.1.4?
Attachment #233698 - Flags: approval1.8.1.4-
Attachment #233698 - Flags: approval1.8.0.12?
Attachment #233698 - Flags: approval1.8.0.12-
Attachment #262065 - Flags: approval1.8.1.4?
Attachment #262065 - Flags: approval1.8.0.12?
Please consider this instead. (After porting the earlier patch, I tested that it compiled and didn't crash the testcase as expected, but then forgot to "cvs update" before doing the "cvs diff". This new diff is the proper one in sync as it comes after the cvs update.)
Attachment #262065 - Attachment is obsolete: true
Attachment #262079 - Flags: approval1.8.1.4?
Attachment #262079 - Flags: approval1.8.0.12?
Attachment #262065 - Flags: approval1.8.1.4?
Attachment #262065 - Flags: approval1.8.0.12?
Comment on attachment 262079 [details] [diff] [review]
262065: patch in sync with the 1.8.1.x & 1.8.0.x branches

approved for 1.8.0.12 and 1.8.1.4, a=dveditz for release-drivers
Attachment #262079 - Flags: approval1.8.1.4?
Attachment #262079 - Flags: approval1.8.1.4+
Attachment #262079 - Flags: approval1.8.0.12?
Attachment #262079 - Flags: approval1.8.0.12+
Checked in the branches: fixed1.8.0.12, fixed1.8.1.4
v.fixed on 1.8 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.4) Gecko/20070515 Firefox/2.0.0.4, no crash with first testcase, but I also see the "invalid markup" that dveditz saw in comment #7 with both testcases.  

I am assuming the "invalid markup" is expected for those testcases?
v.fixed on 1.8.0 branch with 1.5.0.12rc2
Group: security
Flags: in-testsuite?
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.