699796 - Note objects holding JSScript as roots in the CC

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

Bug 678830 made us add JSScript objects to the cycle collector, but it didn't change CheckParticipatesInCycleCollection to deal with script objects. We're not adding XPCOM objects holding a JSScript as roots to the cycle collector and as a result we're missing collectable cycles and leaking. I spent a lot of time tracking this down :-/.

This is probably the cause of bug 687857, which shouldn't have been ignored as a regression from bug 678830.

Comment 1

[Justin Lebar (not reading bugmail)]

Peter, why do only some people see the leak in bug 687857?  How does one trigger this?

Comment 2

[Peter Van der Beken [:peterv]]

(In reply to Justin Lebar [:jlebar] from comment #1)
> Peter, why do only some people see the leak in bug 687857?  How does one
> trigger this?

I don't know. The patch from bug 637099 made it almost always reproducible for me, but I haven't tried much without it.

Comment 3

[Peter Van der Beken [:peterv]]

(In reply to Peter Van der Beken [:peterv] from comment #2)
> The patch from bug 637099

That patch makes us not break certain cycles manually, so we rely more on the CC. It might be order of unlinking or other randomness that saves us sometimes without it.

Comment 4

[Justin Lebar (not reading bugmail)]

I see, thanks.  Fingers crossed!

Comment 5

[Nicholas Nethercote [inactive]]

Fantastic work, Peter!

Comment 6

[Andrew McCreight [:mccr8]]

I'll try to figure out whether these edges were introduced in the initial scripts-as-gcthings or in the followup that got rid of the object wrappers, as that will affect whether this needs to go in 9 or just 10.

Comment 7

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/848eaac421d9

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/848eaac421d9

Comment 9

[Alex Keybl [:akeybl]]

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

[Triage Comment]
Approving for aurora and beta, in the hopes that this low risk fix helps resolve 687857 and the FF9-specific negative input feedback around memory issues.

Comment 10

[Andrew McCreight [:mccr8]]

I'll land this in beta tomorrow.  It should already be in 10.

Comment 11

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/releases/mozilla-beta/rev/92f39c8a9cf3

Comment 12

[u279076]

Is there a testcase to verify this fix?

Comment 13

[Andrew McCreight [:mccr8]]

Not really.