Closed Bug 1080212 Opened 5 years ago Closed 5 years ago

[e10s] Add Telemetry probe for js exceptions that occur in the addon js compartment

Categories

(Firefox Health Report Graveyard :: Data Collection, defect)

defect
Not set

Tracking

(e10sm4+)

RESOLVED FIXED
Firefox 37
Tracking Status
e10s m4+ ---

People

(Reporter: ally, Assigned: ally)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 13 obsolete files)

15.34 KB, patch
billm
: review+
Details | Diff | Splinter Review
17.44 KB, patch
Details | Diff | Splinter Review
to help us with the long tail of addon bustage likely to follow e10s, track with addons throw how many errors in the js compartment for addons.
tracking-e10s: --- → ?
Assignee: nobody → ally
Summary: [e10s] Add telemetry probe for js exceptions that occur in the addon js compartment → [e10s] Add FHR probe for js exceptions that occur in the addon js compartment
What's the process of getting things added to the FHR data set?  "How to get a new probe" says more details to come. https://wiki.mozilla.org/Firefox_Health_Report
Component: General → Data Collection
Flags: needinfo?(benjamin)
Product: Firefox → Firefox Health Report
Please follow the guideline at https://wiki.mozilla.org/Firefox/Data_Collection

I don't think this should be part of the current-FHR. Please focus effort on adding this to Telemetry, which will become the one data-collection system by the time e10s hits release. Then we can decide whether we need/want to collect this data for all users or only prerelease users.
Flags: needinfo?(benjamin)
Summary: [e10s] Add FHR probe for js exceptions that occur in the addon js compartment → [e10s] Add Telemetry probe for js exceptions that occur in the addon js compartment
Attached patch ProbeAddonExceptions - wip (obsolete) — Splinter Review
Now, to find an addon for testing
Attached patch ProbeAddonExceptions wip (obsolete) — Splinter Review
compiles, links, and does not crash on startup.
Attachment #8516185 - Attachment is obsolete: true
While this approach sounded very promising, I think we might be barking up the wrong tree. When I see addon spew to console about exceptions, the js engine is silent. 

For example:
STR
1 install Lightbeam
2 visit a webpage (each one triggers policy.js in the addon to run)

expected
- jsexn.cpp js_ReportUncaughtException to be hit

actual
- 'TypeError: aRequestOrigin is null  policy.js: 94' shows up in console
- no breakpoints in hsexn.cpp are hit
  

I note that breakpoints in jsexn are always hit upon startup, and addonIdptr is always null (not a surprise, but the breakpoints are definitely working)

:billm, I know you've seen the patch before. Is the test case wrong? Is this the wrong approach? Is the shims code somehow interfering?
Flags: needinfo?(wmccloskey)
This appears to work(catching and printing correctly), does this match what you & bholley had in mind?

I want to make sure that's settled before figuring out how to hook telemetry into it.
Attachment #8516389 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8518370 - Flags: feedback?(wmccloskey)
Comment on attachment 8518370 [details] [diff] [review]
ProbeAddonExceptions wip - prints out errors

Yeah, I think this is probably the best place to put this.
Attachment #8518370 - Flags: feedback?(wmccloskey) → feedback+
Onto Part 2! Houston, we have a problem.

Ideally we want the to get out of this a dynamically assembled string (addonid::exceptionstring) to a count. That way we will know what addons are throwing what error and how often.

Telemetry does not seem to support this. Currently its structure is to map predefined strings to ints or predefined int id to ints. The key to mapping must be known ahead of time, and we won't know that. The goal is to find that out! 

The best we could do in this case is count how many generic errors a given firefox throws, without know what extensions threw and what the error was. That doesn't seem very useful to me.

