Closed Bug 408939 Opened 12 years ago Closed 12 years ago

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


(Core :: HTML: Parser, defect)

Not set





(Reporter: jruderman, Assigned: mrbkap)


(Keywords: memory-leak, testcase)


(2 files)

Loading the testcase locally causes a CToken to leak (as reported by trace-refcnt at shutdown).
Attached file testcase
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?
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.
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?
Yeah, my preliminary patch already does that.
Attached patch Proposed fixSplinter Review
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+
Attachment #293930 - Flags: superreview?(jst) → superreview+
Fix checked into trunk.
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Added crashtest:
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.