1097987 - Make all JS::Evaluate and JS_Execute*Script entrypoints take scope chains explicitly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The idea is to not have anything except With and globals on the toplevel scope chain.  This is bug 679939 comment 29 item 1.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change some JS shell function signatures to make it clearer that they are always working with globals, not arbitrary objects

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change JS shell's Run to always compile/execute the script in global scope instead of using its this object

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Change XPCShellEnvironment to use the scopechain version of JS_ExecuteScript as needed

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Change mozJSSubScriptLoader to use the scopechain version of JS_ExecuteScript as needed

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Change XPCShellImpl to use the scopechain version of JS_ExecuteScript as needed

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Require callers of JS_ExecuteScript to either use the global as the scope or pass in an explicit scopechain

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Require callers of JS::Evaluate to either use the global as the scope or pass in an explicit scopechain

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Change ExecuteInGlobalAndReturnScope to not use a random object on the scopechain

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Require callers of JS::Evaluate to either use the global as the scope or pass in an explicit scopechain

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Require callers of JS::Evaluate to either use the global as the scope or pass in an explicit scopechain

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Change ExecuteInGlobalAndReturnScope to not use a random object on the scopechain

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8576984 [details] [diff] [review]
part 8.  Change ExecuteInGlobalAndReturnScope to not use a random object on the scopechain

Part 8 here is going to need more work.  I've spun it off into bug 1142832.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8576928 [details] [diff] [review]
part 3.  Change XPCShellEnvironment to use the scopechain version of JS_ExecuteScript as needed

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

::: ipc/testshell/XPCShellEnvironment.cpp
@@ +171,5 @@
>          fclose(file);
>          if (!ok)
>              return false;
>  
> +        // XXXbz are we intentionally allowing load.call(someNonGlobalObject)?

No idea. I'd be happy to try throwing for a non-global |obj| and see if it breaks anything.

@@ +340,5 @@
>              }
>          }
>          ungetc(ch, file);
>  
>          JSAutoRequest ar(cx);

There should be an AutoJSAPI or AutoEntryScript somewhere up the stack, right? If so, I think this JSAutoRequest can go away.

@@ +359,5 @@
>      do {
>          bufp = buffer;
>          *bufp = '\0';
>  
>          JSAutoRequest ar(cx);

same.

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 8576983 [details] [diff] [review]
part 7.  Require callers of JS::Evaluate to either use the global as the scope or pass in an explicit scopechain

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

::: js/xpconnect/src/Sandbox.cpp
@@ +1533,5 @@
>          options.setFileAndLine(filenameBuf.get(), lineNo)
>                 .setVersion(jsVersion);
>          JS::RootedObject rootedSandbox(sandcx, sandbox);
> +        MOZ_ASSERT(JS_IsGlobalObject(sandbox));
> +        ok = JS::Evaluate(sandcx, options,

|rootedSandbox| is an unused variable now -- remove it.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> No idea. I'd be happy to try throwing for a non-global |obj| and see if it breaks
> anything.

Alright, let's give it a shot.

> There should be an AutoJSAPI or AutoEntryScript somewhere up the stack, right? 

Hmm.  There's an AutoSafeJSContext, right.  Great.

> |rootedSandbox| is an unused variable now -- remove it.

Done.

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92ff2fa07cf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/975552180fb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c1eafe518d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f642c3fe61bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3c79f06c22
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf1fb1365dd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/702b075ca1e6

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/92ff2fa07cf0
https://hg.mozilla.org/mozilla-central/rev/975552180fb6
https://hg.mozilla.org/mozilla-central/rev/9c1eafe518d0
https://hg.mozilla.org/mozilla-central/rev/f642c3fe61bd
https://hg.mozilla.org/mozilla-central/rev/2a3c79f06c22
https://hg.mozilla.org/mozilla-central/rev/cf1fb1365dd9
https://hg.mozilla.org/mozilla-central/rev/702b075ca1e6