Closed Bug 699796 Opened 8 years ago Closed 8 years ago

Note objects holding JSScript as roots in the CC


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox8 --- unaffected
firefox9 - fixed
firefox10 - fixed


(Reporter: peterv, Assigned: peterv)



(Whiteboard: [MemShrink:P1][qa-])


(1 file)

Attached patch v1Splinter Review
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.
Attachment #571968 - Flags: review?(igor)
Peter, why do only some people see the leak in bug 687857?  How does one trigger this?
Attachment #571968 - Flags: review?(igor) → review+
(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.
(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.
I see, thanks.  Fingers crossed!
Whiteboard: [MemShrink:P1]
Fantastic work, Peter!
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.
Closed: 8 years ago
Resolution: --- → FIXED
Comment on attachment 571968 [details] [diff] [review]

[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.
Attachment #571968 - Flags: approval-mozilla-beta+
Attachment #571968 - Flags: approval-mozilla-aurora+
I'll land this in beta tomorrow.  It should already be in 10.
Is there a testcase to verify this fix?
Whiteboard: [MemShrink:P1] → [MemShrink:P1][qa?]
Not really.
Whiteboard: [MemShrink:P1][qa?] → [MemShrink:P1][qa-]
You need to log in before you can comment on or make changes to this bug.