637099 - Remove JS_ClearScope calls on windows

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andreas Gal :gal]

JS_ClearScope is incompatible with modern JS language specifications that our JITs optimize against. We should remove it.

Comment 1

[Andreas Gal :gal]

Attachment: patch

Work in progress. Incomplete. Not ready for review.

Comment 2

[David Mandelin [:dmandelin]]

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]

peterv is working on this

Comment 4

[Peter Van der Beken [:peterv]]

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]]

our hero!

Comment 6

[Brendan Eich [:brendan]]

Can't wait.

/be

Comment 7

[Peter Van der Beken [:peterv]]

(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]]

(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]

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]]

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]]

Still debugging a leak.

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 557951 [details] [diff] [review]
v2

This looks awesome.

Comment 14

[David Mandelin [:dmandelin]]

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]]

Unfortunately no, there's another mochitest leak that I'm tracking down.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: v2.1

Merged, still leaks.

Comment 17

[Peter Van der Beken [:peterv]]

Attachment: v2.1

Updated to trunk, needs leak fix from bug 731173.

Comment 18

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/491ceed82be3

Comment 19

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/491ceed82be3