This is a breakout bug from bug 317544. Once attachment 207573 [details] [diff] [review] is checked in I wanted to remove the CSS rule at the top of mathml.css that prevents abosolute positioning. This is because it does not protect against the same thing being done using XBL. However once I do that I start crashing again. Steps to reproduce: 1. Apply attachment 207573 [details] [diff] [review] and remove the top rule in mathml.css 2. Go to http://golem.ph.utexas.edu/~distler/blog/archives/000539.html 3. Start StirDOM 4. Use parameters 187,217,44,181 Stack: gklayout.dll!nsIFrame::GetParent() Line 652 + 0xa C++ gklayout.dll!GetNearestContainingBlock(nsIFrame * aFrame=0x00000000, nsMargin & aContentArea={...}) Line 660 + 0x8 C++ gklayout.dll!nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext * aPresContext=0x03104ca8, const nsHTMLReflowState * cbrs=0x00129b10, int containingBlockWidth=0x00002d00, int containingBlockHeight=0x00003dad) Line 1049 + 0xd C++ gklayout.dll!nsHTMLReflowState::InitConstraints(nsPresContext * aPresContext=0x03104ca8, int aContainingBlockWidth=0x00002d00, int aContainingBlockHeight=0x00003dad, nsMargin * aBorder=0x00000000, nsMargin * aPadding=0x00000000) Line 1965 C++ gklayout.dll!nsHTMLReflowState::Init(nsPresContext * aPresContext=0x03104ca8, int aContainingBlockWidth=0x00002d00, int aContainingBlockHeight=0x00003dad, nsMargin * aBorder=0x00000000, nsMargin * aPadding=0x00000000) Line 343 C++ gklayout.dll!nsHTMLReflowState::nsHTMLReflowState(nsPresContext * aPresContext=0x03104ca8, const nsHTMLReflowState & aParentReflowState={...}, nsIFrame * aFrame=0x0378c344, const nsSize & aAvailableSpace={...}, int aContainingBlockWidth=0x00002d00, int aContainingBlockHeight=0x00003dad, nsReflowReason aReason=eReflowReason_Resize) Line 315 C++ > gklayout.dll!nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame * aDelegatingFrame=0x0354ee30, nsPresContext * aPresContext=0x03104ca8, const nsHTMLReflowState & aReflowState={...}, int aContainingBlockWidth=0x00002d00, int aContainingBlockHeight=0x00003dad, nsIFrame * aKidFrame=0x0378c344, nsReflowReason aReason=eReflowReason_Resize, unsigned int & aStatus=0x00000000) Line 521 C++ gklayout.dll!nsAbsoluteContainingBlock::Reflow(nsIFrame * aDelegatingFrame=0x0354ee30, nsPresContext * aPresContext=0x03104ca8, const nsHTMLReflowState & aReflowState={...}, int aContainingBlockWidth=0x00002d00, int aContainingBlockHeight=0x00003dad, nsRect * aChildBounds=0x00129e18, int aForceReflow=0x00000001, int aCBWidthChanged=0x00000001, int aCBHeightChanged=0x00000001) Line 208 C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 1073 + 0x41 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000000, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000001, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012a7fc) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012a7fc, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000001, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000000, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012b43c) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012b43c, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000000, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000001, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012c07c) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012c07c, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000000, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000001, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012ccbc) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012ccbc, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000001, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000000, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012d8fc) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012d8fc, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsBlockReflowContext::ReflowBlock(const nsRect & aSpace={...}, int aApplyTopMargin=0x00000001, nsCollapsingMargin & aPrevMargin={...}, int aClearance=0x00000000, int aIsAdjacentWithTop=0x00000000, nsMargin & aComputedOffsets={...}, nsHTMLReflowState & aFrameRS={...}, unsigned int & aFrameReflowStatus=0x00000000) Line 605 + 0x2a C++ gklayout.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012e53c) Line 3455 + 0x3f C++ gklayout.dll!nsBlockFrame::ReflowLine(nsBlockReflowState & aState={...}, nsLineList_iterator aLine={...}, int * aKeepReflowGoing=0x0012e53c, int aDamageDirtyArea=0x00000001) Line 2617 + 0x1b C++ gklayout.dll!nsBlockFrame::ReflowDirtyLines(nsBlockReflowState & aState={...}, int aTryPull=0x00000001) Line 2269 + 0x1f C++ gklayout.dll!nsBlockFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aMetrics={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 902 + 0x11 C++ gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x0354e38c, nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000000, unsigned int & aStatus=0x00000000) Line 891 + 0x1f C++ gklayout.dll!CanvasFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 524 C++ gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x036c8d48, nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000003, unsigned int & aStatus=0x00000000) Line 891 + 0x1f C++ gklayout.dll!nsHTMLScrollFrame::ReflowScrolledFrame(const ScrollReflowState & aState={...}, int aAssumeHScroll=0x00000000, int aAssumeVScroll=0x00000001, nsHTMLReflowMetrics * aMetrics=0x0012ee3c, int aFirstPass=0x00000001) Line 513 + 0x36 C++ gklayout.dll!nsHTMLScrollFrame::ReflowContents(ScrollReflowState * aState=0x0012efa4, const nsHTMLReflowMetrics & aDesiredSize={...}) Line 583 + 0x1b C++ gklayout.dll!nsHTMLScrollFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 790 + 0x13 C++ gklayout.dll!nsContainerFrame::ReflowChild(nsIFrame * aKidFrame=0x036c8e94, nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, int aX=0x00000000, int aY=0x00000000, unsigned int aFlags=0x00000000, unsigned int & aStatus=0x00000000) Line 891 + 0x1f C++ gklayout.dll!ViewportFrame::Reflow(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsHTMLReflowState & aReflowState={...}, unsigned int & aStatus=0x00000000) Line 229 + 0x2b C++ gklayout.dll!IncrementalReflow::Dispatch(nsPresContext * aPresContext=0x03104ca8, nsHTMLReflowMetrics & aDesiredSize={...}, const nsSize & aMaxSize={...}, nsIRenderingContext & aRendContext={...}) Line 857 C++ gklayout.dll!PresShell::ProcessReflowCommands(int aInterruptible=0x00000001) Line 6473 C++ gklayout.dll!PresShell::WillPaint() Line 6151 C++

