122748 - Typo that can lead to disaster

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[rbs]

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;
}

Comment 1

[rbs]

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

Comment 2

[rbs]

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.

Comment 3

[Chris Waterson]

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?)

Comment 4

[rbs]

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.

Comment 5

[rbs]

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

Comment 6

[rbs]

Attachment: fix

Comment 7

[rbs]

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;
}

Comment 8

[rbs]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

Comment 10

[rbs]

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 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 67301 [details] [diff] [review]
fix

a=[e-mail]
No risk - no one is currently calling this; obviously broken.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 67301 [details] [diff] [review]
fix

marking obsolete - trunk obviously has this fix already.

can this bug be marked fixed now?

Comment 13

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

Comment on attachment 67301 [details] [diff] [review]
fix

You claim "obviously", but you didn't check, and the claim happens to be
false...

Comment 14

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

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.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 16

[rbs]

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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

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

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