Closed Bug 829121 Opened 8 years ago Closed 8 years ago

Send the official Firefox OS version string in crash reports

Categories

(Firefox OS Graveyard :: General, defect)

defect
Not set
normal

Tracking

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

VERIFIED FIXED
B2G C4 (2jan on)
blocking-basecamp -
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed

People

(Reporter: kairo, Assigned: hub)

References

Details

Attachments

(1 file)

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?
Attachment #700686 - Flags: review?(fabrice)
Attachment #700686 - Flags: feedback?(kairo)
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 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+
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.
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: ? → -
marking as fixed as per jst new rules.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → B2G C4 (2jan on)
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
Blocks: 832351
Blocks: 832362
You need to log in before you can comment on or make changes to this bug.