494283 - "ASSERTION: Dead placeholder in placeholder map" and more badness with abs pos, table root

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that shows that part of the problem is not related to the root element at all

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....

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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...

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Jesse Ruderman]

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

Comment 9

[Jesse Ruderman]

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/c3f21caf76f8

Jesse, I assume we should take this on branches too, right?

Comment 16

[Samuel Sidler (old account; do not CC)]

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

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 387711 [details] [diff] [review]
Proposed fix

Yeah, this applies fine to all the branches.

Comment 18

[Samuel Sidler (old account; do not CC)]

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 22

[Samuel Sidler (old account; do not CC)]

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

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 24

[Mats Palmgren (inactive)]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/00a42cd24f09