Closed Bug 1335752 Opened 3 years ago Closed 3 years ago

Intermittent services/sync/tests/unit/test_syncengine_sync.js | xpcshell return code: 0

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 54
Tracking Status
firefox52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: Lina)

References

Details

(Keywords: intermittent-failure)

Attachments

(2 files)

Priority: -- → P2
At first blush, this looks similar to bug 1325523. Let's try disabling tracker persistence, or adding `engine.finalize()` calls to the cleanup functions.
Assignee: nobody → kit
Status: NEW → ASSIGNED
Priority: P2 → P1
Comment on attachment 8836887 [details]
Bug 1335752 - Disable persistence and clean up after `RotaryEngine` in tests.

https://reviewboard.mozilla.org/r/112204/#review113458

::: services/sync/modules-testing/rotaryengine.js:96
(Diff revision 1)
>  this.RotaryTracker = function RotaryTracker(name, engine) {
>    Tracker.call(this, name, engine);
>  }
>  RotaryTracker.prototype = {
> -  __proto__: Tracker.prototype
> +  __proto__: Tracker.prototype,
> +  persistChangedIDs: false,

I'm of two minds about whether we want to do this. On the one hand, we really don't care about persistence. Before bug 1319175, I think those calls were replaced by http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/services/sync/modules-testing/fakeservices.js#21-71, anyway.

On the other, keeping it enabled caught an issue where we weren't cleaning up observers after we unregistered the engine. (Visible as a flurry of "Unable to arm timer, the object has been finalized" errors in the logs). So maybe it's worth enabling persistence, and seeing what else shakes out?
Comment on attachment 8836886 [details]
Bug 1335752 - Remove all observers when finalizing the tracker.

https://reviewboard.mozilla.org/r/112202/#review113560

This seems sane to me.
Attachment #8836886 - Flags: review?(markh) → review+
Comment on attachment 8836887 [details]
Bug 1335752 - Disable persistence and clean up after `RotaryEngine` in tests.

https://reviewboard.mozilla.org/r/112204/#review113562

seems fine to make - consider these comments totally optional :)

::: services/sync/tests/unit/test_engine_abort.js:67
(Diff revision 1)
>    await promiseStopServer(server);
>    Svc.Prefs.resetBranch("");
>    Service.recordManager.clearCache();
> +
> +  engine._tracker.clearChangedIDs();
> +  engine.finalize();

I nearly mentioned in the previous patch that exposing finalize as a promise is fine (ie, making this call |await|, and having other callers of finalize spinning) - feel free to make that change, but I'm not too bothered by that either way.

::: services/sync/tests/unit/test_fxa_node_reassignment.js:256
(Diff revision 1)
>                                        "weave:service:sync:finish",
>                                        between,
>                                        "weave:service:sync:finish",
>                                        Service.storageURL + "rotary");
> +
> +  tracker.clearChangedIDs();

I wonder if we can trick finalize into auto unregistering itself (whereas IIUC, unregistering the engine currently does the finalize). My thinking is that it seems to be cleaner if we could just call engine.finalize() here (and this not even require registerRotaryEngine() to return the tracker?
Attachment #8836887 - Flags: review?(markh) → review+
Comment on attachment 8836887 [details]
Bug 1335752 - Disable persistence and clean up after `RotaryEngine` in tests.

https://reviewboard.mozilla.org/r/112204/#review113562

> I nearly mentioned in the previous patch that exposing finalize as a promise is fine (ie, making this call |await|, and having other callers of finalize spinning) - feel free to make that change, but I'm not too bothered by that either way.

Good call! Done.

> I wonder if we can trick finalize into auto unregistering itself (whereas IIUC, unregistering the engine currently does the finalize). My thinking is that it seems to be cleaner if we could just call engine.finalize() here (and this not even require registerRotaryEngine() to return the tracker?

I considered this, but decided not to. It seems like a surprising side effect for an engine to remove itself; I guess because we always create the engine and pass it to `EngineManager::register`, and having the caller explicitly remove it seems symmetrical. We'd still want to call `finalize` in `EngineManager::unregister` (and `clear`) in any case. I'd suggest keeping this as is, but I don't feel strongly about it, either. :-)
(In reply to Kit Cambridge [:kitcambridge] from comment #10)
> I considered this, but decided not to.

SGTM!
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d5f1ce4b34d5
Remove all observers when finalizing the tracker. r=markh
https://hg.mozilla.org/integration/autoland/rev/0577296908e7
Disable persistence and clean up after `RotaryEngine` in tests. r=markh
https://hg.mozilla.org/mozilla-central/rev/d5f1ce4b34d5
https://hg.mozilla.org/mozilla-central/rev/0577296908e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
See Also: → 1414181
You need to log in before you can comment on or make changes to this bug.