Last Comment Bug 556241 - HTMLContentSink needs to participate in cycle collection
: HTMLContentSink needs to participate in cycle collection
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: CVE-2010-1121
  Show dependency treegraph
 
Reported: 2010-03-31 04:33 PDT by Peter Van der Beken [:peterv]
Modified: 2010-04-23 05:22 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.3+
.3-fixed
.10+
.10-fixed


Attachments
v1 (7.45 KB, patch)
2010-03-31 04:33 PDT, Peter Van der Beken [:peterv]
peterv: review+
dveditz: approval1.9.2.3+
christian: approval1.9.1.10+
Details | Diff | Splinter Review
Testcase (474 bytes, text/html)
2010-03-31 07:55 PDT, Peter Van der Beken [:peterv]
no flags Details
Testcase (489 bytes, text/html)
2010-03-31 08:41 PDT, Peter Van der Beken [:peterv]
no flags Details

Description Peter Van der Beken [:peterv] 2010-03-31 04:33:22 PDT
Created attachment 436160 [details] [diff] [review]
v1

It holds a document and it can end up in a cycle with the parser and that document.

Patch has r=jst. I'll try to write a testcase for this.
Comment 1 Boris Zbarsky [:bz] 2010-03-31 06:15:46 PDT
I thought we always broke this cycle as needed.... when do we fail to do it?
Comment 2 Peter Van der Beken [:peterv] 2010-03-31 06:27:39 PDT
Still working on the testcase, but note that the nsContentSink already unlinks its document.
Comment 3 Peter Van der Beken [:peterv] 2010-03-31 06:57:36 PDT
http://hg.mozilla.org/mozilla-central/rev/86fd7df36a31
Comment 4 Peter Van der Beken [:peterv] 2010-03-31 07:55:54 PDT
Created attachment 436192 [details]
Testcase

This is the simplest testcase that leaks for me. I'm also seeing two assertions:

###!!! ASSERTION: Non-global object has the wrong flags: '!(jsclazz->flags & JSCLASS_IS_GLOBAL)', file js/src/xpconnect/src/xpcwrappednative.cpp, line 1118

###!!! ASSERTION: Overwriting an existing document channel!: '(loadFlags & nsIChannel::LOAD_REPLACE) || !(mDocumentRequest.get())', file uriloader/base/nsDocLoader.cpp, line 532

Need to figure out a way to stop the reloads after a while too.

Who is supposed to break the cycle without CC? We should be able to check that in this testcase.
Comment 5 Peter Van der Beken [:peterv] 2010-03-31 08:41:25 PDT
Created attachment 436200 [details]
Testcase

This one reloads the main page twice. When shutting down I get in the leaked urls:

  file:///Users/peterv/test_bug556241.html
  file:///Users/peterv/test_bug556241.html
  file:///Users/peterv/test_bug556241.html?1
Comment 6 Boris Zbarsky [:bz] 2010-03-31 11:36:46 PDT
> Who is supposed to break the cycle without CC?

Used to be that DidBuildModel on the sink would drop refs as needed....  But maybe we changed that.
Comment 7 Peter Van der Beken [:peterv] 2010-03-31 12:12:24 PDT
Ah, but that drops the reference from sink to parser iirc. I think the issue here is that the testcase just calls document.open, so the document holds a reference to the parser, the parser holds a reference to the sink and the sink holds two references to the document (one in nsContentSink and one in HTMLContentSink). CC knows about all of these, except for HTMLContentSink->document.
Comment 8 Boris Zbarsky [:bz] 2010-03-31 12:16:10 PDT
Ah, I see. OK, thanks!
Comment 9 Blake Kaplan (:mrbkap) 2010-03-31 17:07:28 PDT
Comment on attachment 436160 [details] [diff] [review]
v1

I think we wanted to take this as well...
Comment 10 Daniel Veditz [:dveditz] 2010-03-31 17:39:19 PDT
Comment on attachment 436160 [details] [diff] [review]
v1

Approved for 1.9.2.3, a=dveditz for release-drivers
Comment 11 Blake Kaplan (:mrbkap) 2010-03-31 20:00:29 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a98711249c9c
Comment 13 christian 2010-04-15 16:12:40 PDT
Can we get this fixed on the 1.9.1 branch, by tomorrow if possible?
Comment 14 Peter Van der Beken [:peterv] 2010-04-19 13:25:26 PDT
Comment on attachment 436160 [details] [diff] [review]
v1

The patch works as-is on 1.9.1 too.
Comment 15 christian 2010-04-20 06:14:31 PDT
Comment on attachment 436160 [details] [diff] [review]
v1

a=LegNeato for 1.9.1.10
Comment 16 Peter Van der Beken [:peterv] 2010-04-23 05:22:09 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe14252a861d

Note You need to log in before you can comment on or make changes to this bug.