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

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
Layout: Misc Code
P2
normal
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Simon Fraser, Assigned: bz)

Tracking

Trunk
mozilla1.8beta4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

Comment 2

13 years ago
I see this in Camino. No XUL popups there  :)
What's a page that triggers the warning in Camino?
Simon, what's a page that shows this warning in Camino?
(Reporter)

Comment 5

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

Comment 6

13 years ago
I'm also seeing a ton of these clicking around on http://arstechnica.com/
Created attachment 188905 [details] [diff] [review]
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).
Assignee: roc → bzbarsky
Status: NEW → ASSIGNED
Attachment #188905 - Flags: superreview?(dbaron)
Attachment #188905 - Flags: review?(dbaron)
Priority: -- → P2
Summary: Remove or fix warning: Deleting out of flow without tearing down placeholder relationship → [FIX]Remove or fix warning: Deleting out of flow without tearing down placeholder relationship
Target Milestone: --- → mozilla1.8beta4
So why shouldn't this happen for the other caller of ClearPlaceholderFrameMap too?
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...
Created attachment 189704 [details] [diff] [review]
With consistent behavior all around
Attachment #188905 - Attachment is obsolete: true
Attachment #189704 - Flags: superreview?(dbaron)
Attachment #189704 - Flags: review?(dbaron)
Attachment #188905 - Attachment is obsolete: false
Attachment #188905 - Flags: superreview?(dbaron)
Attachment #188905 - Flags: review?(dbaron)
Attachment #188905 - Attachment is obsolete: true
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.
Attachment #189704 - Flags: superreview?(dbaron)
Attachment #189704 - Flags: superreview+
Attachment #189704 - Flags: review?(dbaron)
Attachment #189704 - Flags: review+
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.
Attachment #189704 - Flags: approval1.8b4?
Created attachment 189716 [details] [diff] [review]
Patch to make this a real assert

This part is 1.9 material, imo.
Attachment #189716 - Flags: superreview?(dbaron)
Attachment #189716 - Flags: review?(dbaron)
Summary: [FIX]Remove or fix warning: Deleting out of flow without tearing down placeholder relationship → [FIXr]Remove or fix warning: Deleting out of flow without tearing down placeholder relationship

Updated

12 years ago
Attachment #189704 - Flags: approval1.8b4? → approval1.8b4+
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.)
Attachment #189716 - Flags: superreview?(dbaron)
Attachment #189716 - Flags: superreview+
Attachment #189716 - Flags: review?(dbaron)
Attachment #189716 - Flags: review+
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...
Created attachment 192824 [details] [diff] [review]
Updated to review comments
Attachment #189716 - Attachment is obsolete: true
Fixed.  Note that the first patch was checked in for 1.8, while the second was
only checked in for 1.9.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.