Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks

RESOLVED FIXED in mozilla25

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 780185 [details] [diff] [review]
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.
Attachment #780185 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Depends on: 897676
(Assignee)

Comment 3

5 years ago
Created attachment 780592 [details] [diff] [review]
Redesign JS_NewGlobalObject API to sanely handle onNewGlobalObject debugger hooks. v3

A few minor fixes to the previous patch.
Attachment #780185 - Attachment is obsolete: true
Attachment #780185 - Flags: review?(jorendorff)
Attachment #780592 - Flags: review?(jorendorff)
(Assignee)

Comment 4

5 years ago
 q
(Assignee)

Comment 5

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

Indeed.

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

Comment 6

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

Updated

5 years ago
Blocks: 885301
(Assignee)

Comment 8

5 years ago
Created attachment 784066 [details] [diff] [review]
Part 1 - Match up the script global and compile-and-go global when cloning function scripts. v1
Attachment #784066 - Flags: review?(jimb)
(Assignee)

Comment 9

5 years ago
Created attachment 784067 [details] [diff] [review]
Part 2 - Assert when scripts are created that we've fired onNewGlobalObject. v1
Attachment #784067 - Flags: review?(jimb)
(Assignee)

Comment 10

5 years ago
Created attachment 784068 [details] [diff] [review]
Part 3 - Allow callers to manually fire OnNewGlobalObject when bootstrapping is complete. v1
Attachment #784068 - Flags: review?(luke)

Comment 11

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

Comment 12

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

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

Updated

5 years ago
Attachment #784067 - Flags: review?(jimb) → review+
https://hg.mozilla.org/mozilla-central/rev/ecd30f33574b
https://hg.mozilla.org/mozilla-central/rev/e6c4e8428664
https://hg.mozilla.org/mozilla-central/rev/72386d4f6797
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 901658
You need to log in before you can comment on or make changes to this bug.