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.
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?
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.
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).
13 years ago
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
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 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.
Created attachment 189716 [details] [diff] [review] Patch to make this a real assert This part is 1.9 material, imo.
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.)
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
Fixed. Note that the first patch was checked in for 1.8, while the second was only checked in for 1.9.