890576 - Debugger onNewGlobalObject hook shouldn't be fallible

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Jim Blandy :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.)

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 1 - Disallow resumption values in onNewGlobalObject hooks. v1

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 2 - Make onNewGlobalObject infallible. v1

Comment 4

[Jim Blandy :jimb]

Comment on attachment 780114 [details] [diff] [review]
Part 1 - Disallow resumption values in onNewGlobalObject hooks. v1

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

Wonderful.

Comment 5

[Jim Blandy :jimb]

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 6

[Jim Blandy :jimb]

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?

Comment 7

[Jim Blandy :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.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Make onNewGlobalObject infallible. v1

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 780588 [details] [diff] [review]
Make onNewGlobalObject infallible. v1

Oops, wrong patch/bug.

Comment 10

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/71fd6f8348c4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a3cb2b1f0c44

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/71fd6f8348c4
https://hg.mozilla.org/mozilla-central/rev/a3cb2b1f0c44