Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dmandelin, Assigned: luke)

Tracking

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.

http://mxr.mozilla.org/mozilla-central/ident?i=JS_ClearScope
And

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dom_quickstubs.qsconf#565

for localStorage.clear().
Depends on: 660237
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Let's try having JS_ClearScope just assign UndefinedValue to all non-reserved slots:
  https://tbpl.mozilla.org/?tree=Try&rev=1e1e19282c2b

Fun fact: ObjectOps::clear is totally unused.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(Also, no leaks reported!)
(Assignee)

Comment 6

5 years ago
Awesome.  Two simple reasons for the two test failures.  Let's try again:

https://tbpl.mozilla.org/?tree=Try&rev=33f4ddf29a4a
(Assignee)

Comment 7

5 years ago
Sweet, now for a real patch.
(Assignee)

Comment 8

5 years ago
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.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #655175 - Flags: review?(mrbkap)
(Assignee)

Comment 9

5 years ago
Created attachment 655176 [details] [diff] [review]
clear JS-internal clear-scope remnants

Let me know if I missed anything else you can think of.
Attachment #655176 - Flags: review?(bhackett1024)
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.
Attachment #655176 - Flags: review?(bhackett1024) → review+
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 on attachment 655175 [details] [diff] [review]
replace JS_ClearScope with not-bad functions

Nice.
Attachment #655175 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/104671eaadb8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e208bf8354c9

Arg, I saw comment 10 too late!  I'll remove it in another patch.
https://hg.mozilla.org/mozilla-central/rev/104671eaadb8
https://hg.mozilla.org/mozilla-central/rev/e208bf8354c9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.