Saw this little gem that hasn't hit anybody because nobody calls the method now. PRBool nsFrameList::ReplaceAndDestroyFrame(nsIPresContext* aPresContext, nsIFrame* aParent, nsIFrame* aOldFrame, nsIFrame* aNewFrame) { NS_PRECONDITION(nsnull != aOldFrame, "null ptr"); NS_PRECONDITION(nsnull != aNewFrame, "null ptr"); if (ReplaceFrame(aParent, aOldFrame, aNewFrame)) { aNewFrame->Destroy(aPresContext); <-- destroying the new instead of the old return PR_TRUE; } return PR_FALSE; }

And while on this, could somebody fills me on what is happening to those frames that are being replaced? Are they hanging around ad infinitum or what? For reference, there is some code that is replacing a frame with another one in nsCSSFrameConstructor::CantRenderReplacedElement(...).

To clarify, I am not refering to the underlying arena. I was rather wondering why Destroy() isn't called on replaced frames that cease to be in use.

The code in nsFrameList certainly looks wrong! r= or sr=waterson to fix it in the obvious way. I'm not sure I understand your other questions. If this method were written properly, and aOldFrame was being replaced, wouldn't it just be destroyed? (And its space reclaimed?)

I was referring this time to the normal ReplaceFrame() which is used, e.g., in nsCSSFrameConstructor::CantRenderReplacedElement(...). When the old frame is replaced, I missed locating where the Destroy() of the old frame is being called -- and thus I was wondering if people have being assuming that ReplaceFrame() does destroy the old frame too. Am I missing something or what? (With the arena, not destroying a frame may not have averse effects -- but that may just be a coincidence, not the intention, since the arena was added later on, and what if the dangling frame has external things, e.g., an image). So I just have these questions that were bugging me.

Re-assigning to myself for the correction. r=, anyone?

Sorry if I dump this here rather than opening another bug. (I should perhaps change the title to something like 'Typos in the usage of child frame lists') NS_IMETHODIMP FrameManager::GetCanvasFrame(nsIPresContext* aPresContext, nsIFrame** aCanvasFrame) const { NS_ENSURE_TRUE(mPresShell, NS_ERROR_NOT_AVAILABLE); NS_PRECONDITION(aCanvasFrame, "aCanvasFrame argument cannot be null"); NS_PRECONDITION(aPresContext, "aPresContext argument cannot be null"); *aCanvasFrame = nsnull; if (mRootFrame) { // walk the children of the root frame looking for a frame with type==canvas // start at the root nsIFrame* childFrame = mRootFrame; while (childFrame) { // get each sibling of the child and check them, startig at the child nsIFrame *siblingFrame = childFrame; while (siblingFrame) { nsCOMPtr<nsIAtom> frameType; siblingFrame->GetFrameType(getter_AddRefs(frameType)); if (frameType.get() == nsLayoutAtoms::canvasFrame) { // this is it: set the out-arg and stop looking *aCanvasFrame = siblingFrame; break; <--- shouldn't this be a speedy return to avoid the outer loop } else { siblingFrame->GetNextSibling(&siblingFrame); } } // move on to the child's child childFrame->FirstChild(aPresContext, nsnull, &childFrame); } } return NS_OK; }

No time to hunt r/a forever, re-assigning to module owner.

Comment on attachment 67301 [details] [diff] [review] fix r=bzbarsky. Want me to hunt down a= for this? Let's not have crashers lurking in the tree. :)

I am leaving the final call to the module owner. I was hoping for some follow-up to my question in comment #4. It might be best to leave the bug open so that it can be used to provide a definitive answer later on.

Comment on attachment 67301 [details] [diff] [review] fix a=rjesup@wgate.com No risk - no one is currently calling this; obviously broken.

Comment on attachment 67301 [details] [diff] [review] fix marking obsolete - trunk obviously has this fix already. can this bug be marked fixed now?

Comment on attachment 67301 [details] [diff] [review] fix You claim "obviously", but you didn't check, and the claim happens to be false...

Taking bug from attinasi so I remember to get this checked in. rbs: Feel free to take it from me if you want to check it in yourself.

I actually did check... wonder why I missed it... maybe I read the patch the wrong way...

Suggesting to eliminate |nsFrameList::ReplaceAndDestroyFrame()| altogether, and to instead use a boolean in |nsFrameList::ReplaceFrame(..., PRBool aDestroy)|. This way, everybody will be forced to use that single entry-point and more importantly, to stop and think about telling their desired intention in |aDestroy| when calling nsFrameList::ReplaceFrame(). End result: less confusion and less prone to errors.

I just rolled comment 16, as well as the fix for the typo, into the patch on bug 176915, since I was touching related code anyway. Re comment 4, those are calling ReplaceFrame() on a _frame_, not on an nsFrameList. The frame impl should be handling the destruction.

Fixed with bzbarsky's checkin of bug 176915. (Right?)