I'll attach the framedump that i'm referring to below. What happens is that we in nsHTMLReflowState::InitAbsoluteConstraints tries to get the placeholder for the frame 0378c344, however it comes back as null which cause us to crash. That frame belongs to a <div> element (036fa5e0) whos parent is an <msub> (0370ABF8). Here the fun begins since this msub has two separate frames pointing to it. One at 0375B148 and one at 03789CE0. The first one has no child frames, and the second has a single placeholder frame.

To add to what Jonas said, there are also two abs pos frames that both point to 036fa5e0 as the mContent. The placeholder of one of those two is the child of the 03789CE0 frame (for msub). The other abs pos frame has no placeholder, hence we crash. So it really looks like we failed to tear down a piece of the frame tree somewhere. Or something.

is this a safe-ish null deref, or does comment 1 and 3 mean this could be worse?

It's probably worse.

This fix the problem by checking the style of the frame _before_ it is created -- in the same way that we do for display:none. However, the cost per page now is the depth of the tree times the number of floated and positioned elements on the page. This hits all pages (not just pages with MathML) -- and is more expensive that the previous code that was pushing a null containing block and was walking up pass mathml frames when looking up a float containing block. This former approach was not working for content subsequently appended inside MathML (thus this crash). Assuming that the depth of the tree is usually about 15 or less, and that most webpages have 0, 1, 2 or 3 floats (and definitely much less than 10), the patch is worthy of consideration, as it fixes bug 348415 as well. As for the assertion in nsMathMLChar.cpp: NS_ASSERTION(mStyleContext, "chars shoud always have style context"); it turned out to be triggered by this debugging call in nsMathMLTokenFrame: fm->DebugVerifyStyleTree(parentFrame ? parentFrame : this); What happens is that StirDOM puts <mi> as a child of <mo> <mo> <mi> and when DebugVerifyStyleTree() is called in the parent, i.e. <mo>, this <mo> has not yet received its SetInitialChildList() to setup its own nsMathMLChar. Fortunately, I could just comment the whole thing out because there is another re-resolve sweep through the <math> subtree anyway -- to setup all the potential style data needed for scripting elements. So the style change needed by <mi> will happen as a side-effect.

