Last Comment Bug 749371 - rm JS_ClearScope
: rm JS_ClearScope
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
Depends on: 637099 660237
Blocks: 470510 560556
  Show dependency treegraph
Reported: 2012-04-26 14:04 PDT by David Mandelin [:dmandelin]
Modified: 2012-08-27 19:16 PDT (History)
10 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

replace JS_ClearScope with not-bad functions (32.76 KB, patch)
2012-08-24 14:58 PDT, Luke Wagner [:luke]
mrbkap: review+
Details | Diff | Splinter Review
clear JS-internal clear-scope remnants (24.59 KB, patch)
2012-08-24 14:59 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review

Description David Mandelin [:dmandelin] 2012-04-26 14:04:32 PDT
JS_ClearScope is very dangerous, especially for JM+TI. With bug 637099 fixed, we should be able to remove the API entirely. According to MXR, it's used in a few test shells, and there is one use in Gecko at js/xpconnect/loader/mozJSComponentLoader.h:137.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-04-26 14:19:42 PDT

for localStorage.clear().
Comment 2 Luke Wagner [:luke] 2012-04-27 16:18:37 PDT
In case anyone tries this and immediately sees xpcshell leaks (which I suspect is somewhat likely), we could add a semantically-much-less-scary function (outside js/src even) to set undefined to all properties.
Comment 3 Luke Wagner [:luke] 2012-08-22 16:46:54 PDT
Let's try having JS_ClearScope just assign UndefinedValue to all non-reserved slots:

Fun fact: ObjectOps::clear is totally unused.
Comment 4 Luke Wagner [:luke] 2012-08-23 11:28:36 PDT
Well, improvement from comment 2, there are only two individual test failures.  One of them is in an xpcshell test:
  TypeError: Ci is undefined
which is exactly what would happen if you tried to run code on a cleared global (which previously reported an error but not with my dinky patch).

I'll try to tweak the tests not to run code on a cleared global.
Comment 5 Luke Wagner [:luke] 2012-08-23 11:29:18 PDT
(Also, no leaks reported!)
Comment 6 Luke Wagner [:luke] 2012-08-23 19:53:24 PDT
Awesome.  Two simple reasons for the two test failures.  Let's try again:
Comment 7 Luke Wagner [:luke] 2012-08-24 10:03:09 PDT
Sweet, now for a real patch.
Comment 8 Luke Wagner [:luke] 2012-08-24 14:58:54 PDT
Created attachment 655175 [details] [diff] [review]
replace JS_ClearScope with not-bad functions

The patch also takes the liberty of inlining js_NativeClear b/c this is the only use, and a gross one at that.
Comment 9 Luke Wagner [:luke] 2012-08-24 14:59:36 PDT
Created attachment 655176 [details] [diff] [review]
clear JS-internal clear-scope remnants

Let me know if I missed anything else you can think of.
Comment 10 Brian Hackett (:bhackett) 2012-08-24 15:26:27 PDT
Comment on attachment 655176 [details] [diff] [review]
clear JS-internal clear-scope remnants

Review of attachment 655176 [details] [diff] [review]:

Yay!  This catches everything I could think of.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2012-08-25 10:23:24 PDT
I found

249     /*
250      * Calling a function from a cleared global triggers this (yeah, I know).
251      * Uncomment this once bug 470510 is fixed (if that bug doesn't remove
252      * isCleared entirely).
253      */
254     // JS_ASSERT(!isCleared());

in GlobalObject::initFunctionAndObjectClasses, want to remove that too? :)
Comment 12 Blake Kaplan (:mrbkap) 2012-08-27 13:18:39 PDT
Comment on attachment 655175 [details] [diff] [review]
replace JS_ClearScope with not-bad functions


Note You need to log in before you can comment on or make changes to this bug.