The default bug view has changed. See this FAQ.

HTMLContentSink needs to participate in cycle collection

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(blocking1.9.2 .3+, status1.9.2 .3-fixed, blocking1.9.1 .10+, status1.9.1 .10-fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
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.
Attachment #436160 - Flags: review+
I thought we always broke this cycle as needed.... when do we fail to do it?
(Assignee)

Comment 2

7 years ago
Still working on the testcase, but note that the nsContentSink already unlinks its document.
(Assignee)

Comment 3

7 years ago
http://hg.mozilla.org/mozilla-central/rev/86fd7df36a31
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Comment 4

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

Comment 5

7 years ago
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
Attachment #436192 - Attachment is obsolete: true
> 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.
(Assignee)

Comment 7

7 years ago
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.
Ah, I see. OK, thanks!

Updated

7 years ago
Attachment #436160 - Flags: approval1.9.2.3?
Comment on attachment 436160 [details] [diff] [review]
v1

I think we wanted to take this as well...
blocking1.9.1: --- → .10+
blocking1.9.2: --- → .3+
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Blocks: 555109
Comment on attachment 436160 [details] [diff] [review]
v1

Approved for 1.9.2.3, a=dveditz for release-drivers
Attachment #436160 - Flags: approval1.9.2.3? → approval1.9.2.3+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a98711249c9c
status1.9.2: wanted → .3-fixed
blocking1.9.2: .4+ → .3+
status1.9.2: .4-fixed → .3-fixed
Attachment #436160 - Flags: approval1.9.2.4+ → approval1.9.2.3+

Comment 13

7 years ago
Can we get this fixed on the 1.9.1 branch, by tomorrow if possible?
(Assignee)

Comment 14

7 years ago
Comment on attachment 436160 [details] [diff] [review]
v1

The patch works as-is on 1.9.1 too.
Attachment #436160 - Flags: approval1.9.1.10?

Comment 15

7 years ago
Comment on attachment 436160 [details] [diff] [review]
v1

a=LegNeato for 1.9.1.10
Attachment #436160 - Flags: approval1.9.1.10? → approval1.9.1.10+
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/fe14252a861d
status1.9.1: wanted → .10-fixed
You need to log in before you can comment on or make changes to this bug.