297850 - [FIXr]Remove or fix warning: Deleting out of flow without tearing down placeholder relationship

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Simon Fraser [no longer active]]

This warning shows up so frequently when loading pages that it seems worthless:

WARNING: Deleting out of flow without tearing down placeholder relationship,
file ../../../../mozilla/layout/generic/nsFrame.cpp, line 641

Can we just remove it, or fix whatever issue it's trying to indicate?

Comment 1

[Boris Zbarsky [:bzbarsky]]

In almost all cases when I've run into this, it's had to do with XUL popups. 
And yes, it's indicating a possible problem -- any time this happens we have a
risk of dereferencing garbage memory if someone tries to us the placeholder for
somethign after the out-of-flow dies....

The problem, I bet, is that we have no clean way to determine whether a frame is
out of flow short of listing manually all the conditions that could cause that
to happen... and that the list is incomplete in one or more places.

Comment 2

[Simon Fraser [no longer active]]

I see this in Camino. No XUL popups there  :)

Comment 3

[Boris Zbarsky [:bzbarsky]]

What's a page that triggers the warning in Camino?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Simon, what's a page that shows this warning in Camino?

Comment 5

[Simon Fraser [no longer active]]

http://www.smfr.org
The warning is printed when closing the tab or window showing the page.

While we're here, this one is also pretty chatty:
WARNING: Couldn't add reflow command, so splitting.

Comment 6

[Simon Fraser [no longer active]]

I'm also seeing a ton of these clicking around on http://arstechnica.com/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

So I was wrong; this happens any time a page with out-of-flows is torn down,
since in that case we don't do the whole DeletingFrameSubtree thing.

The patch basically does the following:

1)  At teardown, clear all refs from placeholders to their out of flows
2)  Clear out the placeholder map _before_ destroying the frame tree so out of
    flows don't find their placeholders in nsFrame::Destroy.
3)  Remove a comment that no longer corresponds to reality (I left the
    null-check, since it makes UnregisterPlaceholderFrame() safe to do no
matter
    what).

Comment 8

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

So why shouldn't this happen for the other caller of ClearPlaceholderFrameMap too?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Which part?  The other caller already calls ClearPlaceholderFrameMap before
destroying the frame tree.  Do you mean that it should also clear out the
pointers from placeholders to out-of-flows?  That might not be a bad idea...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: With consistent behavior all around

Comment 11

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

Comment on attachment 189704 [details] [diff] [review]
With consistent behavior all around

Yeah, with the previous patch it seemed like the other caller would trigger the
assertions that this bug is about.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 189704 [details] [diff] [review]
With consistent behavior all around

Requesting approval for this fix to document teardown order to keep it from
triggering warnings.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to make this a real assert

This part is 1.9 material, imo.

Comment 14

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

Comment on attachment 189716 [details] [diff] [review]
Patch to make this a real assert

Even though this is debug-only, I'd prefer if you still checked
NS_FRAME_OUT_OF_FLOW.

I'm a little skeptical about whether we really want to do this; we spent a lot
of time fighting reregistration edge cases for the primary frame map; then
again, hopefully we don't do frame construction in the middle of frame
destruction.  (The only registration point here is frame construction, I
assume.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Yes, the only registration here should be frame construction.  And yes, if we're
doing that in frame destruction I think we have a major problem...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to review comments

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed.  Note that the first patch was checked in for 1.8, while the second was
only checked in for 1.9.