Closed
Bug 26896
Opened 25 years ago
Closed 25 years ago
Leak of a style context?
Categories
(Core :: Layout, defect, P1)
Tracking
()
VERIFIED
INVALID
M16
People
(Reporter: rbs, Assigned: buster)
References
()
Details
(Keywords: memory-leak)
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;" ?
mine! mine mine mine! all mine! whoo-hoo!
Assignee: kipp → buster
Keywords: mlk
memory leaks matter a lot, setting priority to P1, severity to major. Will fix
very soon.
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P1
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."
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → INVALID
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•25 years ago
|
||
Marking verified invalid based on Steve's comments.
Status: RESOLVED → VERIFIED
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.
Reporter | ||
Comment 10•25 years ago
|
||
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.
Reporter | ||
Comment 11•25 years ago
|
||
Steve? This bug is still itching me...
You need to log in
before you can comment on or make changes to this bug.
Description
•