Closed Bug 749392 Opened 12 years ago Closed 10 years ago

Remove remaining calls to JS_ClearScope()

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jst, Assigned: peterv)

References

Details

Attachments

(1 file)

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.
https://tbpl.mozilla.org/?tree=Try&rev=0a71087141ad is a try push with the "clear" function removed from all the shells.
This looks green on try!
Attachment #619063 - Flags: review?(jwalden+bmo)
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.
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 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+
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.
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]
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)
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(peterv)
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: