Annotate the current telemetry environment in crash reports

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

(Depends on 1 bug)

unspecified
mozilla40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

4 years ago
For bug 1121013, I'd like to add the telemetry environment data as an annotation on crash reports. This will not only help to unify analysis across telemetry and crash-stats, but it will also be a way to submit a useful crash ping based on the same data.
Depends on: 1140558
Assignee

Updated

4 years ago
Assignee: nobody → benjamin
Assignee

Comment 1

4 years ago
Attachment #8581664 - Flags: review?(ted)
Attachment #8581664 - Flags: review?(gfritzsche)
Comment on attachment 8581664 [details] [diff] [review]
1140132-annotate-environment

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

::: toolkit/components/telemetry/TelemetryStartup.js
@@ +21,5 @@
>  
>  TelemetryStartup.prototype.classID = Components.ID("{117b219f-92fe-4bd2-a21b-95a342a9d474}");
>  TelemetryStartup.prototype.QueryInterface = XPCOMUtils.generateQI([Components.interfaces.nsIObserver])
>  TelemetryStartup.prototype.observe = function(aSubject, aTopic, aData) {
>    if (aTopic == "profile-after-change" || aTopic == "app-startup") {

"profile-after-change" is registered for chrome processes, "app-startup" for content processes.
We probably only want to do the annotations in the chrome process?
Attachment #8581664 - Flags: review?(gfritzsche) → review+
Comment on attachment 8581664 [details] [diff] [review]
1140132-annotate-environment

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

::: toolkit/components/telemetry/TelemetryStartup.js
@@ +35,5 @@
> +  try {
> +    let cr = Cc["@mozilla.org/toolkit/crash-reporter;1"]
> +      .getService(Ci.nsICrashReporter);
> +    let env = JSON.stringify(TelemetryEnvironment.currentEnvironment);
> +    cr.annotateCrashReport("Environment", env);

I think TelemetryEnvironment would be more descriptive here.
Attachment #8581664 - Flags: review?(ted)
Attachment #8581664 - Flags: review?(gfritzsche)
Attachment #8581664 - Flags: review+
Attachment #8581664 - Flags: review?(gfritzsche) → review+
Assignee

Comment 5

4 years ago
This ran into xperf main-thread-I/O issues.

09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\secmod.db' was accessed and we were not expecting it. DiskReadCount: 6, DiskWriteCount: 0, DiskReadBytes: 16904, DiskWriteBytes: 0
09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\cert8.db' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 33288, DiskWriteBytes: 0
09:00:57 INFO - TEST-UNEXPECTED-FAIL : xperf: File '{profile}\key3.db' was accessed and we were not expecting it. DiskReadCount: 4, DiskWriteCount: 0, DiskReadBytes: 8712, DiskWriteBytes: 0 

With this c++ stack:

>	xul.dll!EnsureNSSInitialized(EnsureNSSOperator op) Line 107	C++
 	xul.dll!`anonymous namespace'::nsCryptoHashConstructor(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 199	C++
 	xul.dll!mozilla::GenericFactory::CreateInstance(nsISupports * aOuter, const nsID & aIID, void * * aResult) Line 17	C++
 	xul.dll!nsComponentManagerImpl::CreateInstance(const nsID & aClass, nsISupports * aDelegate, const nsID & aIID, void * * aResult) Line 1146	C++
 	xul.dll!nsJSCID::CreateInstance(JS::Handle<JS::Value> iidval, JSContext * cx, unsigned char optionalArgc, JS::MutableHandle<JS::Value> retval) Line 651	C++
 	xul.dll!NS_InvokeByIndex(nsISupports * that, unsigned int methodIndex, unsigned int paramCount, nsXPTCVariant * params) Line 71	C++

And this JS stack:

0 getIDHashForString(aStr = "Adobe AcrobatAdobe PDF Plug-In For Firefox and Nets
cape 11.0.10") ["resource://gre/modules/addons/PluginProvider.jsm":33]
1 PL_getPluginList() ["resource://gre/modules/addons/PluginProvider.jsm":198]
    this = [object Object]
2 PL_buildPluginList() ["resource://gre/modules/addons/PluginProvider.jsm":219]
    this = [object Object]
3 PL_getAddonsByTypes(aTypes = plugin, aCallback = [function]) ["resource://gre/
modules/addons/PluginProvider.jsm":144]
    this = [object Object]
4 callProviderAsync(aProvider = [object Object], aMethod = "getAddonsByTypes", a
Args = plugin,function getAddonsByTypes_concatAddons(aProviderAddons) {
"use strict";

          if (aProviderAddons) {
            addons = addons.concat(aProviderAddons);
          }
          aCaller.callNext();
        }, [function]) ["resource://gre/modules/AddonManager.jsm":235]
5 getAddonsByTypes_nextObject(aCaller = [object Object], aProvider = [object Obj
ect]) ["resource://gre/modules/AddonManager.jsm":2226]
    this = [object Object]
6 AOC_callNext() ["resource://gre/modules/AddonManager.jsm":311]
    this = [object Object]
7 getAddonsByTypes_concatAddons(aProviderAddons = [object Object],[object Object
]) ["resource://gre/modules/AddonManager.jsm":2231]
8 GMPProvider.getAddonsByTypes(aTypes = plugin, aCallback = [function]) ["resour
ce://gre/modules/addons/GMPProvider.jsm":560]
    this = [object Object]
9 callProviderAsync(aProvider = [object Object], aMethod = "getAddonsByTypes", a
Args = plugin,function getAddonsByTypes_concatAddons(aProviderAddons) {
"use strict";

          if (aProviderAddons) {
            addons = addons.concat(aProviderAddons);
          }
          aCaller.callNext();
        }, [function]) ["resource://gre/modules/AddonManager.jsm":235]
10 getAddonsByTypes_nextObject(aCaller = [object Object], aProvider = [object Ob
ject]) ["resource://gre/modules/AddonManager.jsm":2226]
    this = [object Object]
11 AOC_callNext() ["resource://gre/modules/AddonManager.jsm":311]
    this = [object Object]

I'm surprised that the crypto-hash code needs to do the full initialization of NSS, but this will clearly happen right *near* startup anyway, if the user is visiting SSL sites. Are we willing to accept this main-thread I/O hit for these files anyway, on the theory that we're going to end up doing this main-thread I/O anyway? Avi, are you the right person to make this decision?

If we do accept this, what's the right procedure to get these files added to the xperf whitelist?
Flags: needinfo?(avihpit)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> ... 
> I'm surprised that the crypto-hash code needs to do the full initialization
> of NSS, but this will clearly happen right *near* startup anyway, if the
> user is visiting SSL sites. Are we willing to accept this main-thread I/O
> hit for these files anyway, on the theory that we're going to end up doing
> this main-thread I/O anyway? Avi, are you the right person to make this
> decision?

Typically Vladan and I _think Aaron help with the main thread IO decisions.

I'm pretty sure there wasn't and isn't a hard threshold as for what should be considered acceptable. It's a judgment call which tries to assess the overall usefulness of one approach vs another.

That being said, from what I understand so far this isn't a new thing, so it's not a "Regression", but rather a situation which needs to be assessed if it can be improved or not.

Aaron, any insights on this?
Flags: needinfo?(avihpit) → needinfo?(aklotz)
One of the common things that we see is that something that previously caused main thread I/O outside of startup (where in the case of xperf, this means past the sessionstore-windows-restored cutoff point) is moved earlier such that it now falls under the scope of xperf.

When they can be easily avoided, we like to do so, but sometimes we just have to take the hit. NSS seems to me like an instance of the latter. I'd be ok with whitelisting these.
Flags: needinfo?(aklotz)
Assignee

Comment 9

4 years ago
Posted patch 1140132-nssSplinter Review
Attachment #8598171 - Flags: review?(aklotz)
Comment on attachment 8598171 [details] [diff] [review]
1140132-nss

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

Once this lands in Talos you'll also need to update the talos revision in m-c. See bug 966347.
Attachment #8598171 - Flags: review?(aklotz) → review+
Assignee

Updated

4 years ago
Depends on: 1138079
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/1815927ac2fc
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.