1199502 - Properly disable unused Telemetry modules on B2G

Status: RESOLVED WORKSFORME

(Toolkit :: Telemetry, defect, P4)

Description

[[:fabrice] Fabrice Desré]

I'm seeing this exception on every b2g device run:
E/GeckoConsole(29260): [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
E/GeckoConsole(29260): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
E/GeckoConsole(29260): 
E/GeckoConsole(29260): Date: Thu Aug 27 2015 15:51:34 GMT-0700 (UTC)
E/GeckoConsole(29260): Full Message: TypeError: invalid 'in' operand hls
E/GeckoConsole(29260): Full Stack: getHistograms@resource://gre/modules/TelemetrySession.jsm:966:1
E/GeckoConsole(29260): Impl.assemblePayloadWithMeasurements@resource://gre/modules/TelemetrySession.jsm:1249:19
E/GeckoConsole(29260): getSessionPayload@resource://gre/modules/TelemetrySession.jsm:1318:19
E/GeckoConsole(29260): Impl._saveAbortedSessionPing@resource://gre/modules/TelemetrySession.jsm:1942:17
E/GeckoConsole(29260): setupChromeProcess/this._delayedInitTask<@resource://gre/modules/TelemetrySession.jsm:1451:19
E/GeckoConsole(29260): TaskImpl_run@resource://gre/modules/Task.jsm:314:40
E/GeckoConsole(29260): Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
E/GeckoConsole(29260): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:813:7
E/GeckoConsole(29260): this.PromiseWalker.sched

Once I fixed this one, a new one appeared:

E/GeckoConsole(30349): [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
E/GeckoConsole(30349): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
E/GeckoConsole(30349): 
E/GeckoConsole(30349): Date: Thu Aug 27 2015 16:09:56 GMT-0700 (UTC)
E/GeckoConsole(30349): Full Message: TypeError: keyed.subsessionSnapshot is not a function
E/GeckoConsole(30349): Full Stack: Impl.getKeyedHistograms@resource://gre/modules/TelemetrySession.jsm:1012:38
E/GeckoConsole(30349): Impl.assemblePayloadWithMeasurements@resource://gre/modules/TelemetrySession.jsm:1255:24
E/GeckoConsole(30349): getSessionPayload@resource://gre/modules/TelemetrySession.jsm:1323:19
E/GeckoConsole(30349): Impl._saveAbortedSessionPing@resource://gre/modules/TelemetrySession.jsm:1947:17
E/GeckoConsole(30349): setupChromeProcess/this._delayedInitTask<@resource://gre/modules/TelemetrySession.jsm:1456:19
E/GeckoConsole(30349): TaskImpl_run@resource://gre/modules/Task.jsm:314:40
E/GeckoConsole(30349): Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:934:23
E/GeckoConsole(30349): this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: telemetry.patch

Georg, this fixes the exceptions, but I don't know if they are the sign of some other issue!

Comment 2

[Georg Fritzsche [:gfritzsche]]

The underlying issue would be "we shouldn't init the Telemetry JSM modules on any B2G builds" :)
Tamara, do you know if anyone looked at why this activates here?

Comment 3

[Tamara Hills [:thills]]

Hi Georg,

I would think that it's getting activated by what we're doing in /gecko/b2g/chrome/content/devtools/hud.js.

I'm not sure what's loading the session though.  I will take a look and see if some of this is getting loaded without our hud.js code being called.

I'll leave the ni here for now.
-tamara

Comment 4

[Georg Fritzsche [:gfritzsche]]

I think related bugs already happened before you started to work on those things, so it might be an older issue.

Thanks for checking.

Comment 5

[Tamara Hills [:thills]]

Hi Georg,

Here's some more info...  Let me know if you have additional thoughts.

I'm calling the following APIs:
Services.telemetry.getAddonHistogram
  .add(), .clear(), .snapshot()
Services.telemetry.getKeyedHistogramById
  .add(), .clear(), .snapshot()
Services.telemetry.registerAddonHistogram

I believe this is triggering the load of the session and probably the environment too... which is strange, because I thought these could be used independently of the session.

I tried commenting out all our calls to these functions and I don't get the exceptions.  I do see the following message in the logs (even without our use of the Telemetry objects above):
E/GeckoConsole( 3459): [JavaScript Error: "1440769896554	Toolkit.Telemetry	ERROR	TelemetryStorage::loadAbortedSessionPing - error removing ping: PingReadError JS Stack trace: [e-mail]:80:15 < TelemetryStorageImpl.loadPingFile<@TelemetryStorage.jsm:1448:13" {file: "resource://gre/modules/Log.jsm" line: 749}]

I'm not sure how this TelemetryStorage gets initialized either.

Also, with Fabrice's patch, I did not see the two exceptions he listed above.  I did also come across the following additional exception:

E/GeckoConsole( 9804): [JavaScript Error: "A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
E/GeckoConsole( 9804): See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise
E/GeckoConsole( 9804): 
E/GeckoConsole( 9804): Date: Fri Aug 28 2015 10:46:16 GMT-0400 (EDT)
E/GeckoConsole( 9804): Full Message: TypeError: Cc[aContract] is undefined
E/GeckoConsole( 9804): Full Stack: XPCU_serviceLambda@resource://gre/modules/XPCOMUtils.jsm:228:7
E/GeckoConsole( 9804): XPCU_defineLazyGetter/<.get@resource://gre/modules/XPCOMUtils.jsm:196:21
E/GeckoConsole( 9804): EnvironmentCache.prototype._updateSearchEngine@resource://gre/modules/TelemetryEnvironment.jsm:815:9
E/GeckoConsole( 9804): EnvironmentCache.prototype._updateSettings@resource://gre/modules/TelemetryEnvironment.jsm:931:5
E/GeckoConsole( 9804): EnvironmentCache@resource://gre/modules/TelemetryEnvironment.jsm:588:3
E/GeckoConsole( 9804): getGlobal@resource://gre/modules/TelemetryEnvironment.jsm:46:26
E/GeckoConsole( 9804): this.TelemetryEnvironment.currentEnvironment@resource://gre/modules/TelemetryEnvironment.jsm:53:12
E/GeckoConsole( 9804): assemblePing@resource://gre/modules/TelemetryController.jsm:469:7
E/GeckoConsole( 9804): Impl.saveAbortedSessionPing@resource://gre/modules/TelemetryControl


Thanks,

-tamara

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Tamara Hills [:thills] from comment #5)
> Hi Georg,
> 
> Here's some more info...  Let me know if you have additional thoughts.
> 
> I'm calling the following APIs:
> Services.telemetry.getAddonHistogram
>   .add(), .clear(), .snapshot()
> Services.telemetry.getKeyedHistogramById
>   .add(), .clear(), .snapshot()
> Services.telemetry.registerAddonHistogram
> 
> I believe this is triggering the load of the session and probably the
> environment too... which is strange, because I thought these could be used
> independently of the session.

Yeah, those should not trigger any of the Telemetry JS modules to become active.
This all goes through Telemetry.cpp, which doesn't call into the Telemetry JS modules.

This really looks like the Telemetry JS modules are initialized, e.g. through here:
https://dxr.mozilla.org/mozilla-central/rev/a6786bf8d71d4cf40c3d40e06d8e3c9866863475/toolkit/components/telemetry/TelemetryStartup.js#29

Comment 7

[Georg Fritzsche [:gfritzsche]]

Ok, sorry, this has been lying around for a bit.
Fabrice, given that B2G definitely takes a different path and will not use these Telemetry modules, do you mind if i morph this bug into "make sure we don't run these modules" instead?

Fabrice or Tamara, where can i reproduce this?
Any starter links for what i need to build and run (without devices preferrably)?

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 5 - 13] from comment #7)
> Ok, sorry, this has been lying around for a bit.
> Fabrice, given that B2G definitely takes a different path and will not use
> these Telemetry modules, do you mind if i morph this bug into "make sure we
> don't run these modules" instead?

Sure please do.

> Fabrice or Tamara, where can i reproduce this?
> Any starter links for what i need to build and run (without devices
> preferrably)?

I've only seen that on device, but it's been a while since I used desktop builds. Let us know if you need a device.

Comment 9

[Georg Fritzsche [:gfritzsche]]

I didn't have success finding a Nightly simulator that actually works for me.

We should have devices i can use when i'm back in Berlin next month, i can look into it then.

Comment 10

[Thomas Huelbert]

unable to size at this time

Comment 11

[Thomas Huelbert]

and sized

Comment 12

[Tamara Hills [:thills]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Ok, sorry, this has been lying around for a bit.
> Fabrice, given that B2G definitely takes a different path and will not use
> these Telemetry modules, do you mind if i morph this bug into "make sure we
> don't run these modules" instead?
> 
> Fabrice or Tamara, where can i reproduce this?
> Any starter links for what i need to build and run (without devices
> preferrably)?

Hi Georg,

I'm seeing it on a fresh build about a minute or so after the device finishes loading.  If I'm going through the FTU, it's usually after I click thru the FTU.

I have not tried to repro on the desktop build.

Thanks,
tamara

Comment 15

[Georg Fritzsche [:gfritzsche]]

From searching around DXR, i think we need to:
* remove these entries:
  https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/b2g/installer/package-manifest.in#639-640
  https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/mobile/android/b2gdroid/installer/package-manifest.in#410-411
* possibly also remove the telemetry.xpt entries in those two manifest files
* stop including the Telemetry JS modules in the build:
  https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/toolkit/components/telemetry/moz.build#28-51
* disable all the Telemetry unit tests and additional test files except test_nsITelemetry.js (which tests the Telemetry C++ core, which is used on B2G):
  https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/xpcshell.ini

Comment 16

[Russ Nicoletti [:russn]]

After making these changes, I no longer see any of the exceptions. However, I'm finding that removing all of the "EXTRA_JS_MODULES" from the build causes the device to not startup.

Trial and error has shown that if 'TelemetryStopwatch.jsm' is not included in the build, the build is somehow defective. I don't see any helpful errors in the logcat when using this build.

Any ideas?

Comment 17

[Georg Fritzsche [:gfritzsche]]

Ah, good point!
TelemetryStopWatch.jsm will be used across the build, so we should leave that and test_TelemetryStopwatch.js in the B2G build/tests.

Comment 18

[Russ Nicoletti [:russn]]

Attachment: disable-telemetry-modules.patch

This is the patch I've created. And this is the try link [1]. As you can see, the try builds failed [2]. This is confusing to me because I don't see why my changes affect the 'browser' build.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd6f5de4ef79
[2] Error from build log:

15:53:43     INFO -  Error: /builds/slave/try-l64-0000000000000000000000/build/src/browser/installer/package-manifest.in:525: Missing file(s): bin/components/TelemetryStartup.js
15:53:43     INFO -  Error: /builds/slave/try-l64-0000000000000000000000/build/src/browser/installer/package-manifest.in:526: Missing file(s): bin/components/TelemetryStartup.manifest

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8690328 [details] [diff] [review]
disable-telemetry-modules.patch

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

::: toolkit/components/telemetry/moz.build
@@ -25,5 @@
>      'WebrtcTelemetry.cpp',
>  ]
>  
> -EXTRA_COMPONENTS += [
> -    'TelemetryStartup.js',

You are removing the files here from the build for all platforms.
Instead of removing them, you should add them to the build for only the non-B2G builds.

::: toolkit/components/telemetry/tests/unit/xpcshell.ini
@@ -26,5 @@
>  [test_SubsessionChaining.js]
>  tags = addons
> -[test_TelemetryEnvironment.js]
> -# Bug 1144395: crash on Android 4.3
> -skip-if = android_version == "18"

Same with the tests here: This removes the tests for all the builds.
You should add "skip-if" conditions for the B2G builds.

Comment 20

[Russ Nicoletti [:russn]]

Attachment: disable-telemetry-modules.patch

To disable the telemetry tests for b2g, I've used |os == "android"| instead of |android_version == "18"| because the former seems more generic while the latter seems overly specific (if the android version changes these entries need to be changed). Not knowing much about the build process or build files, I could be completely wrong about this.

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691125 [details] [diff] [review]
disable-telemetry-modules.patch

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

::: toolkit/components/telemetry/moz.build
@@ +40,5 @@
> +        'TelemetryReportingPolicy.jsm',
> +        'TelemetrySend.jsm',
> +        'TelemetrySession.jsm',
> +        'TelemetryStorage.jsm',
> +        'TelemetryTimestamps.jsm',

Are you sure that TelemetryTimestamps.jsm is not used anywhere in B2G code?
I think we should leave that and test_TelemetryTimestamps.js in the build.

::: toolkit/components/telemetry/tests/unit/xpcshell.ini
@@ +34,5 @@
>  tags = addons
>  [test_PingAPI.js]
>  skip-if = os == "android"
>  [test_TelemetryFlagClear.js]
> +# Note [1]

For me it's ok if we only add one general comment at the top saying that most tests are disabled for B2G build because the tested components are not available there.

@@ +35,5 @@
>  [test_PingAPI.js]
>  skip-if = os == "android"
>  [test_TelemetryFlagClear.js]
> +# Note [1]
> +skip-if = os == "android"

This will fully disable the tests on Firefox for Android builds, we need that test coverage there.
We should only disable the tests for any B2G builds - looking through a "skip-if" search:
https://dxr.mozilla.org/mozilla-central/search?q=skip-if&case=true
... i think we could use something like this:
skip-if = (android_version == "18" || buildapp == "mulet" || buildapp == "b2g" || toolkit == "gonk")

Someone with B2G experience can probably comment better on the exact entry needed here.

Comment 22

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #21)
> This will fully disable the tests on Firefox for Android builds, we need
> that test coverage there.
> We should only disable the tests for any B2G builds - looking through a
> "skip-if" search:
> https://dxr.mozilla.org/mozilla-central/search?q=skip-if&case=true
> ... i think we could use something like this:
> skip-if = (android_version == "18" || buildapp == "mulet" || buildapp ==
> "b2g" || toolkit == "gonk")
> 
> Someone with B2G experience can probably comment better on the exact entry
> needed here.

Fabrice, do you know?

Comment 23

[[:fabrice] Fabrice Desré]

To disable on all b2g products (except Mulet, which is similar to Firefox desktop for telemetry as far as I know), just do
skip-if = buildapp == "b2g"

We don't run xpcshell tests on b2gdroid yet so I'll deal with that once we reach this point.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Ok, so depending on the test-entry we can use one of:
skip-if = build-app == "b2g"
skip-if = (android_version == "18" || buildapp == "b2g")
skip-if = (os == "android" || buildapp == "b2g")

Comment 25

[Georg Fritzsche [:gfritzsche]]

B2G is gone from mozilla-central per bug 1346200.