866253 - Provider manager calls undefined function this._recordError

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

providermanager.jsm makes a few calls to this._recordError(). However, that function doesn't exist. It's supposed to call this._recordProviderError().

Apparently we don't have test coverage over these error conditions otherwise we would have detected this.

Fortunately, I don't believe this is biting us (at least too often).

I'd like to uplift to 22 out of principle of this should be a low-risk bug fix. Not sure about 21. We can probably do without.

Will try to have a patch (with new tests) shortly.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Call _recordProviderError, v1

As usual, the tests took longer than the patch.

Comment 2

[bhavana bajaj [:bajaj]]

(In reply to Gregory Szorc [:gps] from comment #0)
> providermanager.jsm makes a few calls to this._recordError(). However, that
> function doesn't exist. It's supposed to call this._recordProviderError().
> 
> Apparently we don't have test coverage over these error conditions otherwise
> we would have detected this.
> 
> Fortunately, I don't believe this is biting us (at least too often).
> 
> I'd like to uplift to 22 out of principle of this should be a low-risk bug
> fix. Not sure about 21. We can probably do without.
> 
> Will try to have a patch (with new tests) shortly.
Can you please explain the user impact here ? If this not needed for Fx21, why are we rushing it in Fx22 ?

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 742545 [details] [diff] [review]
Call _recordProviderError, v1

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

Nits!

::: services/metrics/modules-testing/mocks.jsm
@@ +133,5 @@
> +this.DummyPullOnlyThrowsOnInitProvider.prototype = {
> +  __proto__: DummyConstantProvider.prototype,
> +
> +  name: "DummyPullOnlyThrowsOnInitProvider",
> +

Spare line back in the spare line cupboard, please.

::: services/metrics/tests/xpcshell/test_metrics_provider_manager.js
@@ +280,5 @@
> +                      "resource://testing-common/services/metrics/mocks.jsm",
> +                      false, true);
> +
> +  let deferred = Promise.defer();
> +  let deferred = Promise.defer();

What's going on here? Shouldn't this fail in strict mode?

@@ +301,5 @@
> +});
> +
> +add_task(function test_pull_only_registration_error() {
> +  // Re-use database for perf reasons.
> +  let storage = yield Metrics.Storage("registration_errors");

const SHARED_TEST_DB = "registration_errors";

@@ +338,5 @@
> +    deferred.resolve(msg);
> +  };
> +
> +  yield manager.registerProviderFromType(DummyThrowOnShutdownProvider);
> +  yield manager.registerProviderFromType(DummyProvider);

Also

  do_check_eq(manager.providers.length, 2);

no?

Comment 4

[Gregory Szorc [:gps]]

It's possible some elements of data collection are failing and we don't know about it because the error reporting is broken. If we can get it in Beta/21 I'd love to. But, it's towards the end of that cycle and I figured it best not to ask for uplift. Aurora is still mid cycle and would get this patch in the hands of a wider audience sooner. So, I figured why not ask.

If we discover from Nightly landing that this patch reveals many hidden errors, that would give more justification for uplifting to any channel.

Comment 5

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/17e8172c812a

I can't think of an easy way to verify this. So, qa-.

Comment 6

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/17e8172c812a

Comment 7

[Alex Keybl [:akeybl]]

Not critical enough to track, but please nominate for Aurora approval with risk/reward.