Typo that can lead to disaster

RESOLVED FIXED in mozilla1.2beta

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: rbs, Assigned: dbaron)

Tracking

Trunk
mozilla1.2beta
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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;
}
(Reporter)

Comment 1

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

Comment 2

16 years ago
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

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

Comment 4

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

Comment 5

16 years ago
Re-assigning to myself for the correction.
r=, anyone?
Assignee: attinasi → rbs
(Reporter)

Comment 6

16 years ago
Created attachment 67301 [details] [diff] [review]
fix
(Reporter)

Updated

16 years ago
Attachment #67301 - Flags: superreview+

Updated

16 years ago
Priority: -- → P2
(Reporter)

Comment 7

16 years ago
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;
}
(Reporter)

Comment 8

16 years ago
No time to hunt r/a forever, re-assigning to module owner.
Assignee: rbs → attinasi

Updated

16 years ago
Target Milestone: --- → Future

Updated

16 years ago
Keywords: patch
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.  :)
Attachment #67301 - Flags: review+
(Reporter)

Comment 10

16 years ago
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.
Attachment #67301 - Flags: approval+
Attachment #67301 - Attachment is obsolete: true
Comment on attachment 67301 [details] [diff] [review]
fix

marking obsolete - trunk obviously has this fix already.

can this bug be marked fixed now?
Attachment #67301 - Attachment is obsolete: false
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.
Assignee: attinasi → dbaron
Whiteboard: [patch]
Target Milestone: Future → mozilla1.2beta
I actually did check... wonder why I missed it... maybe I read the patch the
wrong way...
(Reporter)

Comment 16

15 years ago
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.
Status: NEW → ASSIGNED
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.
Depends on: 176915
Fixed with bzbarsky's checkin of bug 176915.  (Right?)
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.