> This former approach was not working for content subsequently appended inside > MathML So maybe what we should _really_ do is fix GetFloatContainingBlock() to work the same way that the "construct it all at once" codepath works? I believe we have other bugs on mismatches between the two, btw... > Assuming that the depth of the tree is usually about 15 or less Sadly, this isn't a good assumption. On typical "commercial" top100 sites (fox news, cnn, mapquest, etc, etc) depths of 30-40 are common; more than that happens reasonably often. Below 20 pretty much doesn't happen. Consider that each level of table nesting automatically gives you a depth of 6 in the frame tree (table-outer, table, row-group, row, cell, cell-inner)... > and that most webpages have 0, 1, 2 or 3 floats (and definitely much less > than 10) Again, not a great assumption. All left and right aligned tables and images are floats. Having a dozen or so per page is common; many "long" pages have hundreds to thousands. More accurate numbers could be produced with bc's spider, of course....

Some pages do layout primarily using floats; having thousands is probably not too unusual.

Another idea crossed my mind for a cheaper fix. We can have an "insideMathML" flag that is inherited when states are pushed, or set when a MathML frame is created. That could substantially reduce the number of walks. Where we have to be careful is to ensure that everything is in sync. But it seems to me that the following steps might do the trick to alleviate your concerns about the cost: with the FrameConstructorState: - if the flag is still unset, walk up the tree and set it when the _first_ float is created. - inherit the flag when pushing the state. - set the flag when a MathML frame is created.

I guess my real question is what we're trying to accomplish, exactly. Are we trying to say that a float inside a block inside mathml should not float? Or that if a float is appended dynamically as a child of mathml it should not float? If the former, why? If the latter, we already do a walk up the frame tree in GetFloatContainingBlock() in that case, so I stand by comment 7.

I think it is okay to have any reliable and long-lasting fix for these float-related crashes. If the floats are making things fragile, then I don't mind if they are stopped anywhere within <math>...</math>. In fact, I am happy to stop them so that we can put the matter behind us and don't have to worry about them anymore. It is not that we are breaking the MathML spec because the spec does not expect us to support floats. Otherwise, trying to support them by all means is as if we are fighting the wrong fight (as far as MathML is concerned). Besides, they won't be portable. To put it another way, the fix that is not costly and that does not require sprinkling the logic all over the frame constructor code is fine by me.

Well, the point is that changing GetFloatContainingBlock() would not prevent floats inside a block inside mathml. If we _do_ need to prevent such floats then we need another solution. If not, then fixing GetFloatContainingBlock() should work fine.

At this point, preventing floats altogether is okay and it might save us from unnecessary worries down the track. The approach I outlined in comment 9 seems reasonnable for that, although there is no patch yet to show for it. But once I get a patch, I will submit it (or rather, comment on any difficulty encountered that prevented such a patch...) to gather further feedback.

I guess my point is that preventing only direct child floats is _easy_. If that's good enough, we should do it.

That didn't prove good enough. Indeed we have a rule in mathml.css that should have done that (for positioning): -/* MathML doesn't permit positioning */ -*, * > *|* { - position: static !important; -} - But it still crashes due to positioning (as the title of the bug says). Whereas, with the (costly) patch I attached, I left StirDOM running overnight and it did not crash.

> Indeed we have a rule in mathml.css that should have done that (for > positioning) That doesn't do the same thing as what I'm proposing. I'm proposing that any time you have a mathml frame that has a floating descendant whose containing block is not a descendant of the same mathml frame we don't let the float float. Similar condition for positioning (and I'm not sure why you're disabling position:relative, btw). That said, the positioning case might be a little more difficult than floats, but not by much. Certainly not if you don't care about fixed positioning and just need to prevent absolute positioning.

> That didn't prove good enough. Indeed we have a rule in mathml.css that should > have done that (for positioning) One more thing. Isn't this bug about us crashing when that rule is _removed_? In any case, how about I post a patch tomorrow evening? I'd appreciate whoever has a StirWhatever setup to test MathML testing it.

