Closed Bug 1167422 Opened 9 years ago Closed 9 years ago

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

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: ishikawa, Assigned: mkmelin)

References

Details

(Whiteboard: [uplift])

Attachments

(1 file, 2 obsolete files)

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
Summary: 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 → Error: 1432246297697 Toolkit.Telemetry ERROR TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service Source File: resource://gre/modules/Log.jsm Line: 749
Don't produce warning that's not relevant for thunderbird.
Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Attachment #8609899 - Flags: review?(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.
Attachment #8609899 - Flags: review?(gfritzsche)
Using info() instead.
Attachment #8609899 - Attachment is obsolete: true
Attachment #8610211 - Flags: review?(gfritzsche)
What Thunderbird versions are affected by this?
(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 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.
Attachment #8610211 - Flags: review?(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.
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)
(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.
(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.
(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.
(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)
Attachment #8610211 - Attachment is obsolete: true
Attachment #8610755 - Flags: review?(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.
Attachment #8610755 - Flags: review?(gfritzsche) → review+
Component: General → Telemetry
Product: Thunderbird → Toolkit
Summary: Error: 1432246297697 Toolkit.Telemetry ERROR TelemetryEnvironment::_isDefaultBrowser - Could not obtain shell service Source File: resource://gre/modules/Log.jsm Line: 749 → don't log it as an error that telemetry can't get a browser shell
https://hg.mozilla.org/mozilla-central/rev/dd485758af5c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1120356
Whiteboard: [uplift]
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.
Attachment #8610755 - Flags: approval-mozilla-aurora?
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.
Attachment #8610755 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.