Leak CToken with <meta http-equiv="Content-Type"> not near top of document




11 years ago
10 years ago


(Reporter: jruderman, Assigned: mrbkap)


({memory-leak, testcase})

Mac OS X
memory-leak, testcase
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(2 attachments)



11 years ago
Loading the testcase locally causes a CToken to leak (as reported by trace-refcnt at shutdown).

Comment 1

11 years ago
Created attachment 293795 [details]

Comment 2

11 years ago
I can't reproduce loading the testcase from Bugzilla, probably because Bugzilla includes "charset=UTF-8" in its Content-Type HTTP header.
So the issue is that CNavDTD::HandleToken treats an error return from HandleStartToken and company by leaking the token and returning NS_OK.  That seems a little odd, to me....  Shouldn't we IF_FREE the token even if handling failed?  In fact, free it no matter what Handle*Token returned?

In any case, in this case the return value is NS_ERROR_HTMLPARSER_STOPPARSING, which isn't even a "real" error as far as this stuff is concerned...

Blake, do you see anything wrong with just unconditionally releasing theToken?

Comment 4

11 years ago
I tried doing just that for bug 395846 and it caused a bunch of really odd regressions that I didn't have a chance to sort out at the time. I'll try taking another look to see what was really happening soon.

Comment 5

11 years ago
Trying again, I wonder if I fat-fingered my changes the last time I tried to do this. I just tried again and the patch passes the parser mochitests. I'll do some more testing and attach a patch tomorrow.
Assignee: nobody → mrbkap
We'll want to back out bug 395846 at that point, right?

Comment 7

11 years ago
Yeah, my preliminary patch already does that.

Comment 8

11 years ago
Created attachment 293930 [details] [diff] [review]
Proposed fix

To convince myself of this patch's validity, I went through CNavDTD and convinced myself that we don't addref or release anything based on return values. I also attempted to convince myself that every time we call IF_FREE on a token, we do not fall through to this case in HandleToken. In particular, every time we call IF_FREE, we are popping tokens off of the tokenizer or the misplaced content queue, so we shouldn't run into a problem there. Please check my logic on the new if statements. I suspect that's where I botched things last time.
Attachment #293930 - Flags: superreview?(jst)
Attachment #293930 - Flags: review?(bzbarsky)
Comment on attachment 293930 [details] [diff] [review]
Proposed fix

Yeah, that looks good.
Attachment #293930 - Flags: review?(bzbarsky) → review+


11 years ago
Attachment #293930 - Flags: superreview?(jst) → superreview+

Comment 10

11 years ago
Fix checked into trunk.
Last Resolved: 11 years ago
Resolution: --- → FIXED
Flags: in-testsuite?

Comment 11

10 years ago
Added crashtest:
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.