Yeah, feel free to submit a patch. I think there is a crash even with a fresh pull from the trunk. My setup is this. I created a link called StirDOM on the personal bar, and edited its properties to copy-paste the StirDOM URL there (as you will do with a bookmarket). I have put the StirDOM (1.8)'s javascript:URL in the URL field of the bug for you to copy. Then, to experiment your patch: 1. Visit http://golem.ph.utexas.edu/~distler/blog/archives/000539.html 2. Click on your StirDOM link on the personal bar 3. On its prompt, fill-in the parameters 187,217,44,181

Iteration on the previous patch to only hit pages with actual MathML. This doesn't implement the trickeries I indicated in comment 9. But it is simple and it effectively addresses the cost concerns by only hitting pages with actual MathML in a way that is not invasive to the frame constructor. The idea here is to use a single flag on the frame constructor itself. The first ever created MathML frame sets the flag, which can then be queried later on to decide whether or not to walk. I don't think there can be any simpler and yet comprehensive solution than this.

I don't like this patch for the same reasons I don't like the original ones. It's heavy-handed and doesn't actually enforce the invariants we want to enforce as far as I can see. By the way, running stirdom without any of these patches on trunk asserts like crazy (chars without style contexts, bogus frame trees because a block is a child of an inline(!) which is the direct child of an nsMathMLmrowFrame, that sort of thing).

Someone else will need to test this. I can't reproduce this bug at all (followed steps in comment 0, ran for 30 minutes, no crashes).

Let me give your patch a try to see how it goes at my end.

I am observing that your patch does the trick. Curiously however, after about 2.5 hours or so, Firefox automatically shuts down with a non-zero exit code -- see the output below. Perhaps jesse can shed some light on this? Is this how StirDOM is supposed to end? Asumming this issue is not serious, I am fine with your patch. I still intend to include the other bits of my patch that eliminated the assertion about chars without style contexts -- it is a bogus assertion as I indicated earlier, and there are a couple of IsFrameOfType(eMathML) that were overlooked. Just for clarification -- my patch didn't have invariants to worry about, as those frames did not exist since they were not created. But your patch is more light-weight, being targeted at more deep pages, although that might not be an issue with most pages with mathml that I have seen. Also, BTW, for future improvements, there are numerous calls to GetAbsoluteContainingBlock()/GetFloatContainingBlock() to setup the state for _all_ frames. Their cummulative cost may be negating the light-weight benefit. Seems they could be setup only when a floated or positioned frame that matters is actually encountered (a bit like my patch attempted), but let's leave that for some other bug. Here is the output of the debugger when Firefox shuts down: The thread 'Win32 Thread' (0x16dc) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xe3c) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xc80) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x156c) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x858) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x1184) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x10dc) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xa70) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x204) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x118c) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x139c) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xb64) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x172c) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x1174) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xb98) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xd28) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x1638) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x70) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xd98) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x730) has exited with code 0 (0x0). The thread 'Win32 Thread' (0xb10) has exited with code 0 (0x0). The thread 'Win32 Thread' (0x1324) has exited with code 3 (0x3). The thread 'Win32 Thread' (0xbc) has exited with code 3 (0x3). The thread 'Win32 Thread' (0x634) has exited with code 3 (0x3). The thread 'Win32 Thread' (0x1214) has exited with code 3 (0x3). The thread 'Win32 Thread' (0x1358) has exited with code 3 (0x3). The thread 'Win32 Thread' (0x1240) has exited with code 3 (0x3). The thread 'Win32 Thread' (0xce0) has exited with code 3 (0x3). The thread 'Win32 Thread' (0x580) has exited with code 3 (0x3). The program '[4904] firefox.exe: Native' has exited with code 3 (0x3). Googling for "has exited with code 3 (0x3)" suggests an access violation -- which might be a problem in StirDOM? Or might be another Gecko bug that could range from a simple array[out-of-bound] to a null pointer deference). I tried testing several times, and it invariably shuts down without breaking into the debugger, making it hard to tell was is going on, and so I could not get any further insight.

Yeah, there's been some thought on eliminating the tracking of float/abs pos containing blocks in the state... That might happen on trunk. Does this testcase also test relative and fixed positioning? Note that my patch only affects absolute positioning (unlike both rbs's patch and the CSS currently in the tree). If the testcase doesn't test fixed positioning, is there one that does? I'd like that to get tested before we decide my patch is OK.

