All users were logged out of Bugzilla on October 13th, 2018

Remove remaining calls to JS_ClearScope()

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jst, Assigned: peterv)

Tracking

unspecified
mozilla18
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
We removed the call to JS_ClearScope() that we make on window objects when we navigate away from a window in bug 637099, and now we have only a handful left. This bug is to track the removal of the remaining calls, which are:

  1: xpschell, the ipc test shell, and the js shell all expose "clear" to script, which calls JS_ClearScope() internally
  2: xpcshell calls JS_ClearScope() on the global before exiting
  3: the JS component loader calls JS_ClearScope() on its global when unloading
  4: the quickstub for DOM storage calls JS_ClearScope() when the storage is cleared.

For 1 we should probably just remove that functionality. I'll push to a try build w/o exposing clear and see how many tests, if any, depend on that function be exposed. For 2 and 3 we might simply be able to remove the call, worst case (i.e. if that causes nasty leaks) we can replace all properties with undefined instead of depending on JS_ClearScope(). 4 will be solved by new proxy based bindings for DOM storage.
(Reporter)

Comment 1

7 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0a71087141ad is a try push with the "clear" function removed from all the shells.
(Reporter)

Comment 2

7 years ago
Created attachment 619063 [details] [diff] [review]
Stop exposing "clear" in our shells.

This looks green on try!
Attachment #619063 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 3

7 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ae8fe73838a0 is a push to try with the JS component loader no longer calling JS_ClearScope() on module destruction. It's so far all green, but there's still many tests to go.
(Reporter)

Comment 4

7 years ago
The reason that was reasonably green was that I accidentally did optimized builds, so the leaks that it introduced were not detected. Running the same patch through debug builds shows leaks in all tests :(

Comment 5

7 years ago
Comment on attachment 619063 [details] [diff] [review]
Stop exposing "clear" in our shells.

Review of attachment 619063 [details] [diff] [review]:
-----------------------------------------------------------------

No rest for the wicked!  :-)  Feel free to pocket this r+ for landing after all those leaks get taken care of.
Attachment #619063 - Flags: review?(jwalden+bmo) → review+
(Reporter)

Comment 6

7 years ago
Thanks waldo! The leaks were with a patch that actually removed calls to JS_ClearScope(), the patch you reviewed just removes the ability to call JS_ClearScope() from our shells, and as the tests shows, nobody uses that. But leak fixing needs to happen before further work beyond this patch can land.
(Reporter)

Comment 7

7 years ago
Landed the "clear" method removal from our shells. More work to do here, so this bug should stay open after this hits m-c.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c748caad8e4
Whiteboard: [leave open]
Duplicate of this bug: 620942
Duplicate of this bug: 621629

Comment 11

5 years ago
Can this bug be closed?

Bug 749371 "[broke up] JS_ClearScope into two not-as-bad functions":
https://hg.mozilla.org/mozilla-central/rev/104671eaadb8

Bug 749371 Comment 5:
> (Also, no leaks reported!)

Comment 6:
> Awesome.  Two simple reasons for the two test failures.  Let's try again:
> https://tbpl.mozilla.org/?tree=Try&rev=33f4ddf29a4a

Comment 7:
> Sweet, now for a real patch.
Flags: needinfo?
Flags: needinfo? → needinfo?(peterv)
(Assignee)

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(peterv)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.