Closed Bug 1329850 Opened 3 years ago Closed 3 years ago

Exceptions creating outgoing records aren't handled correctly nor reported via telemetry

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: markh, Assigned: eoger)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Investigating bug 1165811, I changed getTabState in tabs.js to:

  getTabState: function (tab) {
    throw new Error("oh no");

And noticed the following badness:

* We catch the exception at https://dxr.mozilla.org/mozilla-central/rev/2977ca1224525680cbfb5c3ce3018818b6dfd8f2/services/sync/modules/engines.js#1588

* We then fail to count the record as a failure, meaning the error isn't recorded to telemetry. This makes it impossible to determine how many users might be seeing such errors.

* We fail to honor this.allowSkippedRecord, and thus we may end up uploading an inconsistent state for things like bookmarks.

In an ideal world me might want to differentiate the failures here (ie, to differentiate if the error was an unhandled exception or was rejected by the server) and to record details of the exceptions encountered, but just counting them as the same in the short term would be an improvement.
Summary: Exceptions creating outgoing records aren't handled correctly not are reported via telemetry → Exceptions creating outgoing records aren't handled correctly nor reported via telemetry
Blocks: 1165811
Priority: -- → P2
Assignee: nobody → eoger
Priority: P2 → P1
Note: this breaks test_clients_engine.js:
|Unexpected exception TypeError: record.cleartext is undefined at resource://services-sync/engines/clients.js:734|
Comment on attachment 8830051 [details]
Bug 1329850 - Sync records creation failures are reported to telemetry.

https://reviewboard.mozilla.org/r/106972/#review108102

Thanks!

::: services/sync/modules/engines.js:1585
(Diff revision 1)
>            ok = true;
>          } catch (ex) {
> -          if (Async.isShutdownException(ex)) {
> +          ++counts.failed;
> +          this._log.warn("Error creating record", ex);
> +          if (Async.isShutdownException(ex) || !this.allowSkippedRecord) {
>              throw ex;

I think we probably want to send the "weave:engine:sync:uploaded" notification here too, so telemetry sees it (or possibly move the one below into a finally?)

::: services/sync/tests/unit/test_syncengine_sync.js:1653
(Diff revision 1)
> +
> +  try {
> +    engine._syncStartup();
> +    let error = null;
> +    try {
> +      engine._uploadOutgoing();

we should be checking the telemetry failed count here too (which as mentioned above, probably is going to fail as written).

It also looks like this test is largely identical to the one above except for the setting of allowSkippedRecord - maybe it could be split into a helper function that takes an argument for allowSkippedRecord and have it called twice?
Attachment #8830051 - Flags: review?(markh) → review+
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/20863de3b8f4
Sync records creation failures are reported to telemetry. r=markh
Backed out for eslint failure: redeclaration of collection in deeper scope in test_syncengine_sync.js:

https://hg.mozilla.org/integration/autoland/rev/b5acbffc8ebf1472c5e6d6f85acdf52e142b0d31

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=20863de3b8f47cdd4760237c5e63fb0489a0c093
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=73299838&repo=autoland

> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/services/sync/tests/unit/test_syncengine_sync.js:1535:37 | 'collection' is already declared in the upper scope. (no-shadow)
Flags: needinfo?(eoger)
Thank you, sorry about that.
Flags: needinfo?(eoger)
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6ec1eae0161
Sync records creation failures are reported to telemetry. r=markh
https://hg.mozilla.org/mozilla-central/rev/d6ec1eae0161
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
You need to log in before you can comment on or make changes to this bug.