Closed Bug 1172246 Opened 9 years ago Closed 8 years ago

window.onerror catches JSON.parse error in Promise fulfillment

Categories

(Core :: DOM: Core & HTML, defect)

38 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: me, Assigned: bzbarsky)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/42.0.2311.135 Safari/537.36

Steps to reproduce:

http://jsbin.com/gomiliyevu/1/edit?js,console

ref: https://github.com/github/fetch/pull/157


Actual results:

I'm unclear if this captured in any spec. However, behavior between vendors varies. With the above jsbin, Chrome's global error handler will not receive an error. In Firefox, the error will be logged by `window.onerror`. Oddly enough, if `JSON.parse` is invoked within an anonymous function, `onerror` will not receive the error.


Expected results:

I'd expect behavior between vendors to be consistent.
Component: Untriaged → DOM
Product: Firefox → Core
not sure if it's more related to DOM promises or to the global error handler, so I moved to DOM for now for more triage.
So the issue is that CallSetup says it wants the engine to not report uncaught exceptions, but JSON parsing is using JS_ReportErrorNumber instead of something that would guarantee setting an exception (because SpiderMonkey error reporting, yeah), so it ends up reporting the error directly when no JS is on the stack.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Also, Kurt, thank you for the excellent testcase!
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8616802 [details] [diff] [review]
Make sure CallSetup's handling of exceptions it wants to deal with itself works even when the callable is a JSNative that use the JS_Report*Error APIs instead of throwing exceptions in the usual way

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

::: dom/bindings/CallbackObject.cpp
@@ +187,5 @@
>    // Make sure the JS engine doesn't report exceptions we want to re-throw
>    if ((mCompartment && mExceptionHandling == eRethrowContentExceptions) ||
>        mExceptionHandling == eRethrowExceptions) {
>      mSavedJSContextOptions = JS::ContextOptionsRef(cx);
>      JS::ContextOptionsRef(cx).setDontReportUncaught(true);

You should be able to nix the dontReportUncaught here.
Attachment #8616802 - Flags: review?(bobbyholley) → review+
> You should be able to nix the dontReportUncaught here.

Yeah, looks like I can, since the one consumer checks the other boolean too.  OK, great!
Oh, gah.  Bobby, what's our story here for workers?  Why can't I TakeOwnershipOfErrorReporting there?
Flags: needinfo?(bobbyholley)
I guess the problem is that the HasException() bit in ~AutoJSAPI assumes mainthread...

But in my case I'm guaranteeing I'll handle things myself.  Except I _do_ want to be able to JS_ReportPendingException and have it work correctly, which is not the case with TakeOwnershipOfErrorReporting() because it nukes the error reporter on the runtime.

So I guess I need a new JSContext flag which will make spidermonkey ignore the JS_IsRunning() check but not have any of these other nasty side-effects or something?
Flags: needinfo?(jorendorff)
Or, can we make AutoJSAPI not mess with the error reporter and just JS_ReportPendingException in its destructor instead of trying to reimplement that bit?
Boris and I talked about this over IRC and email.
Flags: needinfo?(jorendorff)
Flags: needinfo?(bobbyholley)
Yeah, so the upshot is that I basically need to fix bug 1072144 before I can make this setup work on workers.  That's... frustrating.  I guess I'll see if I can disentangle that ball of wax.  :(
Depends on: 1072144
Depends on: 1225717
Attachment #8616802 - Attachment is obsolete: true
Comment on attachment 8689087 [details] [diff] [review]
Make sure CallSetup's handling of exceptions it wants to deal with itself works even when the callable is a JSNative that uses the JS_Report*Error APIs instead of throwing exceptions in the usual way

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

::: dom/bindings/CallbackObject.cpp
@@ +266,5 @@
> +            needToDealWithException = false;
> +          } else {
> +            // Put it back; we actually want to report it
> +            JS_SetPendingException(mCx, exn);
> +          }

This seems wrong, especially given the comment. Don't we basically want an api to peak at the exception, rather than stealing it and then re-reporting it?

If it makes sense to add such an API, use it here, and get rid of of the JS_SetPendingException here, r=me with that change. Otherwise ask for re-review I guess.
Attachment #8689087 - Flags: review?(bobbyholley) → review+
> This seems wrong, especially given the comment.

Ah, the comment predates me doing the "steal and put back" thing.

> Don't we basically want an api to peak at the exception

Yes.  I've added such an API (PeekException, with StealException doing that and then if it succeeds a JS_ClearPendingException) and this caller now looks like this:

        if (mAutoEntryScript->PeekException(&exn) &&
            ShouldRethrowException(exn)) {
          mAutoEntryScript->ClearException();
          MOZ_ASSERT(!mAutoEntryScript->HasException());
          mErrorResult.ThrowJSException(mCx, exn);
          needToDealWithException = false;
        }
And backed out again.  Apparently we don't run mochitest-JP on try on the platforms I did try pushes on.

We're failing the assert in dom::WarningOnlyErrorReporter.

The reason that happens is that we end up calling a Promise callback (which goes through CallbackObject stuff).  Then there is a long stack of pure-JS stuff.  Then that pure-JS stuff creates a sandbox, which calls JS_FireOnNewGlobalObject which lands in Debugger::fireNewGlobalObject which calls Debugger::handleUncaughtExceptionHelper which calls JS_ReportPendingException, which of course fatally asserts.  This is in the jetpack-package/addon-sdk/source/test/test-sandbox.js.test test, for my future reference.

So now some questions:

1)  Why is the debugger code trying to report the exception and pretend like nothing happened instead of propagating it up?

