The default bug view has changed. See this FAQ.

Note objects holding JSScript as roots in the CC

RESOLVED FIXED in Firefox 9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox8 unaffected, firefox9- fixed, firefox10- fixed)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
Created attachment 571968 [details] [diff] [review]
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.
Attachment #571968 - Flags: review?(igor)
Peter, why do only some people see the leak in bug 687857?  How does one trigger this?

Updated

6 years ago
Attachment #571968 - Flags: review?(igor) → review+
(Assignee)

Comment 2

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

Comment 3

6 years ago
(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!
status-firefox10: --- → affected
status-firefox8: --- → unaffected
status-firefox9: --- → affected
tracking-firefox10: --- → ?
tracking-firefox9: --- → ?
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.
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/848eaac421d9
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/848eaac421d9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox10: affected → fixed

Comment 9

5 years ago
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.
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.
https://hg.mozilla.org/releases/mozilla-beta/rev/92f39c8a9cf3
status-firefox9: affected → fixed
Target Milestone: mozilla10 → mozilla9

Updated

5 years ago
tracking-firefox10: ? → -
tracking-firefox9: ? → -
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.