Closed Bug 1097987 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 4 obsolete files)

10.08 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.86 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
6.70 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.36 KB, patch
bholley
: review+
Details | Diff | Splinter Review
10.44 KB, patch
bholley
: review+
Details | Diff | Splinter Review
27.79 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
23.19 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
The idea is to not have anything except With and globals on the toplevel scope chain.  This is bug 679939 comment 29 item 1.
Depends on: 1141905
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8576931 - Flags: review?(bobbyholley)
Attachment #8576937 - Attachment is obsolete: true
Attachment #8576937 - Flags: review?(jwalden+bmo)
Attachment #8576975 - Attachment is obsolete: true
Attachment #8576975 - Flags: review?(jwalden+bmo)
Attachment #8576938 - Attachment is obsolete: true
Attachment #8576938 - Flags: review?(jwalden+bmo)
Attachment #8576925 - Flags: review?(jwalden+bmo) → review+
Attachment #8576927 - Flags: review?(jwalden+bmo) → review+
Blocks: 1142310
Blocks: 1142832
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.
Attachment #8576984 - Attachment is obsolete: true
Attachment #8576984 - Flags: review?(jwalden+bmo)
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.
Attachment #8576928 - Flags: review?(bobbyholley) → review+
Attachment #8576931 - Flags: review?(bobbyholley) → review+
Attachment #8576932 - Flags: review?(bobbyholley) → review+
Attachment #8576935 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8576983 - Flags: review?(jwalden+bmo) → review+
> 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.
Blocks: 1122993
You need to log in before you can comment on or make changes to this bug.