Prototype new AutoJSAPI-based error handling

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Depends on 1 bug)

unspecified
mozilla35
x86
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Assignee

Description

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

Updated

5 years ago
Depends on: 1070131
Assignee

Updated

5 years ago
No longer blocks: 1070049
Assignee

Comment 8

5 years ago
Attachment #8493096 - Flags: review?(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.
Attachment #8493090 - Flags: review?(bzbarsky) → review+
Comment on attachment 8493092 [details] [diff] [review]
Part 2 - Factor script error event dispatch into a separate function. v1

r=me
Attachment #8493092 - Flags: review?(bzbarsky) → review+
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...
Attachment #8493093 - Flags: review?(bzbarsky) → review+
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?
Attachment #8493094 - Flags: review?(bzbarsky) → review+
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.
Attachment #8493095 - Flags: review?(bzbarsky) → review+
Comment on attachment 8493096 [details] [diff] [review]
Part 6 - Tests. v1

r=me
Attachment #8493096 - Flags: review?(bzbarsky) → review+
Assignee

Comment 15

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

Comment 16

5 years ago
(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.
> 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.
Depends on: 1077045
Assignee

Updated

5 years ago
Blocks: 1070049
Assignee

Updated

5 years ago
Depends on: 1107684

Updated

4 years ago
Depends on: 1127620
Depends on: 1147215
You need to log in before you can comment on or make changes to this bug.