Last Comment Bug 699796 - Note objects holding JSScript as roots in the CC
: Note objects holding JSScript as roots in the CC
Status: RESOLVED FIXED
[MemShrink:P1][qa-]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Peter Van der Beken [:peterv]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 637099 678830 687857
  Show dependency treegraph
 
Reported: 2011-11-04 07:38 PDT by Peter Van der Beken [:peterv]
Modified: 2011-12-07 14:32 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
-
fixed
-
fixed


Attachments
v1 (3.72 KB, patch)
2011-11-04 07:38 PDT, Peter Van der Beken [:peterv]
igor: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2011-11-04 07:38:33 PDT
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.
Comment 1 Justin Lebar (not reading bugmail) 2011-11-04 08:07:30 PDT
Peter, why do only some people see the leak in bug 687857?  How does one trigger this?
Comment 2 Peter Van der Beken [:peterv] 2011-11-04 08:45:19 PDT
(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] 2011-11-04 08:46:45 PDT
(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) 2011-11-04 08:47:10 PDT
I see, thanks.  Fingers crossed!
Comment 5 Nicholas Nethercote [:njn] 2011-11-04 13:19:45 PDT
Fantastic work, Peter!
Comment 6 Andrew McCreight [:mccr8] 2011-11-04 13:31:02 PDT
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] 2011-11-07 09:21:44 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/848eaac421d9
Comment 8 Ed Morley [:emorley] 2011-11-07 17:25:49 PST
https://hg.mozilla.org/mozilla-central/rev/848eaac421d9
Comment 9 Alex Keybl [:akeybl] 2011-12-01 14:58:30 PST
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] 2011-12-01 15:18:24 PST
I'll land this in beta tomorrow.  It should already be in 10.
Comment 11 Andrew McCreight [:mccr8] 2011-12-02 06:13:49 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/92f39c8a9cf3
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-07 13:37:06 PST
Is there a testcase to verify this fix?
Comment 13 Andrew McCreight [:mccr8] 2011-12-07 13:38:40 PST
Not really.

Note You need to log in before you can comment on or make changes to this bug.