1062077 - Unify xpc::SystemErrorReporter and NS_ScriptErrorReporter

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Buckle up.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Introduce xpc::ErrorReport. v1

I've got it! Let's just wrap the JS thing in an XPCOM thing that duplicates
its fields with a different ownership model! We totally need another one of
those, right? And we could even stick it in XPConnect! </sarcasm>

In seriousness - the code to own, format, and display error reports is
currently spread between the JS engine, JSErrorReporters, an async Runnable
abstraction, and elsewhere. We need to condense it somewhere to start chipping
away at this mess.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist console logging duties into xpc::ErrorReport. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Switch SystemErrorReporter to xpc::ErrorReport. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Report exceptions immediately for non-window globals, and assert that we have a window in ScriptErrorEvent. v1

Note that this is a no-op right now, because NS_ScriptErrorReporter currently
relies on nsIScriptContext to get its global, and thus will only ever operate
on window globals. But that will change once we merge this code with that in
xpc::SystemErrorReporter.

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Merge NS_ScriptErrorReporter and xpc::SystemErrorReporter. v1

We keep the latter name because it's referenced in more places. Error reporters
will go away entirely in bug 981187.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 6 - Remove now-no-op error reporter munging in mozJSSubScriptLoader.cpp. v1

In the past this might theoretically have switched us from
NS_ScriptErrorReporter to xpc::SystemErrorReporter, but those have now been
merged.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I won't get to this until Monday or more likely Tuesday.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484781 [details] [diff] [review]
Part 1 - Introduce xpc::ErrorReport. v1

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484782 [details] [diff] [review]
Part 2 - Hoist console logging duties into xpc::ErrorReport. v1

This introduces a behavior change.  Before this patch, the NSPR and stderr logging happened no matter what.  After this change it only happens if the window's onerror event handler doesn't suppress the error report (which is when we go ahead and log to console).

I'm probably OK with this change, though it might be nice to preserve the old behavior...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484782 [details] [diff] [review]
Part 2 - Hoist console logging duties into xpc::ErrorReport. v1

Actually, this also stops logging to stderr if there is no console service.  I expect we'd want to fix that, at the very least.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484783 [details] [diff] [review]
Part 3 - Switch SystemErrorReporter to xpc::ErrorReport. v1

r=me assuming the stderr bits from part 2 are fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484784 [details] [diff] [review]
Part 4 - Report exceptions immediately for non-window globals, and assert that we have a window in ScriptErrorEvent. v1

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484785 [details] [diff] [review]
Part 5 - Merge NS_ScriptErrorReporter and xpc::SystemErrorReporter. v1

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8484786 [details] [diff] [review]
Part 6 - Remove now-no-op error reporter munging in mozJSSubScriptLoader.cpp. v1

r=me

Comment 17

[Bobby Holley (:bholley)]

Thanks for the reviews, Boris!

Full try push: https://tbpl.mozilla.org/?tree=Try&rev=4fb3317d59fd

Comment 18

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4859464f9e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c905979c161e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/67e8b7d84de8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3568bcb8b6c6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/519e6a37545d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8da12b68dc

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cf4859464f9e
https://hg.mozilla.org/mozilla-central/rev/c905979c161e
https://hg.mozilla.org/mozilla-central/rev/67e8b7d84de8
https://hg.mozilla.org/mozilla-central/rev/3568bcb8b6c6
https://hg.mozilla.org/mozilla-central/rev/519e6a37545d
https://hg.mozilla.org/mozilla-central/rev/1b8da12b68dc