556241 - HTMLContentSink needs to participate in cycle collection

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: 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 [:bzbarsky]]

I thought we always broke this cycle as needed.... when do we fail to do it?

Comment 2

[Peter Van der Beken [:peterv]]

Still working on the testcase, but note that the nsContentSink already unlinks its document.

Comment 3

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/86fd7df36a31

Comment 4

[Peter Van der Beken [:peterv]]

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

Attachment: 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 [:bzbarsky]]

> 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]]

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 [:bzbarsky]]

Ah, I see. OK, thanks!

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 436160 [details] [diff] [review]
v1

I think we wanted to take this as well...

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 436160 [details] [diff] [review]
v1

Approved for 1.9.2.3, a=dveditz for release-drivers

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a98711249c9c

Comment 13

[LegNeato]

Can we get this fixed on the 1.9.1 branch, by tomorrow if possible?

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 436160 [details] [diff] [review]
v1

The patch works as-is on 1.9.1 too.

Comment 15

[LegNeato]

Comment on attachment 436160 [details] [diff] [review]
v1

a=LegNeato for 1.9.1.10

Comment 16

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe14252a861d