Closed Bug 889911 Opened 7 years ago Closed 7 years ago

Unify non-DOM JSErrorReporters

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25
Tracking Status
firefox24 --- fixed
firefox25 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files)

I'm pretty deep down a rabbit hole, but this needs to happen at some point. So if it turns out to be be straightforward, I might as well take a crack at it.

We have a bunch of different error reporters scattered around for things like mozJSComponentLoader, xpcshell, SafeJSContext, XPCShellEnvironment, and more. These get applied willy-nilly to various JSContexts, which is a concept that's going away.

We should instead implement a universal "SystemErrorReporter" to be used in situations where there's no DOM window in sight. It should report to the console, dump to the terminal, and allow consumers to register observer notifications for any custom behavior they might want.

Hopefully I can do this without braking any automation log parsing. CCing clint, who can hopefully CC the most appropriate person who knows about that stuff.
When we start sending everything reported to the JSErrorReporter to the console,
these tests end up getting confused by warnings.
Attachment #773614 - Flags: review?(mrbkap)
PCShell currently overrides all the JSContexts whose creation it observes with
its own custom error reporter. This reporter does all sorts of funny things which
we try to clean up for the most part. But there are a few very intricate
considerations at play.

First, the old xpcshell error reporter does some mumbo jumbo with the
XPCCallContext stack to try to guess whether some other code might catch the
exception. This is total garbage on a number of fronts, particularly because
the XPCCallContext stack has no concept of saved frame chains, nested event
loops, sandbox boundaries, origin boundaries, or any of the myriad of
complicating factors that determine whether or not an exception will propagate.

So we get rid of it. But this causes some crazy debugger tests to fail, because
they rely on an exception from uriloader/exthandler/nsHandlerService.js getting
squelched, and can't handle anybody reporting errors to the console service at
the particular moment of contortionism when the exception is raised. So we need
to introduce an explicit mechanism to disable the error reporter here to keep
things running.

Second, we have to be very careful about tracking the return status of the
xpcshell binary. The old code would simply flag an error code if the error
handler was invoked, and we can mostly continue to do that. But there are some
complications. See the comments.

Finally, we don't anything analogous in XPCShellEnvironment, because I have
patches in bug 889714 to remove its state-dependence on the error reporter.
I'll switch it to SystemErrorReporter in that bug.
Attachment #773615 - Flags: review?(mrbkap)
Comment on attachment 773612 [details] [diff] [review]
Part 1 - Introduce xpc::SystemErrorReporter, roughly based on mozJSComponentLoader's error reporter. v1

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +260,5 @@
> +              uclinebuf ? nsDependentString(uclinebuf) : EmptyString(),
> +              rep->lineno, column, rep->flags,
> +              "system javascript");
> +        if (NS_SUCCEEDED(rv)) {
> +            rv = consoleService->LogMessage(errorObject);

Nits: no need for the braces and there doesn't seem to be a reason to grab rv from the call to LogMessage (unless you want to warn if it failed).
Attachment #773612 - Flags: review?(mrbkap) → review+
Attachment #773613 - Flags: review?(mrbkap) → review+
Attachment #773614 - Flags: review?(mrbkap) → review+
Attachment #773615 - Flags: review?(mrbkap) → review+
It turns out that adding NS_EXPORT_ to an error reporter makes it incompatible with the JSErrorReporter type on windows. I made a separate external wrapper that can be used by xpcshell.

https://tbpl.mozilla.org/?tree=Try&rev=54bf0deb13cc
I effed up the last push with some rebase fail, which we backed out. Trying again:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3745b7a8bfb9
Depends on: 896738
(In reply to Bobby Holley (:bholley) from comment #18)
> https://hg.mozilla.org/releases/mozilla-aurora/
> pushloghtml?changeset=84b828b63115

Backed out from Aurora for possibly causing xpcshell crashes along with the other changes from bholley's push in https://hg.mozilla.org/releases/mozilla-aurora/rev/659b0d61fbc6
Depends on: 896756
Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.