I am working in bug 353876 to get rid of the assertions about chars without style contexts. Apart from the uncertainty with relative positioning, it remains puzzling that we have this curious 2.5h time limit. Is it something on my machine? Can somebody else reproduce? Is is exploitable? I was prompted to work on this bug upon seeing comment #5 that suggested that security risks could be worse with things as is. That's why I am not yet retracting my bullet-proofing patch in case there is no way out in the grand scheme of things (considering the 2.5h time limit of bz's patch and that its light-weight benefit may be outweighed by the numerous calls, and that we don't see much MathML pages with numerous floats in today's web).

OK, so applying fixed positioning to actual MathML nodes crashes; Martijn can point to the talkback IDs. At least in some cases (fixed-pos <math> element, which I think should work) it's clearly a matter of trying to do block layout without a space manager; in other cases (fixed-pos msub) you get things like http://talkback-public.mozilla.org/search/start.jsp?search=2&type=iid&id=TB23627925K which look bad. I'll look into the <math> issue and think about what we can do to sanely prevent fixed-positioning of mathml nodes. Maybe we need to start pushing a fixed containing block too...

Comment on attachment 238643 [details] [diff] [review] The approach I think we should take OK, apparently fixed positioning is happy now, so I think we should try this. roc, what do you think of the ConstructBlock change especially? That's the only part that at all worries me here in terms of effect on non-MathML stuff...

Comment on attachment 238643 [details] [diff] [review] The approach I think we should take The patch needs to be sync'ed to the trunk. I wanted to see if there has been a change to to 2.5h time limit. But applying the patch conflicted with the trunk.

