1167422 - don't log it as an error that telemetry can't get a browser shell

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[ISHIKAWA, Chiaki]

When I start up local build of C-C TB, I see the following message
in error console:

Timestamp: 2015年05月22日 07時11分37秒
Error: 1432246297697	Toolkit.Telemetry	ERROR	TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service
Source File: resource://gre/modules/Log.jsm
Line: 749

Maybe this is why Telemetry does not work (or does it?).

TIA

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: bug1167422_telemetry_default_browser_thunderbird.patch

Don't produce warning that's not relevant for thunderbird.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8609899 [details] [diff] [review]
bug1167422_telemetry_default_browser_thunderbird.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +934,3 @@
>  #else
>      if (!("@mozilla.org/browser/shell-service;1" in Cc)) {
>        this._log.error("_isDefaultBrowser - Could not obtain shell service");

I think instead of special-casing Thunderbird above, lets just make it this._log.info().
There are potentially more products that don't have the shell service.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: bug1167422_telemetry_default_browser_thunderbird.patch

Using info() instead.

Comment 4

[Kent James (:rkent)]

What Thunderbird versions are affected by this?

Comment 5

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #4)
> What Thunderbird versions are affected by this?

I saw the original error when I was testing C-C TB.
I have no idea how far the versions need to be backtraced until the error is no longer seen.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610211 [details] [diff] [review]
bug1167422_telemetry_default_browser_thunderbird.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +939,5 @@
>      try {
>        shellService = Cc["@mozilla.org/browser/shell-service;1"]
>                         .getService(Ci.nsIShellService);
>      } catch (ex) {
> +      this._log.info("_isDefaultBrowser - Could not obtain shell service", ex);

I think we only need to change the first log statement: if the product is built with the shellservice, we should be able to retrieve it and check for the default browser.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to ISHIKAWA, Chiaki from comment #5)
> (In reply to Kent James (:rkent) from comment #4)
> > What Thunderbird versions are affected by this?
> 
> I saw the original error when I was testing C-C TB.
> I have no idea how far the versions need to be backtraced until the error is
> no longer seen.

That should be back to fx 39. However this is only logging an error, not an actual failure that affects other behavior.

Comment 8

[Magnus Melin [:mkmelin]]

Should I remove the |if (shellService)| check while I'm at it? http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/telemetry/TelemetryEnvironment.jsm#947 (You return early in the catch above when there's no shellservcie)

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Magnus Melin from comment #8)
> Should I remove the |if (shellService)| check while I'm at it?
> http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/
> telemetry/TelemetryEnvironment.jsm#947 (You return early in the catch above
> when there's no shellservcie)

Is .getService() guaranteed to return a valid object?
I can't anywhere specifying that, so i think it's better to leave it.

Comment 10

[Magnus Melin [:mkmelin]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Is .getService() guaranteed to return a valid object?

XPCOM is (per convention at least) not allowed to return null on success.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Magnus Melin from comment #10)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> > Is .getService() guaranteed to return a valid object?
> 
> XPCOM is (per convention at least) not allowed to return null on success.

Ok, worst case we'll just throw here anyway, which is caught locally, so lets remove the check.
Thanks.

Comment 12

[patclash]

(In reply to Kent James (:rkent) from comment #4)
> What Thunderbird versions are affected by this?

I saw this error too after I update Earlybird 39.0a2 to 40.0a2 
(the save-telemetry-ping folder in the profil grow since 15 May)

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1167422_telemetry_default_browser_thunderbird.patch

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610755 [details] [diff] [review]
bug1167422_telemetry_default_browser_thunderbird.patch

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

Thanks!
Nit: This could use a better commit message.

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8610755 [details] [diff] [review]
bug1167422_telemetry_default_browser_thunderbird.patch

Approval Request Comment
[Feature/regressing bug #]:
Unified Telemetry, https://wiki.mozilla.org/Unified_Telemetry
This is part of the first (main) batch of uplifts to 40 to enable shipping on that train, see bug 1120356, comment 2.
[User impact if declined]:
Data & measurement insight projects delayed or blocked with direct impact on projects depending on this.
[Describe test coverage new/current, TreeHerder]:
We have good automation coverage of the feature.
We also had manual tests of the main tasks as well as confirmation of correct behavior on Nightly for the patches here.
[Risks and why]:
Low-risk - these patches are rather isolated to Telemetry and have been on Nightly for a while with no bad reports.
We intend to track on-going data quality and other issues during the 40 aurora & beta and flip the new behavior off when it doesn't meet the requirements.
[String/UUID change made/needed]:
The only string changes were to the about:telemetry page.
We decided that we can live with missing translations on that page for a cycle as that page is not exactly user-facing.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8610755 [details] [diff] [review]
bug1167422_telemetry_default_browser_thunderbird.patch

Unified telemetry is an important new feature. It is blocking some other projects. Taking it.

Comment 19

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b86ca4f51a39