###!!! ASSERTION: Placeholder relationship should have been torn down; see comments in nsPlaceholderFrame.h: '!shell->FrameManager()->GetPlaceholderFrameFor(mOutOfFlowFrame)', file /Users/jruderman/central/layout/generic/nsPlaceholderFrame.cpp, line 132 ###!!! ASSERTION: Dead placeholder in placeholder map: 'entry->placeholderFrame->GetOutOfFlowFrame() != (void*)0xdddddddd', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 135 ###!!! ASSERTION: Some objects allocated with AllocateFrame were not freed: 'mFrameCount == 0', file /Users/jruderman/central/layout/base/nsPresShell.cpp, line 681 This is exploitable in the usual way. It's also really annoying, because the crashes happen all over the place.

The key here is that with an abs pos table it's possible for the placeholder to be destroyed before its out of flow (normally not so easy to do). This testcase triggers the first two asserts from comment 0. Interestingly, it does NOT trigger the third one....

Ah, but the difference is that the "Dead placeholder in placeholder map" assert only happens once in the testcase I just attached, and twice in Jesse's original testcase... Looking into that.

OK, so the key to that seconf assert is that the new address for that block (after the reframe) ends up exactly the same as the old one, as far as I can tell. The second "dead placeholder" assert comes from the nsFrameManager::RegisterPlaceholderFrame call after the frame reconstruct.

I'm not sure it's worth trying to figure out where the "objects not freed" assert is coming from until we have the other fixed...

So it looks like we're never calling the destructor on an nsTextFrame and some style contexts... The crashes come because we presumably also never clear the textrun on this frame, and then try to do something with it.

Oh, I see what happens to cause that last bit. We apparently never remove the dead placeholder from the placeholder map, because by the time we try to unregister it (when we destroy its out of flow) it has 0xdddddddd as the out-of-flow pointer, so doesn't match our passed-in out-of-flow. Then we happen to allocate the nsInlineFrame for the _moz_generated_content_before at the same memory location as the placeholder used to be (placeholders and inlines have the same size, so share the same buckets in the frame arena). Then when we tear down the presshell, we call nsFrameManager::Destroy, which calls nsFrameManager::ClearPlaceholderFrameMap, which calls SetOutOfFlowFrame(nsnull) on all the placeholders in the map. This is an inline method on nsPlaceholderFrame, and for placeholders: (gdb) p (void*)&mOutOfFlowFrame - (void*)this $39 = 56 while for inlines: (gdb) p (void*)&this->mFrames.mFirstChild - (void*)this $45 = 56 so this call basically nulls out mFrames.mFirstChild on the inline, which loses the pointer to the textframe child it has. Short-term, we could add an UnregisterPlaceholderFrame() call in nsPlaceholderFrame::Destroy, I think. That should at least make sure the dead placeholder is gone from the map. The current code guarantees it'll stick around if we get that assert, which is unfortunate. Longer-term we should think about how to fix this in general.... Making tables absolute containing blocks would help this particular testcase, but it seems like we should be able to hit the same issue with fixed-pos frames.

Ah, with fixed-pos frames we're saved by the fact that we don't destroy the viewport, so handle them via the out-of-flow stuff in DeletingFrameSubtree.

layout/generic/crashtests/323493-1.html triggers: ###!!! ASSERTION: Placeholder relationship should have been torn down; see comments in nsPlaceholderFrame.h: '!shell->FrameManager()->GetPlaceholderFrameFor(mOutOfFlowFrame)', file /Users/jruderman/central/layout/generic/nsPlaceholderFrame.cpp, line 132 ###!!! ASSERTION: Dead placeholder in placeholder map: 'entry->placeholderFrame->GetOutOfFlowFrame() != (void*)0xdddddddd', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 135 ###!!! ASSERTION: Registering a placeholder for a frame that already has a placeholder!: '!entry->placeholderFrame', file /Users/jruderman/central/layout/base/nsFrameManager.cpp, line 523

This interferes with fuzzing by corrupting memory and causing crashes all over the place (e.g. in GC). This prevents me from telling my fuzzing scripts to ignore this bug.

