Last Comment Bug 637099 - Remove JS_ClearScope calls on windows
: Remove JS_ClearScope calls on windows
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on: 658632 658636 658637 659580 659581 662243 682539 696817 699796 699807 731173 837011
Blocks: 723449 560556 635548 651030 664797 680456 695505 706467 711695 715149 749371
  Show dependency treegraph
 
Reported: 2011-02-26 21:18 PST by Andreas Gal :gal
Modified: 2013-02-10 11:45 PST (History)
27 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (24.82 KB, patch)
2011-02-26 21:19 PST, Andreas Gal :gal
no flags Details | Diff | Splinter Review
v2 (22.79 KB, patch)
2011-09-02 14:45 PDT, Peter Van der Beken [:peterv]
mrbkap: review+
Details | Diff | Splinter Review
v2.1 (22.35 KB, patch)
2012-01-13 06:46 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
v2.1 (22.83 KB, patch)
2012-02-28 08:36 PST, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-02-26 21:18:20 PST
JS_ClearScope is incompatible with modern JS language specifications that our JITs optimize against. We should remove it.
Comment 1 Andreas Gal :gal 2011-02-26 21:19:12 PST
Created attachment 515422 [details] [diff] [review]
patch

Work in progress. Incomplete. Not ready for review.
Comment 2 David Mandelin [:dmandelin] 2011-04-28 16:28:46 PDT
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.
Comment 3 Andreas Gal :gal 2011-04-28 17:01:09 PDT
peterv is working on this
Comment 4 Peter Van der Beken [:peterv] 2011-06-01 02:58:33 PDT
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.
Comment 5 Luke Wagner [:luke] 2011-06-01 09:29:04 PDT
our hero!
Comment 6 Brendan Eich [:brendan] 2011-06-09 21:07:18 PDT
Can't wait.

/be
Comment 7 Peter Van der Beken [:peterv] 2011-06-14 07:22:37 PDT
(In reply to comment #6)
> Can't wait.

Your review of the patch in bug 659581 will make it happen sooner :-).
Comment 8 Brendan Eich [:brendan] 2011-06-14 09:08:26 PDT
(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.

/be
Comment 9 Jesse Ruderman 2011-06-14 18:04:58 PDT
If you'd like me to fuzz this before it lands, attach an updated patch and let me know :)
Comment 10 Peter Van der Beken [:peterv] 2011-06-28 06:16:21 PDT
There are a number of JS_ClearScope callers, and it's unclear to me if we can now remove them all.

http://mxr.mozilla.org/mozilla-central/search?string=ClearScope&find=&findi=&filter=xpconnect|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.
Comment 11 Peter Van der Beken [:peterv] 2011-08-25 11:45:23 PDT
Still debugging a leak.
Comment 12 Peter Van der Beken [:peterv] 2011-09-02 14:45:02 PDT
Created attachment 557951 [details] [diff] [review]
v2
Comment 13 Blake Kaplan (:mrbkap) 2011-09-02 15:12:39 PDT
Comment on attachment 557951 [details] [diff] [review]
v2

This looks awesome.
Comment 14 David Mandelin [:dmandelin] 2011-10-06 18:58:42 PDT
It looks like there's been an r+'d patch here for about a month. Is that patch ready to land?
Comment 15 Peter Van der Beken [:peterv] 2011-10-07 04:37:52 PDT
Unfortunately no, there's another mochitest leak that I'm tracking down.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-01-13 06:46:33 PST
Created attachment 588405 [details] [diff] [review]
v2.1

Merged, still leaks.
Comment 17 Peter Van der Beken [:peterv] 2012-02-28 08:36:09 PST
Created attachment 601285 [details] [diff] [review]
v2.1

Updated to trunk, needs leak fix from bug 731173.
Comment 18 Peter Van der Beken [:peterv] 2012-02-28 13:16:58 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/491ceed82be3
Comment 19 Matt Brubeck (:mbrubeck) 2012-02-29 11:17:22 PST
https://hg.mozilla.org/mozilla-central/rev/491ceed82be3

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