Properly disable unused Telemetry modules on B2G

RESOLVED WORKSFORME

Status

()

defect
P4
normal
RESOLVED WORKSFORME
4 years ago
2 years ago

People

(Reporter: fabrice, Assigned: rnicoletti)

Tracking

(Blocks 1 bug)

Trunk
Points:
2

Firefox Tracking Flags

(firefox43 affected)

Details

(Whiteboard: [measurement:client])

Attachments

(3 attachments)

(Reporter)

Description

4 years ago
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:
(Reporter)

Comment 1

4 years ago
Georg, this fixes the exceptions, but I don't know if they are the sign of some other issue!
Assignee: nobody → fabrice
Attachment #8653807 - Flags: review?(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?
Flags: needinfo?(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
I think related bugs already happened before you started to work on those things, so it might be an older issue.

Thanks for checking.
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: PingReadError@TelemetryStorage.jsm: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
Flags: needinfo?(thills)
(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
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)?
Flags: needinfo?(thills)
Flags: needinfo?(fabrice)
Attachment #8653807 - Flags: review?(gfritzsche)
(Reporter)

Comment 8

4 years ago
(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.
Flags: needinfo?(fabrice)
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

4 years ago
unable to size at this time
Priority: -- → P3

Comment 11

4 years ago
and sized
Points: --- → 2
(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
Flags: needinfo?(thills)
Summary: Telemetry exceptions on b2g → Properly disable unused Telemetry modules on B2G
Duplicate of this bug: 1213822
Duplicate of this bug: 1213808
Assignee: fabrice → nobody
Priority: P3 → P1
Whiteboard: [measurement:client]
Priority: P1 → P2
Priority: P2 → P3
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
(Assignee)

Comment 16

4 years ago
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?
Flags: needinfo?(gfritzsche)
QA Contact: rnicoletti
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.
Flags: needinfo?(gfritzsche)
(Assignee)

Comment 18

4 years ago
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
Attachment #8690328 - Flags: feedback?(gfritzsche)
(Assignee)

Updated

4 years ago
Assignee: nobody → rnicoletti
QA Contact: rnicoletti
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.
Attachment #8690328 - Flags: feedback?(gfritzsche)
(Assignee)

Comment 20

3 years ago
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.
Attachment #8691125 - Flags: feedback?(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.
Attachment #8691125 - Flags: feedback?(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?
Flags: needinfo?(fabrice)
(Reporter)

Comment 23

3 years ago
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.
Flags: needinfo?(fabrice)
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")
Priority: P3 → P4
See Also: → 1346200
B2G is gone from mozilla-central per bug 1346200.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.