26896 - Leak of a style context?

Status: VERIFIED INVALID

(Core :: Layout, defect, P1)

Description

[rbs]

The Init() method of nsFirstLetterFrame.cpp involves re-resolving the
style context of continuing frames. Once the new style context is known,
newSC, the code proceeds as:

138       if (nsnull != newSC) {
139         if (newSC == aContext) {
140           NS_RELEASE(newSC);
141         }
142         else {
143           aContext = newSC;
144         }
145       }

Line 143 suggests that the previous pointer is overwritten, meaning
the previous style context is left dangling in the air. Shouldn't
this be "NS_RELEASE(aContext); aContext = newSC;" ?

Comment 1

[troy]

Re-assigning to Kipp's bug list

Comment 2

[buster]

nice catch, rbs. 

Comment 3

[buster]

mine! mine mine mine!  all mine!  whoo-hoo!

Comment 4

[buster]

memory leaks matter a lot, setting priority to P1, severity to major.  Will fix 
very soon.

Comment 5

[buster]

This bug is invalid.  Reading the code closely, you'll see that aContext is 
passed in as a pointer, and not a pointer-to-a-pointer.  Equivalent (and in my 
mind more clear) pseudo-code would be:

NS_IMETHODIMP
nsFirstLetterFrame::Init(..., nsIStyleContext* aContext, ...)
{
  nsresult rv;
  // styleContextForInitialization is what we hand to our superclass
  nsIStyleContext * styleContextForInitialization = aContext;
  nsIStyleContext* newSC = nsnull;
  ...

      if (nsnull != newSC) {
        if (newSC == aContext) {
          NS_RELEASE(newSC);
        }
        else {
          styleContextForInitialization = newSC;
        }
      }
    }
  }
  rv = nsFirstLetterFrameSuper::Init(aPresContext, 
                                     styleContextForInitialization,
                                     aParent,
                                     aContext, aPrevInFlow);

The key point here is that it's the caller that owns the refcount on aContext.  
We do nothing in this method to change the pointer that was used to initialize 
the param "aContext."

Comment 6

[rbs]

OK. I now see what you mean. 

On another matter. While I was reading the code again, I saw the following:

122   if (aPrevInFlow) {
...

127       nsIAtom* atom = aPrevInFlow
128         ? nsHTMLAtoms::mozLetterFrame
129         : nsHTMLAtoms::firstLetterPseudo;

...

147   }

Why the unnecessary control in 127-129? Something else was intended there?

Comment 7

[Chris Petersen]

Marking verified invalid based on Steve's comments.

Comment 8

[rbs]

Steve, did you had a chance to look at my other query?

Comment 9

[buster]

rbs:

It's not the kind of code I like to write because it is a bit obscure, but it is 
correct.

The original:

      nsIAtom* atom = aPrevInFlow
        ? nsHTMLAtoms::mozLetterFrame
        : nsHTMLAtoms::firstLetterPseudo;

Can be rewritten as

      nsIAtom* atom;
      if (aPrevInFlow)
        atom = nsHTMLAtoms::mozLetterFrame;
      else
        atom = nsHTMLAtoms::firstLetterPseudo;

which has identical semantics, and most compilers will produce identical code 
for the two versions.  So, the control isn't "unnecessary", it's just not as 
clear as I'd like.

Comment 10

[rbs]

Actually, I am wondering why all this is going on from within 
another if (aPrevInFlow) in line 122. i.e, the code is doing:

if (aPrevInFlow) {
  ...
  if (aPrevInFlow) atom = ...
  ...
}

So I am wondering if the code is missing something else.

Comment 11

[rbs]

Steve? This bug is still itching me...