Provider manager calls undefined function this._recordError

RESOLVED FIXED in Firefox 23

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gps, Assigned: gps)

Tracking

unspecified
Firefox 23
Points:
---

Firefox Tracking Flags

(firefox20 unaffected, firefox21 affected, firefox22- affected, firefox23 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 742545 [details] [diff] [review]
Call _recordProviderError, v1

As usual, the tests took longer than the patch.
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #742545 - Flags: review?(rnewman)
(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 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?
Attachment #742545 - Flags: review?(rnewman) → review+
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

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

I can't think of an easy way to verify this. So, qa-.
Whiteboard: [fixed in services][qa-]
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/17e8172c812a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox23: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed in services][qa-] → [qa-]
Target Milestone: --- → Firefox 23

Updated

5 years ago
tracking-firefox22: ? → -

Comment 7

5 years ago
Not critical enough to track, but please nominate for Aurora approval with risk/reward.
You need to log in before you can comment on or make changes to this bug.