Closed
Bug 889911
Opened 11 years ago
Closed 11 years ago
Unify non-DOM JSErrorReporters
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Whiteboard: [qa-])
Attachments
(4 files)
3.92 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
9.09 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
12.90 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=f5818f00d1b8
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=dc18ce15eb73
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=6f4344b93e3c
Assignee | ||
Comment 4•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=624100c096d4
Assignee | ||
Comment 5•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=149a8204b37f
Assignee | ||
Comment 6•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=4f9b6e6e06ab
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #773612 -
Flags: review?(mrbkap)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #773613 -
Flags: review?(mrbkap)
Assignee | ||
Comment 9•11 years ago
|
||
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)
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #773613 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #773614 -
Flags: review?(mrbkap) → review+
Updated•11 years ago
|
Attachment #773615 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 12•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4ec71ab768f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6e48a408d1de remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e296989dd3d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e55ddfc9dc3
Comment 13•11 years ago
|
||
Backed out for Windows bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/a28ade3d4bdc https://tbpl.mozilla.org/php/getParsedLog.php?id=25291255&tree=Mozilla-Inbound
Assignee | ||
Comment 14•11 years ago
|
||
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
Assignee | ||
Comment 15•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7060ab1588 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/18537c4436c7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5f62284917 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/234bd6d1fbed
Assignee | ||
Comment 16•11 years ago
|
||
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
Comment 17•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/92f89ea3430b https://hg.mozilla.org/mozilla-central/rev/3194ab9959a7 https://hg.mozilla.org/mozilla-central/rev/57382afabfc5 https://hg.mozilla.org/mozilla-central/rev/8167594299bd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Assignee | ||
Comment 18•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=84b828b63115
status-firefox24:
--- → fixed
Updated•11 years ago
|
status-firefox25:
--- → fixed
(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
Assignee | ||
Comment 20•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=5f9484e134f9
Comment 21•11 years ago
|
||
Backed out for xpcshell failures. https://hg.mozilla.org/releases/mozilla-aurora/rev/b3d0c2498b42
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?changeset=3266c1d73816
Comment 23•11 years ago
|
||
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.
Description
•