2)  If we assume this is in fact the desired behavior, how do we handle it?  I suppose we could have CreateSandboxObject put a new AutoJSAPI on the stack that does NOT take ownership of error reporting before calling JS_FireOnNewGlobalObject.  Or something.  That's basically what nsGlobalWindow::FireOnNewGlobalObject does, since it has no convenient cx.  And I guess I'd need to audit all the other places where we create globals.
Flags: needinfo?(jimb)
Flags: needinfo?(bobbyholley)
So here's another interesting question.  What is the API contract for JS_FireOnNewGlobalObject?  Given it has void return value, does it basically promise that when it returns it does not leave a pending exception on cx?

If so, then doing the separate AutoJSAPI before calling it is definitely the way to go.
In general, Debugger code is never supposed to propagate the errors it finds into debuggee code; debuggee code has no way to anticipate that anything would have been happening at that point at all, let alone handle the error meaningfully. Such errors represent bugs in Firefox, not bugs in content. Code using the Debugger API is always supposed to provide an uncaughtExceptionHook to at least show the problem to the developer using the tool.

I think one bug is that the onNewGlobalObject handler is encountering an error; can you tell what that error is?

The second bug is that the uncaughtExceptionHook handler isn't reporting it in some helpful way. Although, if it's some handler only used by tests, crashing is actually a fine thing to do; then there's only the one bug.

JS_FireOnNewGlobalObject returns void because 1) Debugger code isn't supposed to propagate its errors out, and 2) although Debugger handlers usually return a "continue normally / return with different value / throw / terminate" code, i.e. a JSTrapStatus [1], when we implemented that hook Bobby (I think?) said that really, doing anything other than just proceeding normally would be a huge pain in the neck. So since there was no JSTrapStatus to return, void was the alternative.

[1]: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.h#720
Flags: needinfo?(jimb)
> Debugger code is never supposed to propagate the errors it finds into debuggee code

OK.  So it needs to suppress or report them.  The problem with reporting is that it seems fundamentally incompatible with the goal of 981187: that the embedding code completely controls all exception reporting and the JS engine never guesses when to report things.

But in this case the engine isn't really _guessing_.  So maybe what we need is a hook to allow the engine to communicate to the embedding that it's entering a section from which it will definitely report errors...

> can you tell what that error is?

Sure.  The JSErrorReport looks like this:

(gdb) p aRep->filename
$3 = 0x12c32e9ee "resource://gre/modules/commonjs/toolkit/loader.js -> resource://jetpack-package-tests/jetpack-package/addon-sdk/source/test/test-sandbox.js"
(gdb) p aRep->lineno
$4 = 124
(gdb) pu aRep->ucmessage
$5 = 0x148903270 "can't access lexical declaration `self' before initialization"

The relevant code is:

119 exports['test metadata']  = function(assert) {
120   let dbg = new Debugger();
121   dbg.onNewGlobalObject = function(global) {
122     let metadata = Cu.getSandboxMetadata(global.unsafeDereference());
123     assert.ok(metadata, 'this global has attached metadata');
124     assert.equal(metadata.addonID, self.id, 'addon ID is set');
125 
126     dbg.onNewGlobalObject = undefined;
127   }
128 
129   let fixture = sandbox();
130   let self = require('sdk/self');
131 }

Shu, wha's the right way to fix this code?  Just make "self" a var?

In any case, this might turn the JP tests green but doesn't address the fundamental problem of the interaction between debugger and the embedding here.

> The second bug is that the uncaughtExceptionHook handler isn't reporting it in some
> helpful way

Oh, it's trying to.  It's invoking the error reporter on the runtime.  The problem is that since we're in the "embedding will handle exceptions itself, the JS engine should never report them" mode, the error reporter on the runtime asserts that it only gets warnings, not exceptions.  That's what causes the test failure: the fatal assertion.  ;)
Er, actually needinfo shu for the middle part of comment 23.
Flags: needinfo?(shu)
Actually, wait.  Making "self" a var won't help; this isn't really let-specific.  I filed bug 1226868 on the broken test.
Flags: needinfo?(shu)
I've spun off bug 1227190 on the general problem of debugger interactions and bug 1227192 on the specific issue of JS_FireOnNewGlobalObject.
Depends on: 1227192
Flags: needinfo?(bobbyholley)
https://hg.mozilla.org/mozilla-central/rev/b17966965995
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: