Send the official Firefox OS version string in crash reports

VERIFIED FIXED in Firefox 21

Status

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: kairo, Assigned: hub)

Tracking

unspecified
B2G C4 (2jan on)
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

7 years ago
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?
Assignee

Comment 1

7 years ago
Attachment #700686 - Flags: review?(fabrice)
Attachment #700686 - Flags: feedback?(kairo)
Assignee

Comment 2

7 years ago
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.
Assignee

Comment 3

7 years ago
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 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.
Attachment #700686 - Flags: review?(fabrice) → review+
Assignee

Comment 5

7 years ago
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?
(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.
Reporter

Comment 8

7 years ago
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! :)
Attachment #700686 - Flags: feedback?(kairo) → feedback+
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.
Attachment #700686 - Flags: approval-mozilla-b2g18+
blocking-basecamp: ? → -
Assignee

Comment 11

7 years ago
marking as fixed as per jst new rules.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
Reporter

Comment 17

7 years ago
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!
Status: RESOLVED → VERIFIED
Reporter

Updated

7 years ago
Reporter

Updated

7 years ago
Blocks: 832351
Reporter

Updated

7 years ago
Blocks: 832362
You need to log in before you can comment on or make changes to this bug.