Closed Bug 1062077 Opened 8 years ago Closed 8 years ago

Unify xpc::SystemErrorReporter and NS_ScriptErrorReporter

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files)

Buckle up.
Depends on: 1062631
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.
Attachment #8484781 - Flags: review?(bzbarsky)
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.
Attachment #8484784 - Flags: review?(bzbarsky)
We keep the latter name because it's referenced in more places. Error reporters
will go away entirely in bug 981187.
Attachment #8484785 - Flags: review?(bzbarsky)
In the past this might theoretically have switched us from
NS_ScriptErrorReporter to xpc::SystemErrorReporter, but those have now been
merged.
Attachment #8484786 - Flags: review?(bzbarsky)
I won't get to this until Monday or more likely Tuesday.
Comment on attachment 8484781 [details] [diff] [review]
Part 1 - Introduce xpc::ErrorReport. v1

r=me
Attachment #8484781 - Flags: review?(bzbarsky) → review+
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...
Attachment #8484782 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #8484783 - Flags: review?(bzbarsky) → review+
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
Attachment #8484784 - Flags: review?(bzbarsky) → review+
Comment on attachment 8484785 [details] [diff] [review]
Part 5 - Merge NS_ScriptErrorReporter and xpc::SystemErrorReporter. v1

r=me
Attachment #8484785 - Flags: review?(bzbarsky) → review+
Comment on attachment 8484786 [details] [diff] [review]
Part 6 - Remove now-no-op error reporter munging in mozJSSubScriptLoader.cpp. v1

r=me
Attachment #8484786 - Flags: review?(bzbarsky) → review+
Thanks for the reviews, Boris!

Full try push: https://tbpl.mozilla.org/?tree=Try&rev=4fb3317d59fd
You need to log in before you can comment on or make changes to this bug.