Remove JS_ClearScope calls on windows

VERIFIED FIXED in Firefox 13

Status

()

Core
JavaScript Engine
VERIFIED FIXED
7 years ago
5 years ago

People

(Reporter: gal, Assigned: peterv)

Tracking

(Blocks: 1 bug)

unspecified
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 years ago
JS_ClearScope is incompatible with modern JS language specifications that our JITs optimize against. We should remove it.
(Reporter)

Comment 1

7 years ago
Created attachment 515422 [details] [diff] [review]
patch

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
(Reporter)

Comment 3

6 years ago
peterv is working on this
Assignee: gal → peterv
(Assignee)

Updated

6 years ago
Depends on: 658632
(Assignee)

Updated

6 years ago
Depends on: 658636
(Assignee)

Updated

6 years ago
Depends on: 658637
(Assignee)

Updated

6 years ago
Depends on: 659580
(Assignee)

Updated

6 years ago
Depends on: 659581
(Assignee)

Comment 4

6 years ago
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

6 years ago
our hero!
(Assignee)

Updated

6 years ago
Depends on: 662243
Can't wait.

/be
(Assignee)

Comment 7

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

/be

Comment 9

6 years ago
If you'd like me to fuzz this before it lands, attach an updated patch and let me know :)

Updated

6 years ago
Blocks: 664797
(Assignee)

Comment 10

6 years ago
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.
Blocks: 680456

Updated

6 years ago
No longer blocks: 680456

Updated

6 years ago
Blocks: 680456
(Assignee)

Comment 11

6 years ago
Still debugging a leak.
(Assignee)

Updated

6 years ago
Depends on: 682539
Blocks: 635548
(Assignee)

Comment 12

6 years ago
Created attachment 557951 [details] [diff] [review]
v2
Attachment #515422 - Attachment is obsolete: true
Attachment #557951 - Flags: review?(mrbkap)
Comment on attachment 557951 [details] [diff] [review]
v2

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?
(Assignee)

Comment 15

6 years ago
Unfortunately no, there's another mochitest leak that I'm tracking down.
(Assignee)

Updated

6 years ago
Depends on: 696817
(Assignee)

Updated

6 years ago
Depends on: 699796
(Assignee)

Updated

6 years ago
Depends on: 699807
Blocks: 711695

Updated

6 years ago
Blocks: 706467
Created attachment 588405 [details] [diff] [review]
v2.1

Merged, still leaks.
Blocks: 695505
Blocks: 721284
No longer blocks: 721284
Blocks: 715149
Blocks: 723449

Updated

6 years ago
Blocks: 560556
(Assignee)

Updated

6 years ago
Depends on: 731173
(Assignee)

Comment 17

6 years ago
Created attachment 601285 [details] [diff] [review]
v2.1

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+
(Assignee)

Comment 18

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

Updated

6 years ago
Summary: Remove JS_ClearScope → Remove JS_ClearScope calls on windows
https://hg.mozilla.org/mozilla-central/rev/491ceed82be3
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
status-firefox13: --- → fixed
Status: RESOLVED → VERIFIED
Blocks: 749371

Updated

5 years ago
Depends on: 837011
You need to log in before you can comment on or make changes to this bug.