However, the payload formats of telemetry&fhr are being updated, so there is a chance that in the near future there will be a payload format that could work here.
(In reply to please NEEDINFO? :ally  Allison Naaktgeboren from comment #8)
> Onto Part 2! Houston, we have a problem.
> 
> Ideally we want the to get out of this a dynamically assembled string
> (addonid::exceptionstring) to a count. That way we will know what addons are
> throwing what error and how often.
> 
> Telemetry does not seem to support this. Currently its structure is to map
> predefined strings to ints or predefined int id to ints. The key to mapping
> must be known ahead of time, and we won't know that. The goal is to find
> that out! 

That sounds like you want keyed histograms (documented in [1] and nsITelemetry).
They landed last week.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe
(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> That sounds like you want keyed histograms (documented in [1] and
> nsITelemetry).

A keyed HISTOGRAM_COUNT to be more specific.
(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> > That sounds like you want keyed histograms (documented in [1] and
> > nsITelemetry).
> 
> A keyed HISTOGRAM_COUNT to be more specific.

Even though we have no idea what the keys (addon id :: error msg) will be ?
Yes, that is what we added keyed histograms for.
Do you need to do this from C++ or can you do it from JS?

From JS just use a keyed histogram of the COUNT type, for C++ we should at least expose |Accumalate(probeId, key, value)| version to Telemetry.h (which shouldn't be hard).

By the way, are there possible privacy concerns with submitting all of these messages?
Do they possibly contain private paths or URLs etc. that should be sanitized?
(In reply to Georg Fritzsche [:gfritzsche] from comment #12)
> Yes, that is what we added keyed histograms for.
> Do you need to do this from C++ or can you do it from JS?
> 
> From JS just use a keyed histogram of the COUNT type, for C++ we should at
> least expose |Accumalate(probeId, key, value)| version to Telemetry.h (which
> shouldn't be hard).
> 
> By the way, are there possible privacy concerns with submitting all of these
> messages?
> Do they possibly contain private paths or URLs etc. that should be sanitized?

They should contain the path to addon's file & the actual js error. I am not anticipating any user identifying information, but if we start to see identifying data, we can sanitize. 

I'm in c++ fwiw

Will you expose the the new Accumalate?
I'll throw up at least a WIP of exposing it to C++ a bit later.

(In reply to please NEEDINFO? :ally  Allison Naaktgeboren from comment #13)
> > By the way, are there possible privacy concerns with submitting all of these
> > messages?
> > Do they possibly contain private paths or URLs etc. that should be sanitized?
> 
> They should contain the path to addon's file & the actual js error. I am not
> anticipating any user identifying information, but if we start to see
> identifying data, we can sanitize. 

Is the patch relative to the profile directory? If so, we may want to sanitize that. We definitely have some prior art on that around (e.g. in the update hotfix).
(In reply to Georg Fritzsche [:gfritzsche] from comment #15)
> Created attachment 8520040 [details] [diff] [review]
> Quick hack for keyed Accumulate()

This has a quick example in nsPluginHost::ReloadPlugins(), which is e.g. triggered by |navigator.plugins.refresh()| in the web console.
Depends on: 1096785
I'm concerned about the level of detail that would be in the histogram "key". In general I'd expect the key to be only the addon ID or some other well-known piece of data. If we need more than that, we should be very specific about the data and how we're certain it's not identifying data.
Attached patch ProbeAddonExceptions (obsolete) — Splinter Review
So what we really need is the engine's telemetry wiring to understand a variation on histograms that can take strings as arguments.(See georg's patch) 

I can overload the static function AcculmulateTelemetryCallbacks, but I can't overload the typedef for JSAccumulateTelemetryDataCallback currently (id, unit which is what gets passed around.

So, what is the best, minimally invasive way to get the telemetrydatacallbacks to understand a new data type?
Attachment #8518370 - Attachment is obsolete: true
all that's missing is concating the msg const char16_t & the char * to a final char *. yay
Attachment #8521678 - Attachment is obsolete: true
Comment on attachment 8521899 [details] [diff] [review]
ProbeAddonExceptions - almost there!

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

You can e.g. append to a nsCString, converting using NS_ConvertUTF16toUTF8() where needed.
But what about the privacy concerns here? Wouldn't the addon ids be enough to start investigating?

::: js/src/jsfriendapi.h
@@ +130,4 @@
>  };
>  
>  typedef void
> +(*JSAccumulateTelemetryDataCallback)(int id, uint32_t sample, char*key );

Maybe a separate function like JSAccumulateTelemetryKeyedDataCallback makes more sense?
That would avoid changing the existing callback users.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +3015,5 @@
>          MOZ_ASSERT(sample <= 3);
>          Telemetry::Accumulate(Telemetry::JS_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT, sample);
>          break;
> +	  case JS_ADDON_EXCEPTION:
> +		Telemetry::Accumulate(Telemetry::JS_ADDON_EXCEPTIONS, nsCString(aKey), sample);

You can use nsDependentCString(aKey)
OS: Windows 8 → All
Hardware: x86_64 → All
Comment on attachment 8521899 [details] [diff] [review]
ProbeAddonExceptions - almost there!

It's messy, but is this generally good with respect to the callback bits?
Attachment #8521899 - Flags: feedback?(continuation)
the jsexn code changed to take the message. Could you have a quick glance over that?
Attachment #8522431 - Flags: feedback?(wmccloskey)
Comment on attachment 8521899 [details] [diff] [review]
ProbeAddonExceptions - almost there!

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

Yeah, the callback stuff looks good.

::: js/src/jsexn.cpp
@@ +746,5 @@
> +		// maybe there's a printf family function that miht help as we could print them
> +		//char* idAndError = JS_EncodeString(cx, reportp->ucmessage);
> +		char* idAndError = readableName;
> +
> +		JSAccumulateTelemetryDataCallback cb = cx->runtime()->telemetryCallback;

nit: your indentation is off in much of this patch.  Maybe you have tabs instead of spaces?

@@ +747,5 @@
> +		//char* idAndError = JS_EncodeString(cx, reportp->ucmessage);
> +		char* idAndError = readableName;
> +
> +		JSAccumulateTelemetryDataCallback cb = cx->runtime()->telemetryCallback;
> +		if (cb) {

No { on single line if blocks in JS.

::: toolkit/components/telemetry/Histograms.json
@@ +328,5 @@
> +    "expires_in_version" : "never",
> +    "kind": "count",
> +    "keyed" : true,
> +    "description" : "Exceptions thrown in the js Addon compartment"
> +  }, 

nit: trailing space
Attachment #8521899 - Flags: feedback?(continuation) → feedback+
Attachment #8520040 - Attachment is obsolete: true
Per IRC conversation, I think we should go with the following key:
addonid/leafname/lineno

leafnames aren't going to expose the full-path issues, and by avoiding the actual error message we avoid the rest of the privacy risk of having data in error messages.
lightbeam test case, delimited string by spaces

jid1-F9UJ2thwoAm5gQ@jetpack policy.js 94
Attachment #8521899 - Attachment is obsolete: true
Attachment #8522431 - Attachment is obsolete: true
Attachment #8522431 - Flags: feedback?(wmccloskey)
Attachment #8523265 - Attachment is obsolete: true
Attachment #8523270 - Flags: review?(wmccloskey)
Comment on attachment 8523270 [details] [diff] [review]
ProbeAddonExceptionsAddonIdFilenameLineNo.txt - cleaned up

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

::: js/src/jsexn.cpp
@@ +726,2 @@
>  
> +      JSCompartment* cmpt = exnObject->compartment(); //always have one

Please call it |comp|, put the * on the other side, and remove the comment (it's not really needed).

@@ +726,3 @@
>  
> +      JSCompartment* cmpt = exnObject->compartment(); //always have one
> +      JSAddonId* addonIdptr = cmpt->addonId; // if null, not from addon

Let's call this |addonId|, move the * over, and remove the comment.

@@ +726,5 @@
>  
> +      JSCompartment* cmpt = exnObject->compartment(); //always have one
> +      JSAddonId* addonIdptr = cmpt->addonId; // if null, not from addon
> +      if (addonIdptr) {
> +        char *readableName = JS_EncodeString(cx, addonIdptr);

All this should be indented 4 spaces. And let's call this addonIdChars.

@@ +728,5 @@
> +      JSAddonId* addonIdptr = cmpt->addonId; // if null, not from addon
> +      if (addonIdptr) {
> +        char *readableName = JS_EncodeString(cx, addonIdptr);
> +        char* finalKey = readableName;
> +        strcat(finalKey, " ");

It's not safe to append to this string because it's only been allocated large enough to hold the addon ID. I think the best thing would be to make a char buffer on the stack. That would look like this:

char histogramKey[64];
JS_snprintf(histogramKey, sizeof(histogramKey), "%s %s %u", addonIdChars, filename, reportp->lineno);

@@ +729,5 @@
> +      if (addonIdptr) {
> +        char *readableName = JS_EncodeString(cx, addonIdptr);
> +        char* finalKey = readableName;
> +        strcat(finalKey, " ");
> +    

Extra whitespace.

@@ +730,5 @@
> +        char *readableName = JS_EncodeString(cx, addonIdptr);
> +        char* finalKey = readableName;
> +        strcat(finalKey, " ");
> +    
> +        const char *filenameFinder = strrchr(reportp->filename, '/');

Just call this filename.

@@ +731,5 @@
> +        char* finalKey = readableName;
> +        strcat(finalKey, " ");
> +    
> +        const char *filenameFinder = strrchr(reportp->filename, '/');
> +        if (filenameFinder == NULL)

Please flip the order of branches around and remove the |== NULL| part. Also need to indent 4 spaces.

@@ +749,5 @@
> +          (*cb)(JS_ADDON_EXCEPTION, 1, finalKey);
> +
> +      js_free(readableName);
> +      }
> +    }   

Extra whitespace.

::: js/src/jsfriendapi.h
@@ +110,5 @@
>      JS_TELEMETRY_GC_NON_INCREMENTAL,
>      JS_TELEMETRY_GC_SCC_SWEEP_TOTAL_MS,
>      JS_TELEMETRY_GC_SCC_SWEEP_MAX_PAUSE_MS,
> +    JS_TELEMETRY_DEPRECATED_LANGUAGE_EXTENSIONS_IN_CONTENT,
> +    JS_ADDON_EXCEPTION

Please call this JS_TELEMETRY_ADDON_EXCEPTIONS

@@ +115,4 @@
>  };
>  
>  typedef void
> +(*JSAccumulateTelemetryDataCallback)(int id, uint32_t sample, char*key );

Please make the last parameter be const char *key. Also, add a comment explaining what it's for.

Finally, let's add a new function that calls the accumulate telemetry callback. It could be a method of JSRuntime. It would just look like this:

void
JSRuntime::AddTelemetry(int id, uint32_t sample, const char *key = nullptr)
{
    if (telemetryCallback)
        (*telemetryCallback)(id, sample, key);
}

Then please change all the existing uses to use this instead.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2962,5 @@
>  }
>  #endif
>  
>  static void
> +AccumulateTelemetryCallback(int id, uint32_t sample, char *aKey)

Please just call it |key|, and make it const. We don't use the aFoo convention in XPConnect.

::: toolkit/components/telemetry/Histograms.json
@@ +327,5 @@
> +  "JS_ADDON_EXCEPTIONS" : {
> +    "expires_in_version" : "never",
> +    "kind": "count",
> +    "keyed" : true,
> +    "description" : "Exceptions thrown in the js Addon compartment"

"Exceptions thrown by add-ons"

@@ +328,5 @@
> +    "expires_in_version" : "never",
> +    "kind": "count",
> +    "keyed" : true,
> +    "description" : "Exceptions thrown in the js Addon compartment"
> +  }, 

Remove extra space at end.
Attachment #8523270 - Flags: review?(wmccloskey)
Blocks: 1100498
Comment on attachment 8523270 [details] [diff] [review]
ProbeAddonExceptionsAddonIdFilenameLineNo.txt - cleaned up

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

::: toolkit/components/telemetry/Histograms.json
@@ +323,5 @@
>      "kind": "enumerated",
>      "n_values": 10,
>      "description": "Use of SpiderMonkey's deprecated language extensions in web content: ForEach, DestructuringForIn, LegacyGenerator, ExpressionClosure"
>    },
> +  "JS_ADDON_EXCEPTIONS" : {

Should this be ifdef'd to Nightly only? I'm wondering if we want this to ride the trains to release.. it's going to generate a lot of data for other releases which the e10s team won't be looking at much if at all. We might need this later, but not while we're limited to mozilla-central.
Attachment #8523270 - Attachment is obsolete: true
Attachment #8525011 - Flags: review?(wmccloskey)
nasty whitespace removed
Attachment #8525011 - Attachment is obsolete: true
Attachment #8525011 - Flags: review?(wmccloskey)
Attachment #8526875 - Flags: review?(wmccloskey)
Comment on attachment 8525011 [details] [diff] [review]
ProbeAddonExceptionsAddonIdFilenameLineNo.txt v2

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

Mostly just nits.

Also, once you make telemetryCallback private, you'll need to remove anything that checks whether it's null or not. That's fine since AddTelemetry will check that anyway.

You'll also need to add a JSRuntime method to set the telemetry callback. That will be used by JS_SetAccumulateTelemetryCallback.

::: js/src/gc/Statistics.cpp
@@ +744,2 @@
>  
>      if (JSAccumulateTelemetryDataCallback cb = runtime->telemetryCallback) {

We don't need this test anymore since AddTelemetry does its own test. Please remove it.

::: js/src/jsexn.cpp
@@ +726,5 @@
>  
> +        JSCompartment *comp = exnObject->compartment();
> +        JSAddonId *addonId = comp->addonId;
> +        if (addonId) {
> +            char *addonIdChars = JS_EncodeString(cx, addonId);

We can use ScopedFreePtr here like so:

ScopedFreePtr<char> addonIdChars = JS_EncodeString(cx, addonId);

Then you can remove the js_free at the end.

@@ +727,5 @@
> +        JSCompartment *comp = exnObject->compartment();
> +        JSAddonId *addonId = comp->addonId;
> +        if (addonId) {
> +            char *addonIdChars = JS_EncodeString(cx, addonId);
> +            const char *filename= strrchr(reportp->filename, '/');

Space before the equal sign.

@@ +728,5 @@
> +        JSAddonId *addonId = comp->addonId;
> +        if (addonId) {
> +            char *addonIdChars = JS_EncodeString(cx, addonId);
> +            const char *filename= strrchr(reportp->filename, '/');
> +            if (!filename)

This is reversed. Please remove the ! operator.

@@ +736,5 @@
> +
> +            char histogramKey[64];
> +            JS_snprintf(histogramKey, sizeof(histogramKey), "%s %s %u", addonIdChars, filename, reportp->lineno);
> +            JSAccumulateTelemetryDataCallback cb = cx->runtime()->telemetryCallback;
> +            if (cb)

No need for these two lines. AddTelemetry already checks if telemetryCallback is null.

@@ +740,5 @@
> +            if (cb)
> +                cx->runtime()->AddTelemetry(JS_TELEMETRY_ADDON_EXCEPTIONS, 1, histogramKey);
> +            js_free(addonIdChars);
> +        }
> +    }   

Please remove the extra spaces at the end of the line.

::: js/src/jsfriendapi.h
@@ +115,4 @@
>  };
>  
>  typedef void
> +(*JSAccumulateTelemetryDataCallback)(int id, uint32_t sample, const char*key );

Please make the last part read |const char *key);|.

@@ +115,5 @@
>  };
>  
>  typedef void
> +(*JSAccumulateTelemetryDataCallback)(int id, uint32_t sample, const char*key );
> +// key is for Keyed Histograms 

Please remove the comment.

::: js/src/vm/Runtime.h
@@ +757,5 @@
>      void assertCanLock(js::RuntimeLock which);
>  #else
>      void assertCanLock(js::RuntimeLock which) {}
>  #endif
> +    void AddTelemetry(int id, uint32_t sample, const char *key = nullptr);

Please move this so that it's located near the definition of telemetryCallback. Also, please make telemetryCallback private. And we should add a comment for AddTelemetry:

Accumulates data for Firefox telemetry. |id| is the ID of a JS_TELEMETRY_* histogram. |key| provides an additional key to identify the histogram. |sample| is the data to add to the histogram.
Attachment #8525011 - Attachment is obsolete: false
Attachment #8525011 - Attachment is obsolete: true
Attachment #8526875 - Attachment is obsolete: true
Attachment #8527036 - Attachment is obsolete: true
Attachment #8527043 - Flags: review?(wmccloskey)
Comment on attachment 8527043 [details] [diff] [review]
ProbeAddonExceptionsAddonIdFilenameLineNo.txt v5

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

Looks great. Thanks!

We should add a test for this once it lands.

::: js/src/jsexn.cpp
@@ +716,5 @@
> +
> +        JSCompartment *comp = exnObject->compartment();
> +        JSAddonId *addonId = comp->addonId;
> +        if (addonId) {
> +            UniquePtr<char> addonIdChars(JS_EncodeString(cx, addonId));

It looks like UniquePtr<char> will call delete on the chars when you're done, which is not what we want. We want js_free. However, the JS engine already has UniqueChars, so you can use that. Just replace UniquePtr<char> with UniqueChars.

http://mxr.mozilla.org/mozilla-central/source/js/public/Utility.h#634

::: js/src/vm/Runtime.h
@@ +708,5 @@
>    public:
> +    /* Accumulates data for Firefox telemetry. |id| is the ID of a JS_TELEMETRY_*
> +     * histogram. |key| provides an additional key to identify the histogram.
> +     * |sample| is the data to add to the histogram.
> +     * */

Please switch to a //-style comment.

@@ +709,5 @@
> +    /* Accumulates data for Firefox telemetry. |id| is the ID of a JS_TELEMETRY_*
> +     * histogram. |key| provides an additional key to identify the histogram.
> +     * |sample| is the data to add to the histogram.
> +     * */
> +    void AddTelemetry(int id, uint32_t sample, const char *key = nullptr);

Sorry, I forgot this last time: this should be addTelemetry (lowercase 'a'). I've been looking at Gecko code for so long I almost didn't notice.
Attachment #8527043 - Flags: review?(wmccloskey) → review+
Blocks: 1103259
It looks like you need to call .get() on addonIdChars.
https://tbpl.mozilla.org/?tree=Try&rev=bdccfa0ee10e  almost unburned. no repros locally of course.
Flags: needinfo?(ally)
trying to placate the linux compilers and the dt tests
Attachment #8527078 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a808a8e1cc17
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Depends on: e10s-noscript
No longer depends on: e10s-noscript
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.