551572 - 100% CPU when sitting on merge-choice screen

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Ed Lee :Mardak]

If the user doesn't pick a sync direction and/or restarts, Weave will keep trying to sync and fail continuously.

Comment 1

[Mike Connor [:mconnor]]

Attachment: v1

Bah.

Comment 2

[Ed Lee :Mardak]

Comment on attachment 436123 [details] [diff] [review]
v1

>+++ b/source/modules/service.js
>         default:
>+          this.nextSync = 0;
>           this._scheduleNextSync();
So this will schedule the next sync to be probably 24 hours from now? Why was this sync getting triggered in the first place and not just waiting for the merge type to get picked? Auto-connected after restart without picking a merge? Sync scheduled because we do a connect before getting to the merge screen?

Comment 3

[Mike Connor [:mconnor]]

Attachment: v2 - better

* Don't schedule more syncs, and log the abort.
* Treat this state as a "needs setup" case and show the "Set Up Weave..." statusbar message.

Comment 4

[Ed Lee :Mardak]

Comment on attachment 437213 [details] [diff] [review]
v2 - better

>+++ b/source/chrome/content/sync.js
>+    let notSetup = errors.indexOf(Weave.Status.login) != -1;
Could this be called loginError or something else? notSetup sounds kinda weird and isn't quite what the RHS is.

>+++ b/source/modules/service.js
>       switch(Svc.Prefs.get("firstSync")) {
>         default:
>-          this._scheduleNextSync();
>+          this._log.debug("sync() called before setup complete, exiting");
>           return;
That works, but we should probably have _checkSync() return a don't-sync reason if pref("firstSync") == "notReady". So syncs won't get scheduled and a normal sync won't run either.

Comment 5

[Mike Connor [:mconnor]]

Attachment: v3 - the bettering

* moves the check to _checkSync
* moves _checkSync to be up front, as we should do nothing if this check fails
* moves cluster check to be after checkSync, but before firstSync handling, so we don't wipe local before we know we're syncing etc...
* changes variable to loginError per comments

Comment 6

[Ed Lee :Mardak]

Comment on attachment 437749 [details] [diff] [review]
v3 - the bettering

(In reply to comment #5)
> * moves cluster check to be after checkSync, but before firstSync handling, so
> we don't wipe local before we know we're syncing etc...
I don't see how this fixes the wipe local stuff. remoteSetup still happens after firstSync stuff. But this can be done in the other bug. Basically I think we want get("firstSync") == "wipeRemote" before remoteSetup and get("firstSync") == "wipeClient" after remoteSetup.

Comment 7

[Mike Connor [:mconnor]]

It doesn't fix all of it, but it fixes the "wipelocal happens before you get a node assigned" case, and similar ones.  It's just less-stupid, it doesn't fix the remoteSetup case.  It also means we don't call wipeRemote without having a clusterURL :)

Comment 8

[Mike Connor [:mconnor]]

http://hg.mozilla.org/labs/weave/rev/21c361d9d799

Comment 9

[Tracy Walker [:tracy]]

verified with 1.3b5