Closed
Bug 749392
Opened 12 years ago
Closed 10 years ago
Remove remaining calls to JS_ClearScope()
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: jst, Assigned: peterv)
References
Details
Attachments
(1 file)
3.17 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
||
This looks green on try!
Attachment #619063 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 3•12 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•12 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•12 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•12 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•12 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
Updated•12 years ago
|
Whiteboard: [leave open]
Comment 8•12 years ago
|
||
Part 1: https://hg.mozilla.org/mozilla-central/rev/8c748caad8e4
Comment 11•11 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?
Updated•11 years ago
|
Flags: needinfo? → needinfo?(peterv)
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(peterv)
Resolution: --- → FIXED
Whiteboard: [leave open]
Updated•10 years ago
|
Target Milestone: --- → mozilla18
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•