Closed Bug 408939 Opened 12 years ago Closed 12 years ago
Leak CToken with <meta http-equiv="Content-Type"> not near top of document
Loading the testcase locally causes a CToken to leak (as reported by trace-refcnt at shutdown).
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.
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.
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Added crashtest: http://hg.mozilla.org/mozilla-central/rev/63120a5301f2
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.