If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
HTML: Parser
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

({mlk, testcase})

Trunk
x86
Mac OS X
mlk, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

10 years ago
Created attachment 293795 [details]
testcase
(Reporter)

Comment 2

10 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?
(Assignee)

Comment 4

10 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.
(Assignee)

Comment 5

10 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?
(Assignee)

Comment 7

10 years ago
Yeah, my preliminary patch already does that.
(Assignee)

Comment 8

10 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+

Updated

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

Comment 10

10 years ago
Fix checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
(Reporter)

Comment 11

8 years ago
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.