I'm not sure I'm up on the whole placeholder thing enough to tell whether my proposal in comment 6 makes sense...

OK. So right now, DeletingFrameSubtree walks through all the in-flow kids of the frame being deleted and puts any out-of-flow that's not a descendant of aRemovedFrame in the destroy queue. It then recurses into the kids of the out-of-flow, using that out-of-flow as the aRemovedFrame if it went into the destroy queue, and using our original aRemovedFrame otherwise. Then it destroys all the frames in the destroy queue, starting from the end. nsBlockFrame::Destroy destroys its absolute/floating frames before destroying its in-flow kids. So how can a placeholder get destroyed before its out-of-flow? If the out-of-flow (call it X) is in the destroy queue, the placeholder must be a descendant of something _later_ in the destroy queue. Since placing an out-of-flow in the destroy queue means that we're walking through its placeholder, that means the something that might have ended up in the destroy queue must already be there. So this case can't happen. If the out-of-flow (call it X) is not in the destroy queue, that means that the placeholder must be the descendant of an out-of-flow (call it Y) that is either in the destroy queue or will be destroyed earlier than X. And X must not be a descendant of Y. But if X is not a descendant of Y, and Y is in the destroy queue, then X would be in the destroy queue too. So we're left with the case of X and Y being out-of-flows, Y being an ancestor of the placeholder for X, Y not being an ancestor of X, and a common ancestor of X and Y being removed. And Y being destroyed before X. In the testcase attached here, this happens because both the table and the div are on the abs pos child list of the canvas, with the table before the div. I think that's the only way it can happen, though. If Y is out of flow and X is out of flow and Y is not an ancestor of X but is an ancestor of its placeholder, then the options are (modulo popup nastiness): 1) X is absolutely/fixed positioned and Y is floating. Since blockframe destroys abs pos kids before floats, we destroy X before Y (and we should comment that in nsBlockFrame::Destroy!). 2) X is absolutely positioned and Y is absolutely positioned and X is not an ancestor of Y. That's the case in Jesse's testcase. 3) X and Y are fixed positioned. We never remove the viewport frame, so there is no problem here; both will end up in the destroy queue. Thinking about this more, I think we should unregister the placeholder in its Destroy and be done with it (possibly with an assert if it hasn't been unregistered yet). In all codepaths other than this one it's supposed to be unregistered before being destroyed.

With this patch, the testcases will still assert (could take that out if desired), but no longer corrupt memory....

Comment on attachment 387711 [details] [diff] [review] Proposed fix r=dbaron, but we should keep a bug open on not hitting the assertion in this case as well... with ties back to this bug.

Fixing all frames to be able to be abs pos containing blocks (and in particular all frames on abs pos lists to be able to be so) will handle that. So we already have a bug on it...

Pushed http://hg.mozilla.org/mozilla-central/rev/c3f21caf76f8 Jesse, I assume we should take this on branches too, right?

Boris: Does this patch apply to 1.9.0 and 1.9.1? Can you request approval if so?

Comment on attachment 387711 [details] [diff] [review] Proposed fix Yeah, this applies fine to all the branches.

Comment on attachment 387711 [details] [diff] [review] Proposed fix Approved for 1.9.1.3 and 1.9.0.14. a=ss for release-drivers

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b3755cc5e6f0 Checking in layout/generic/nsPlaceholderFrame.cpp; /cvsroot/mozilla/layout/generic/nsPlaceholderFrame.cpp,v <-- nsPlaceholderFrame.cpp new revision: 3.125; previous revision: 3.124 done

Looking at a pre-fix 1.9.0 debug build, I do not see the assertions in it. Did they actually appear in 1.9.0?

I have the same issue in 1.9.1. With debug 1.9.1.2 (and now 1.9.1.3), I don't see the assertions. Is there any way to verify this fix on 1.9.0 and 1.9.1?

Jesse and dbaron: Can you help verify this? Should the assertions be seen on either the 1.9.0 or 1.9.1 branches?

Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.3pre) Gecko/20090824 Shiretoko/3.5.3pre ID:20090824172248 No assertions visible anymore.

crashtests: https://hg.mozilla.org/integration/mozilla-inbound/rev/00a42cd24f09