1100498 - Get error messages out of the js addon compartment to determine if we need to add shims

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Allison Naaktgeboren :ally]

In Bug 1080212 - [e10s] Add Telemetry probe for js exceptions that occur in the addon js compartment , we get the addonid, the filename, and the line number of the error, but we don't get the actual message due to privacy concerns. (see other bug for more info)

The existing data will help tell us about addons that are particularly problematic. However it does not help us identify common errors.

If we know what frequently used functions throw, we'd write a shim,"automagically" making a function work in e10s.

It may need to be restricted access data because we don't know what's in those error strings in the wild.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Bill, do you still want to do this?

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Giorgio, renom this if you think it is going to be useful

Comment 4

[Giorgio Maone [:ma1]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> Giorgio, renom this if you think it is going to be useful

Brad, I'd like to have this not just for shims, but also to have more "hot spots" to look at first when examining other developers' code to help transition.

Comment 5

[Gabor Krizsanits (INACTIVE)]

So I wrote a patch that gets the function name from the stack but when I tried testing it, it turned out that the whole exception report mechanism is broken in most of the cases.

Let's see:
1) exnObject->compartment() (http://mxr.mozilla.org/mozilla-central/source/js/src/jsexn.cpp#693)
This won't tell the compartment of the exception, the exception is wrapped for the context's current compartment (callee), so I guess we need an UncheckedUnwrap here, depending on our intention.
I'm afraid this is the reason why we have not seen any useful exceptions reported :(


2) The bigger issue is that in many cases the addon does something like:
Services.obs.addObserver(observe, "document-element-inserted", false);
or it can be some event listener, it does not really matter. Important part
is that the observe function here is a pure js function but to call it from
native code we wrap it into a nsXPCWrappedJSClass.

So nsXPCWrappedJSClass::CallMethod prevents exception to be reported (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#1210), observe is called, it might call other functions where the exception happens. We unwind the stack. Get back to CallMethod, the stack is empty again and we are in the system compartment. Then we report the exception (http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCWrappedJSClass.cpp#1245)

Problem is that at this point the JS stack in empty (already unwinded). So I have no way to tell the callee of the function where the exception occurred. Maybe we could add another slot to ErrorObject (scope/callee)? Or is there a way to tell the JS engine to save the callstack when an exception occurs and reporting is prevented?

Flagging Bobby since I think he worked on xpconnect exceptions quite a bit...

Comment 6

[Bobby Holley (:bholley)]

In general the XPCWrappedJSClass stuff for exception reporting is kind of a war zone - tweaking it always breaks something else. I started working on cleaning it up about a year ago, but got pulled away to work on other stuff.

JS exceptions should now have a .stack property which is a SavedStack JS object (it's even Xrayable). You almost certainly want to access that property on your exception - it should give you all the data you need. fitzgen made it, and may also be able to answer questions.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that .stack is still the stringified stack string, but it would be easy enough to add a JSAPI method to grab a reference to the underlying SavedFrame object.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Attachment: report function names for addon exceptions. v1

I think the sanest thing to do is to check the scope of the youngest function on the stack to determine if the exception is from an addon scope. Luckily when a JS exception object is created that stack is saved and can be access later as Bobby and Nick pointed it out. What do you think? (I'm not super happy with the current version yet, but the original was broken for sure, so we need to fix it)

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8617284 [details] [diff] [review]
report function names for addon exceptions. v1

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

This looks good. All I can think to do is to move it into a separate function.

Comment 10

[Gabor Krizsanits (INACTIVE)]

Attachment: part2: ReportAddonExceptionToTelementry. v1

(In reply to Bill McCloskey (:billm) from comment #9)
> This looks good. All I can think to do is to move it into a separate
> function.

Since that's just moving a big code block around I think it's easier to review if I file another patch for it, and reflag the original for review as well.

Comment 11

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=707c7a1216c4

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8621521 [details] [diff] [review]
part2: ReportAddonExceptionToTelementry. v1

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

::: js/src/jsexn.cpp
@@ +684,5 @@
> +    RootedObject unwrapped(cx, UncheckedUnwrap(exnObject));
> +    MOZ_ASSERT(unwrapped, "UncheckedUnwrap failed?");
> +
> +    // There is not much we can report if the exception is not an ErrorObject, let's ignore those.
> +    if (unwrapped->is<ErrorObject>()) {

One of the benefits of making this a separate function is that you can |return;| if this check fails. Then you can reduce the indentation of everything below, which makes it easier to read. So let's do that.

@@ +691,5 @@
> +        // Let's ignore TOP level exceptions. For regular add-ons those will not be reported anyway,
> +        // for SDK based once it should not be a valid case either.
> +        // At this point the frame stack is unwinded but the exception object stored the stack so let's
> +        // use that for getting the function name.
> +        if (stack) {

Same here.

@@ +696,5 @@
> +            JSCompartment* comp = stack->compartment();
> +            JSAddonId* addonId = comp->addonId;
> +            // We only want to send the report if the scope that just have thrown belongs to an add-on.
> +            // Let's check the compartment of the youngest function on the stack, to determine that.
> +            if (addonId) {

And here.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8617284 [details] [diff] [review]
report function names for addon exceptions. v1

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

::: js/src/jsexn.cpp
@@ +698,5 @@
> +            Rooted<ErrorObject*> errObj(cx, &unwrapped->as<ErrorObject>());
> +            RootedObject stack(cx, errObj->stack());
> +            // Let's ignore TOP level exceptions. For regular add-ons those will not be reported anyway,
> +            // for SDK based once it should not be a valid case either.
> +            // At this point the frame stack is unwinded but the exception object stored the stack so let's

unwinded -> unwound

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cba58b2e1b4

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=10777460&repo=mozilla-inbound

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdf0d931ed8

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e46d0a7d8794

Comment 18

[Gabor Krizsanits (INACTIVE)]

It was a missing null check for the function name.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e46d0a7d8794