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

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Jesse Ruderman]

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

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

I can't reproduce loading the testcase from Bugzilla, probably because Bugzilla includes "charset=UTF-8" in its Content-Type HTTP header.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Blake Kaplan (:mrbkap) (inactive)]

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

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

We'll want to back out bug 395846 at that point, right?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, my preliminary patch already does that.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 293930 [details] [diff] [review]
Proposed fix

Yeah, that looks good.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 11

[Jesse Ruderman]

Added crashtest:
http://hg.mozilla.org/mozilla-central/rev/63120a5301f2