Closed
Bug 1335752
Opened 8 years ago
Closed 8 years ago
Intermittent services/sync/tests/unit/test_syncengine_sync.js | xpcshell return code: 0
Categories
(Firefox :: Sync, defect, P1)
Firefox
Sync
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)
Comment hidden (Intermittent Failures Robot) |
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 2•8 years ago
|
||
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 hidden (Intermittent Failures Robot) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•8 years ago
|
||
mozreview-review |
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 7•8 years ago
|
||
mozreview-review |
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 8•8 years ago
|
||
mozreview-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 hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 10•8 years ago
|
||
mozreview-review-reply |
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. :-)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
(In reply to Kit Cambridge [:kitcambridge] from comment #10)
> I considered this, but decided not to.
SGTM!
Comment hidden (mozreview-request) |
Comment 15•8 years ago
|
||
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
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d5f1ce4b34d5
https://hg.mozilla.org/mozilla-central/rev/0577296908e7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Updated•8 years ago
|
status-firefox52:
--- → unaffected
status-firefox53:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•