The default bug view has changed. See this FAQ.

Frames not destroyed with <mmultiscripts> and <mtd>

RESOLVED FIXED

Status

()

Core
MathML
--
critical
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: rbs)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
PowerPC
Mac OS X
crash, testcase, verified1.8.0.12, verified1.8.1.4
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.4 +
wanted1.8.1.x +
blocking1.8.0.12 +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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)
(Reporter)

Comment 1

11 years ago
Created attachment 233434 [details]
testcase that crashes on the second reload
(Reporter)

Comment 2

11 years ago
Created attachment 233435 [details]
testcase that does not crash
(Reporter)

Updated

11 years ago
Whiteboard: [sg:critical]
(Assignee)

Comment 3

11 years ago
(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.
(Assignee)

Comment 4

11 years ago
Created attachment 233698 [details] [diff] [review]
patch

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+
(Assignee)

Comment 6

11 years ago
Checked in, with the suggested review changes.
Status: NEW → RESOLVED
Last Resolved: 11 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?
(Assignee)

Comment 8

11 years ago
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?
(Reporter)

Comment 10

11 years ago
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+
(Assignee)

Comment 12

10 years ago
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-
(Assignee)

Comment 14

10 years ago
Created attachment 262065 [details] [diff] [review]
patch in sync with the 1.8.1.x 1.8.0.x branches
Attachment #262065 - Flags: approval1.8.1.4?
Attachment #262065 - Flags: approval1.8.0.12?
(Assignee)

Comment 15

10 years ago
Created attachment 262079 [details] [diff] [review]
262065: patch in sync with the 1.8.1.x & 1.8.0.x branches

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+
(Assignee)

Comment 17

10 years ago
Checked in the branches: fixed1.8.0.12, fixed1.8.1.4
Keywords: fixed1.8.0.12, fixed1.8.1.4

Comment 18

10 years ago
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?
Keywords: fixed1.8.1.4 → verified1.8.1.4

Comment 19

10 years ago
v.fixed on 1.8.0 branch with 1.5.0.12rc2
Keywords: fixed1.8.0.12 → verified1.8.0.12
Group: security
Flags: in-testsuite?
(Reporter)

Comment 20

9 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.