Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug)

unspecified
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
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.
Attachment #780185 - Flags: review?(jorendorff)
Depends on: 897676
A few minor fixes to the previous patch.
Attachment #780185 - Attachment is obsolete: true
Attachment #780185 - Flags: review?(jorendorff)
Attachment #780592 - Flags: review?(jorendorff)
 q
(In reply to Bobby Holley (:bholley) from comment #4)
>  q

Indeed.

https://tbpl.mozilla.org/?tree=Try&rev=e27daa4eb768
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.
Attachment #780592 - Flags: review?(jorendorff)
Blocks: 885301
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?
Attachment #784066 - Flags: review?(jimb) → review+
(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 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?
Attachment #784068 - Flags: review?(luke) → review+
Attachment #784067 - Flags: review?(jimb) → review+
Depends on: 901658
You need to log in before you can comment on or make changes to this bug.