Closed Bug 637099 Opened 14 years ago Closed 13 years ago

Remove JS_ClearScope calls on windows


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox13 --- fixed


(Reporter: gal, Assigned: peterv)




(1 file, 3 obsolete files)

JS_ClearScope is incompatible with modern JS language specifications that our JITs optimize against. We should remove it.
Attached patch patch (obsolete) — Splinter Review
Work in progress. Incomplete. Not ready for review.
Assignee: general → gal
Andreas, do you still intend to work on this or should we find someone else. I really want it fixed because it keeps biting us. Bug 651030 is the latest.
Blocks: 651030
peterv is working on this
Assignee: gal → peterv
Depends on: 658632
Depends on: 658636
Depends on: 658637
Depends on: 659580
Depends on: 659581
With the fixes for the 5 dependent bugs this is looking close, I think there's only one remaining leak in mochitest-a11y. Still tracking that one down.
our hero!
Depends on: 662243
Can't wait.

(In reply to comment #6)
> Can't wait.

Your review of the patch in bug 659581 will make it happen sooner :-).
(In reply to comment #7)
> (In reply to comment #6)
> > Can't wait.
> Your review of the patch in bug 659581 will make it happen sooner :-).

Done! Sorry for the hold-up. sfink is good for jsd reviews too, I think. timeless has moved on from Nokia and Mozilla stuff, FYI.

If you'd like me to fuzz this before it lands, attach an updated patch and let me know :)
Blocks: 664797
There are a number of JS_ClearScope callers, and it's unclear to me if we can now remove them all.|ipc|dom&hitlimit=&tree=mozilla-central

Storage code uses it to remove all properties from a JS object, we'll need to do something else here. What should it do instead?

I think the XPConnect module loader uses it at shutdown to avoid leaks, I haven't tried to remove that one yet. There might be more leaks to fix as a result of removing it :-/.

We still need to make sure that removing the code that clears watchpoints is ok (bug 38959). The code that invalidates the "global scope polluter" looks like it was added to fix a leak (bug 256932), so that should be fine.
Blocks: 680456
No longer blocks: 680456
Blocks: 680456
Still debugging a leak.
Depends on: 682539
Blocks: 635548
Attached patch v2 (obsolete) — Splinter Review
Attachment #515422 - Attachment is obsolete: true
Attachment #557951 - Flags: review?(mrbkap)
Comment on attachment 557951 [details] [diff] [review]

This looks awesome.
Attachment #557951 - Flags: review?(mrbkap) → review+
It looks like there's been an r+'d patch here for about a month. Is that patch ready to land?
Unfortunately no, there's another mochitest leak that I'm tracking down.
Depends on: 696817
Depends on: 699796
Depends on: 699807
Blocks: 711695
Attached patch v2.1 (obsolete) — Splinter Review
Merged, still leaks.
No longer blocks: 721284
Blocks: 715149
Depends on: 731173
Attached patch v2.1Splinter Review
Updated to trunk, needs leak fix from bug 731173.
Attachment #557951 - Attachment is obsolete: true
Attachment #588405 - Attachment is obsolete: true
Attachment #601285 - Flags: review+
Summary: Remove JS_ClearScope → Remove JS_ClearScope calls on windows
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
Depends on: 837011
You need to log in before you can comment on or make changes to this bug.