It is crashing. Your may need these two IsFrameOfType(): https://bugzilla.mozilla.org/attachment.cgi?id=238624&action=diff#mozilla/layout/mathml/base/src/nsMathMLContainerFrame.h_sec2 I will put them in and test again. gklayout.dll!VerifyContextParent(nsPresContext * aPresContext=0x04725680, nsIFrame * aFrame=0x048779b0, nsStyleContext * aContext=0xdddddddd, nsStyleContext * aParentContext=0x00000000) Line 794 C++ gklayout.dll!VerifyStyleTree(nsPresContext * aPresContext=0x04725680, nsIFrame * aFrame=0x048779b0, nsStyleContext * aParentContext=0x00000000) Line 833 + 0x13 bytes C++ gklayout.dll!VerifyStyleTree(nsPresContext * aPresContext=0x04725680, nsIFrame * aFrame=0x04816128, nsStyleContext * aParentContext=0x05e32858) Line 851 + 0xf bytes C++ gklayout.dll!nsFrameManager::DebugVerifyStyleTree(nsIFrame * aFrame=0x04816128) Line 886 + 0x16 bytes C++ gklayout.dll!nsMathMLContainerFrame::PropagateScriptStyleFor(nsIFrame * aFrame=0x04800198, int aParentScriptLevel=0) Line 698 C++ gklayout.dll!nsMathMLContainerFrame::ReResolveScriptStyle(int aParentScriptLevel=0) Line 112 + 0x10 bytes C++ gklayout.dll!nsMathMLContainerFrame::ReLayoutChildren(nsIFrame * aParentFrame=0x04816128) Line 848 C++ > gklayout.dll!nsMathMLmathBlockFrame::RemoveFrame(nsIAtom * aListName=0x00b64d68, nsIFrame * aOldFrame=0x048779b0) Line 377 + 0x9 bytes C++ gklayout.dll!nsFrameManager::RemoveFrame(nsIFrame * aParentFrame=0x04816128, nsIAtom * aListName=0x00b64d68, nsIFrame * aOldFrame=0x048779b0) Line 682 C++ gklayout.dll!nsCSSFrameConstructor::ContentRemoved(nsIContent * aContainer=0x047c6868, nsIContent * aChild=0x047e0b88, int aIndexInContainer=4, int aInReinsertContent=0) Line 10087 + 0x1d bytes C++ gklayout.dll!PresShell::ContentRemoved(nsIDocument * aDocument=0x0471c0b0, nsIContent * aContainer=0x047c6868, nsIContent * aChild=0x047e0b88, int aIndexInContainer=4) Line 5407 C++ gklayout.dll!nsBindingManager::ContentRemoved(nsIDocument * aDocument=0x0471c0b0, nsIContent * aContainer=0x047c6868, nsIContent * aChild=0x047e0b88, int aIndexInContainer=4) Line 1311 + 0x5d bytes C++ gklayout.dll!nsNodeUtils::ContentRemoved(nsINode * aContainer=0x047c6868, nsIContent * aChild=0x047e0b88, int aIndexInContainer=4) Line 153 + 0x85 bytes C++ gklayout.dll!nsGenericElement::doRemoveChildAt(unsigned int aIndex=4, int aNotify=1, nsIContent * aKid=0x047e0b88, nsIContent * aParent=0x047c6868, nsIDocument * aDocument=0x0471c0b0, nsAttrAndChildArray & aChildArray={...}) Line 2417 + 0x11 bytes C++ gklayout.dll!nsGenericElement::RemoveChildAt(unsigned int aIndex=4, int aNotify=1) Line 2364 + 0x2a bytes C++ gklayout.dll!nsGenericElement::doReplaceOrInsertBefore(int aReplace=0, nsIDOMNode * aNewChild=0x047e0bc0, nsIDOMNode * aRefChild=0x00000000, nsIContent * aParent=0x047ba238, nsIDocument * aDocument=0x0471c0b0, nsIDOMNode * * aReturn=0x0012ddbc) Line 2926 + 0x13 bytes C++ gklayout.dll!nsGenericElement::InsertBefore(nsIDOMNode * aNewChild=0x047e0bc0, nsIDOMNode * aRefChild=0x00000000, nsIDOMNode * * aReturn=0x0012ddbc) Line 2535 + 0x20 bytes C++ gklayout.dll!nsXMLElement::InsertBefore(nsIDOMNode * newChild=0x047e0bc0, nsIDOMNode * refChild=0x00000000, nsIDOMNode * * _retval=0x0012ddbc) Line 63 + 0x18 bytes C++ gklayout.dll!nsGenericElement::AppendChild(nsIDOMNode * aNewChild=0x047e0bc0, nsIDOMNode * * aReturn=0x0012ddbc) Line 449 C++ gklayout.dll!nsXMLElement::AppendChild(nsIDOMNode * newChild=0x047e0bc0, nsIDOMNode * * _retval=0x0012ddbc) Line 63 + 0x14 bytes C++ xpcom_core.dll!XPTC_InvokeByIndex(nsISupports * that=0x00000012, unsigned int methodIndex=2, unsigned int paramCount=1236396, nsXPTCVariant * params=0x00000000) Line 102 C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=18) Line 2162 + 0x1e bytes C++ xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD) Line 2162 + 0x1e bytes C++ xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x03f9a680, JSObject * obj=0x03eb9d68, unsigned int argc=1, long * argv=0x06d99224, long * vp=0x0012e07c) Line 1455 + 0xe bytes C++ js3250.dll!js_Invoke(JSContext * cx=0x03f9a680, unsigned int argc=1, unsigned int flags=0) Line 1373 + 0x20 bytes C js3250.dll!js_Interpret(JSContext * cx=0x03f9a680, unsigned char * pc=0x054cb84a, long * result=0x0012eb34) Line 4113 + 0xf bytes C js3250.dll!js_Execute(JSContext * cx=0x03f9a680, JSObject * chain=0x07274550, JSScript * script=0x07289ee0, JSStackFrame * down=0x06d990f8, unsigned int flags=32, long * result=0x0012ecb4) Line 1618 + 0x13 bytes C js3250.dll!obj_eval(JSContext * cx=0x03f9a680, JSObject * obj=0x03f31008, unsigned int argc=1, long * argv=0x06d99178, long * rval=0x0012ecb4) Line 1357 + 0x1b bytes C js3250.dll!js_Invoke(JSContext * cx=0x03f9a680, unsigned int argc=1, unsigned int flags=0) Line 1373 + 0x20 bytes C js3250.dll!js_Interpret(JSContext * cx=0x03f9a680, unsigned char * pc=0x03526ecf, long * result=0x0012f83c) Line 4113 + 0xf bytes C js3250.dll!js_Invoke(JSContext * cx=0x03f9a680, unsigned int argc=1, unsigned int flags=2) Line 1392 + 0x13 bytes C js3250.dll!js_InternalInvoke(JSContext * cx=0x03f9a680, JSObject * obj=0x03f31008, long fval=120012752, unsigned int flags=0, unsigned int argc=1, long * argv=0x07132ee0, long * rval=0x0012f9b8) Line 1467 + 0x14 bytes C js3250.dll!JS_CallFunctionValue(JSContext * cx=0x03f9a680, JSObject * obj=0x03f31008, long fval=120012752, unsigned int argc=1, long * argv=0x07132ee0, long * rval=0x0012f9b8) Line 4419 + 0x1f bytes C gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x04122038, void * aScope=0x03f31008, void * aHandler=0x07273fd0, nsIArray * aargv=0x078af02c, nsIVariant * * arv=0x0012fa6c) Line 1745 + 0x24 bytes C++ gklayout.dll!nsGlobalWindow::RunTimeout(nsTimeout * aTimeout=0x078af1a0) Line 6564 + 0xab bytes C++ gklayout.dll!nsGlobalWindow::TimerCallback(nsITimer * aTimer=0x078af210, void * aClosure=0x078af1a0) Line 6892 C++ xpcom_core.dll!nsTimerImpl::Fire() Line 383 + 0x13 bytes C++ xpcom_core.dll!nsTimerEvent::Run() Line 458 C++ xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fbc4) Line 483 C++ xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00afa130, int mayWait=1) Line 225 + 0x16 bytes C++ gkwidget.dll!nsBaseAppShell::Run() Line 153 + 0xc bytes C++ tkitcmps.dll!nsAppStartup::Run() Line 171 + 0x1c bytes C++ xul.dll!XRE_main(int argc=1, char * * argv=0x00af74f8, const nsXREAppData * aAppData=0x004036b0) Line 2465 + 0x25 bytes C++ firefox.exe!main(int argc=1, char * * argv=0x00af74f8) Line 61 + 0x13 bytes C++ firefox.exe!__tmainCRTStartup() Line 586 + 0x19 bytes C firefox.exe!mainCRTStartup() Line 403 C kernel32.dll!7c816fd7() [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]

