Closed Bug 890576 Opened 6 years ago Closed 6 years ago

Debugger onNewGlobalObject hook shouldn't be fallible

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 1 obsolete file)

This was one of the issues I noticed in bug 885388, which I'm splitting into its own bug.

The current model allows onNewGlobalObject to throw, which in turn causes JS_NewGlobalObject to throw arbitrary scripted exceptions. This is extremely undesirable from the browser perspective, and creates all sorts of headaches. I've convinced jorendorff that JS_NewGlobalObject should be infallible modulo OOM.

I ran into some issues while trying to make the hook infallible, though. Currently, onNewGlobalObject is expected to return a resumption value, and there are a number of tests that painstakingly verify that certain onNewGlobalObject handlers can cancel other ones. The reasoning for this isn't really clear to me, but I'll assume there is one.

However, that leaves us with the tricky situation when we want to make this hook infallible. Do we want to allow only a subset of resumption values (disallowing Error and Throw)? Or something else? I'm unsure enough about this that I'll wait for jimb's feedback before moving forward here.
Flags: needinfo?(jimb)
Rationale for the inter-Debugger onNewGlobalObject handler tests:
- You can have multiple Debugger instances listening for new globals.
- If a Debugger has removed its callback, it should not be called.
- One Debugger's callback can change another Debugger's hooks.

If we're going to make them infallible, then we should probably call the uncaughtExceptionHook if an onNewGlobalHook returns anything other than 'undefined', with an error saying that the hook isn't permitted to return any other kind of resumption value. (That'll need to be documented, too.)
Flags: needinfo?(jimb)
Blocks: 897322
Comment on attachment 780114 [details] [diff] [review]
Part 1 - Disallow resumption values in onNewGlobalObject hooks. v1

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

Wonderful.
Attachment #780114 - Flags: review?(jimb) → review+
Comment on attachment 780114 [details] [diff] [review]
Part 1 - Disallow resumption values in onNewGlobalObject hooks. v1

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

Wonderful.

::: js/src/jit-test/tests/debug/Debugger-onNewGlobalObject-10.js
@@ +16,5 @@
>      return { throw: "snoo" };
>    return undefined;
>  };
> +dbg2.uncaughtExceptionHook = function (exn) {
> +  assertEq(/disallowed/.test(exn), true);

Oh --- it would be nice to put something in 'log' here.
Comment on attachment 780115 [details] [diff] [review]
Part 2 - Make onNewGlobalObject infallible. v1

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

::: js/src/jit-test/tests/debug/Debugger-onNewGlobalObject-11.js
@@ +1,1 @@
> +// Resumption values from uncaughtExceptionHook from onNewGlobalObject handlers are ignored (other than cancelling further hooks).

"Resumption values other than undefined", I would say.

Shall we break this comment into two lines?

::: js/src/jsapi.cpp
@@ +3365,5 @@
> +    // This hook is infallible, because we can't really throw at this point.
> +    // The global object is already created and in the heap, which is
> +    // externally observable since the finalize hook will be called when the
> +    // global is GCed.
> +    Debugger::onNewGlobalObject(cx, global);

What about OOM and slow scripts?
Attachment #780115 - Flags: review?(jimb)
Comment on attachment 780115 [details] [diff] [review]
Part 2 - Make onNewGlobalObject infallible. v1

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

::: js/src/vm/Debugger.cpp
@@ +1367,5 @@
> +        // decides to raise an error, we want to at least avoid invoking the rest
> +        // of the onNewGlobalObject handlers in the list (not for any super
> +        // compelling reason, just because it seems like the right thing to do).
> +        // So we ignore whatever comes out in |value|, but break out of the loop
> +        // if a non-success trap status is returned.

I think we also need to explain that it's okay to eat OOM / slow script termination here because they'll get raised again later.
Attachment #780115 - Flags: review+
Attachment #780588 - Flags: review?(mrbkap)
Comment on attachment 780588 [details] [diff] [review]
Make onNewGlobalObject infallible. v1

Oops, wrong patch/bug.
Attachment #780588 - Attachment is obsolete: true
Attachment #780588 - Flags: review?(mrbkap)
https://hg.mozilla.org/mozilla-central/rev/71fd6f8348c4
https://hg.mozilla.org/mozilla-central/rev/a3cb2b1f0c44
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.