1070842 - Prototype new AutoJSAPI-based error handling

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

I was originally planning on using the entirety of bug 1070049 for the prototyping phase, but I think it involves changing a bit too many consumes, so I'm de-scoping it a little bit. These patches will fix Bill's e10s problem in bug 1067574 if he doesn't decide to spot-fix it sooner.

I've already got a green-ish try run here: https://tbpl.mozilla.org/?tree=Try&rev=36d6bff7107d

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=36d6bff7107d

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=e0e24ccac715

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove ambiguous globals from xpc::ErrorReporter and make meanings explicit. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Factor script error event dispatch into a separate function. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Introduce an API on AutoJSAPI to allow Gecko to handle exceptions without meddling from the JS engine. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Move nsJSUtils::CompileFunction (and consequently event handler compilation and XBL compilation) off of nsJSUtils::ReportPendingException. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Switch to new-style exception reporting in LoadFrameScriptInternal. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Tests. v1

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493090 [details] [diff] [review]
Part 1 - Remove ambiguous globals from xpc::ErrorReporter and make meanings explicit. v1

This seems like mainly undoing earlier pushing down of this stuff into ErrorReport, right?  I assume there's a good reason for this...

>+                 ? "chrome worker javascript" : "content worker javascript";

Where are those category strings coming from?  That's not what they used to be, afaict; please make this match the old behavior without the "worker" in there.

r=me with that fixed.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493092 [details] [diff] [review]
Part 2 - Factor script error event dispatch into a separate function. v1

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493093 [details] [diff] [review]
Part 3 - Introduce an API on AutoJSAPI to allow Gecko to handle exceptions without meddling from the JS engine. v1

>+      const char *category = nsContentUtils::IsCallerChrome() ? "chrome javascript"
>+                                                              : "content javascript";

This doesn't seem like an OK thing to do on a web worker thread, right?  I guess this is guarded on the asserts in TakeOwnershipOfErrorReporting(), but see below.

I'm not terribly happy with the number of places those strings are scattered now; I'd _really_ like to recentralize this in the ErrorReport code.  Why can we not do that (e.g. passing in a boolean and having ErrorReport do the string bits)?

>+  if (mOldErrorReporter) {
>+    JS_SetErrorReporter(JS_GetRuntime(cx()), mOldErrorReporter);

This seems wrong without some comments explaining why it's OK.  Say our ctor was called on the main thread when we had no error reporter on the runtime.  We'd set the error reporter to SystemErrorReporter, and then not unset it here.

But also, why is this error reporter munging mainthread-only?  Why is it not needed on worker threads?  Conversely, why _is_ it needed on mainthread?

>+// Even with dontReportUncaught, the JS engine still sends error reports

You mean "warning reports"?

>+  MOZ_ASSERT(NS_IsMainThread(), "Can't own error reporting off-main-thread yet");

This seems like a pretty serious limitation, esp. given the amount of b2g code we have that afaict doesn't have sane debug test coverage and hence would call this function without triggering the assertion.  What prevents us from supporting this on workers as well as mainthread?

r=me with at least followup bugs filed for the above issues...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493094 [details] [diff] [review]
Part 4 - Move nsJSUtils::CompileFunction (and consequently event handler compilation and XBL compilation) off of nsJSUtils::ReportPendingException. v1

r=me, but can we please add a deleted copy ctor to AutoJSAPI to make sure no one accidentally passes it by value?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493095 [details] [diff] [review]
Part 5 - Switch to new-style exception reporting in LoadFrameScriptInternal. v1

Seems like we don't need the hoisted "ok" declaration anymore.

r=me with that removed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8493096 [details] [diff] [review]
Part 6 - Tests. v1

r=me

Comment 15

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11) 
> I'm not terribly happy with the number of places those strings are scattered
> now; I'd _really_ like to recentralize this in the ErrorReport code.  Why
> can we not do that (e.g. passing in a boolean and having ErrorReport do the
> string bits)?

The main thing I was trying to avoid was storing a nebulous "mIsChrome" member in xpc::ErrorReport. My fear was that we'd be lax about it (since it's only actually used to compute the category string) and then at some point later somebody might use it for something more serious. I also thought it would be useful to have more varied category strings. But assuming that's a non-goal, I can definitely just pass aIsChrome and compute mCategory immediately in xpc::ErrorReport::Init.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #11)
> >+  if (mOldErrorReporter) {
> >+    JS_SetErrorReporter(JS_GetRuntime(cx()), mOldErrorReporter);
> 
> This seems wrong without some comments explaining why it's OK.  Say our ctor
> was called on the main thread when we had no error reporter on the runtime. 
> We'd set the error reporter to SystemErrorReporter, and then not unset it
> here.

Yeah good point - I'll use a Maybe<>. I guess nullptr is basically the only error reporter left that isn't xpc::SystemErrorReporter. ;-)
 
> But also, why is this error reporter munging mainthread-only?  Why is it not
> needed on worker threads?  Conversely, why _is_ it needed on mainthread?

> >+  MOZ_ASSERT(NS_IsMainThread(), "Can't own error reporting off-main-thread yet");
> 
> This seems like a pretty serious limitation, esp. given the amount of b2g
> code we have that afaict doesn't have sane debug test coverage and hence
> would call this function without triggering the assertion.  What prevents us
> from supporting this on workers as well as mainthread?

We will get there, but there's a fair amount more machinery involved. We'd need to hook into the top-level error handling machinery on workers (which is significantly different than it is on the main thread), and workers aren't really using AutoJSAPI much yet at all. This patch is an incremental step, and I want to avoid trying to solve all the problems at once.

At some point, we'll be able to flip the switch so that AutoJSAPI does all this by default in all the cases, but we're not there yet.

> r=me with at least followup bugs filed for the above issues...

The followup bug is bug 1072144, basically.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> But assuming that's a non-goal

It's a non-goal unless we update our devtools/console UI in tandem, I'd think.

> I can definitely just pass aIsChrome and compute mCategory immediately in
> xpc::ErrorReport::Init

Sounds great, thanks.

Comment 18

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=1686e7d2ca58

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0627cc72985f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e84e6017e656
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c8b6d2fd42f0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7e4dde009b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c7b542dbc2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8b19763dd5f1

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0627cc72985f
https://hg.mozilla.org/mozilla-central/rev/e84e6017e656
https://hg.mozilla.org/mozilla-central/rev/c8b6d2fd42f0
https://hg.mozilla.org/mozilla-central/rev/ed7e4dde009b
https://hg.mozilla.org/mozilla-central/rev/b3c7b542dbc2
https://hg.mozilla.org/mozilla-central/rev/8b19763dd5f1