Er... That stack shows MathML triggering style reresolution while the frame tree is in an inconsistent state. Why exactly is it doing that? And can we make it NOT do that?

As far I can tell, the IsFrameOfType() is stopping the crash. I added them as I mentioned above, and relaunched StirDOM, it has now been running for 5h and is still going.

+10h and still cruising. No crash in sight.

> Er... That stack shows MathML triggering style reresolution while the frame > tree is in an inconsistent state. Why exactly is it doing that? And can we > make it NOT do that? Is there an assertion in place to catch that kind of bug? (cf bug 310985)

Comment 31 has become bug 355548, "MathML performs unsafe operations during frame removal".

> Is there an assertion in place to catch that kind of bug? No, but there should be. File a bug? If not, I'll try to tomorrow.

Comment on attachment 241265 [details] [diff] [review] Updated to tip This thing is not crashing anymore. It is cruising on. I need my laptop back... I have to stop it. I am happy to give my r=rbs, with the IsFrameOfType() that were overlooked as I indicated.

Checked in on trunk. We should bake, then think about the branches...

Filed bug 355975 on comment 34.

Branch landing may have to wait for bug 355993 to be fixed.

I should note that we can always decide to land on branch all _but_ the mathml.css changes... That should help with the XBL issues that worry us.

Comment on attachment 241459 [details] [diff] [review] With the IsFrameOfType changes a=mconnor on behalf of drivers for 1.8.0.9 and 1.8.1.1 checkin (from the look of things this is equally applicable to both branches, ignore me if I'm somehow wrong here)

This looks like it'll be seriously painful to merge to branch (have to merge multiple patches, for this and dependent bugs, in the right order, and the branch code is seriously different in some of those cases because other security patches were done differently for branch and trunk).

Fixed for 1.8.0.9, 1.8.1.

I meant 1.8.1.1.