829121 - Send the official Firefox OS version string in crash reports

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Robert Kaiser]

We currently send the Gecko version number as the app version in B2G crash reports.

To have more granularity of actual releases, we should add an annotation with the actual (4-digit, in the future) Firefox OS version number. This is the version that is also displayed in the "Device Information" section of the Settings app.

Let's call the annotation B2G_OS_Version (unless we're good to use the "Firefox" trademark in there and call it FirefoxOS_Version).

Hub, can you do a patch for that in time to land for the 1/15 deadline?

Comment 1

[Hubert Figuiere [:hub]]

Attachment: Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

Comment 2

[Hubert Figuiere [:hub]]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

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

::: b2g/chrome/content/shell.js
@@ +293,5 @@
>      });
>  
> +#ifdef MOZ_WIDGET_GONK
> +#endif
> +

forget these, I already removed them.

Comment 3

[Hubert Figuiere [:hub]]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

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

::: b2g/chrome/content/shell.js
@@ +223,5 @@
>        cr.annotateCrashReport("Android_Version", androidVersion);
> +
> +      SettingsListener.observe("deviceinfo.os", false, function(value) {
> +        try {
> +          dump("deviceinfo.os " + value);

also removed this dump.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

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

r=me with comments addressed

::: b2g/chrome/content/shell.js
@@ +221,5 @@
>        let androidVersion = libcutils.property_get("ro.build.version.sdk") +
>                             "(" + libcutils.property_get("ro.build.version.codename") + ")";
>        cr.annotateCrashReport("Android_Version", androidVersion);
> +
> +      SettingsListener.observe("deviceinfo.os", false, function(value) {

Do we really want |false| as the default value?

@@ +225,5 @@
> +      SettingsListener.observe("deviceinfo.os", false, function(value) {
> +        try {
> +          dump("deviceinfo.os " + value);
> +          let cr = Cc["@mozilla.org/xre/app-info;1"]
> +            .getService(Ci.nsICrashReporter);

nit: align .getService() with ["@...

@@ +229,5 @@
> +            .getService(Ci.nsICrashReporter);
> +          cr.annotateCrashReport("B2G_OS_Version", value);
> +        } catch(e) {
> +          dump("exception: " + e);
> +        }

Do we need this try/catch? We're already in a an outer one, and we have no |finally| statement specific to this block.

Comment 5

[Hubert Figuiere [:hub]]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

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

::: b2g/chrome/content/shell.js
@@ +221,5 @@
>        let androidVersion = libcutils.property_get("ro.build.version.sdk") +
>                             "(" + libcutils.property_get("ro.build.version.codename") + ")";
>        cr.annotateCrashReport("Android_Version", androidVersion);
> +
> +      SettingsListener.observe("deviceinfo.os", false, function(value) {

Ah, no. Changing to ""

@@ +229,5 @@
> +            .getService(Ci.nsICrashReporter);
> +          cr.annotateCrashReport("B2G_OS_Version", value);
> +        } catch(e) {
> +          dump("exception: " + e);
> +        }

This is inside the callback, the outer try {} is outside the callback. And I do see the exception handler when calling the callback in SettingsListener. Am I missing something?

Comment 6

[[:fabrice] Fabrice Desré]

(In reply to Hub Figuiere [:hub] from comment #5)
> 
> This is inside the callback, the outer try {} is outside the callback. And I
> do see the exception handler when calling the callback in SettingsListener.
> Am I missing something?

No problem, please go ahead.

Comment 7

[Hubert Figuiere [:hub]]

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

Comment 8

[Robert Kaiser]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

I can't comment on the code and if you're getting the right thing, but the name and having the annotation looks good, thanks for reacting so fast! :)

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 700686 [details] [diff] [review]
Bug 829121 - Annotate B2G_OS_Version for the crash reporter.

[Triage Comment]
Not blocking, but please land on b2g18 asap.

Comment 10

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/aedc51a5a72d

Comment 11

[Hubert Figuiere [:hub]]

marking as fixed as per jst new rules.

Comment 12

[Hubert Figuiere [:hub]]

Posted a follow-up at fabrice request:

https://hg.mozilla.org/integration/mozilla-inbound/rev/310f267bc184
https://hg.mozilla.org/releases/mozilla-b2g18/rev/128591474a43

Comment 13

[Ed Morley [:emorley]]

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

Comment 14

[Ed Morley [:emorley]]

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

Comment 15

[Ed Morley [:emorley]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/310f267bc184

Comment 17

[Robert Kaiser]

I found this in the raw JSON for bp-28aa375f-dba0-48ec-94a8-9bc2b2130116:

"B2G_OS_Version": "1.0.0-prerelease",

So I call this VERIFIED. Thanks!