897322 - Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Right now, JS_NewGlobalObject fires off debugger notifications before returning the global object to the caller. But in general, globals aren't ready to be used immediately after they're created, since the hooks associated with the JSClass expect certain reserved slots to be set up. So if we fire debugger notifications and manipulate the nascent global with arbitrary script, the half-baked global may run afoul of various invariants, causing us to crash.

In general, this is a pretty tricky problem to solve elegantly. The best idea I've come up with is to allow callers of JS_NewGlobalObject to pass a reference to an RAII class, JSAutoFireOnNewGlobalObject, which JS_NewGlobalObject will arm with the global. This gives callers the flexibility to declare the class at the appropriate location up the callstack, so that the notifications will be fired when it goes out of scope. I'll work up some patches now.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks. v1

I was tempted to add an overload that just scoped the cannon to the call so as
to avoid fixing up the jillion callsites, but I think this is important enough
that all API consumers should be forced to think about it.

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=97837e9da5a6

Comment 3

[Bobby Holley (:bholley)]

Attachment: Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks. v3

A few minor fixes to the previous patch.

Comment 4

[Bobby Holley (:bholley)]

 q

Comment 5

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #4)
>  q

Indeed.

https://tbpl.mozilla.org/?tree=Try&rev=e27daa4eb768

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 780592 [details] [diff] [review]
Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks. v3

Discussed this with luke and sfink on IRC. Because of the potential for intermediate failures between global creation and the global being fully-baked, we decided that the callers should explicitly call a JS_FireOnNewGlobalObject() method instead at the appropriate time. We can enforce this by asserting on onNewScript that we've fired the notification for the global.

Comment 7

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=f36331d8e511

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Match up the script global and compile-and-go global when cloning function scripts. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Assert when scripts are created that we've fired onNewGlobalObject. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Allow callers to manually fire OnNewGlobalObject when bootstrapping is complete. v1

Comment 11

[Jim Blandy :jimb]

Comment on attachment 784066 [details] [diff] [review]
Part 1 - Match up the script global and compile-and-go global when cloning function scripts. v1

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

This sure looks right to me. I hope Try agrees.

Can we get a regression test for this?

Comment 12

[Bobby Holley (:bholley)]

(In reply to Jim Blandy :jimb from comment #11)
> Can we get a regression test for this?

The XPConnect tests already hit the assertions in this patch without the fix. I think that's enough.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 784068 [details] [diff] [review]
Part 3 - Allow callers to manually fire OnNewGlobalObject when bootstrapping is complete. v1

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

::: js/src/jsapi.h
@@ +3205,5 @@
>          return *this;
>      }
>  };
>  
> +enum OnNewGlobalObjectDelegation {

Enums like these tend to be named XOption, so I'd call it "OnNewGlobalHookOption".

@@ +3208,5 @@
>  
> +enum OnNewGlobalObjectDelegation {
> +    CallerFiresHook,
> +    CalleeFiresHook,
> +    DontFireHook

I think it'd be a little more direct to have 2 enumerators:
  FireOnNewGlobalHook
  DontFireOnNewGlobalHook
These names answer the question "which hook?" and avoid having three enumerators for an inherently binary option.

Lastly, could you add a comment to the enum explaining the potential danger of too-early onNewGlobalHook and the solution?

Comment 14

[Bobby Holley (:bholley)]

Fixed the jit-test failures, which makes this green on try.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd30f33574b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e6c4e8428664
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/72386d4f6797

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ecd30f33574b
https://hg.mozilla.org/mozilla-central/rev/e6c4e8428664
https://hg.mozilla.org/mozilla-central/rev/72386d4f6797