Closed Bug 840643 Opened 9 years ago Closed 9 years ago

Work - Connect to Sync shows waiting for other device forever

Categories

(Firefox for Metro Graveyard :: General, defect)

21 Branch
All
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 22

People

(Reporter: verdi, Assigned: mbrubeck)

References

Details

(Whiteboard: feature=work)

Attachments

(2 files)

When I try to set up sync in Firefox Metro I can successfully add the code to Firefox on my Mac and get the success screen. But in Firefox Metro the "Connect to Sync" screen with the message "Waiting for other device..." never goes away. If I wait a 10 min and hit Cancel, the button doesn't respond. The only way to get out of this is to open the task manager and quit Firefox Metro.
Duplicate of this bug: 841710
We've got something wrong with all of our Message Dialogs (that piece of WIndows 8 UI) where they cannot be dismissed. This might be related to that. If so, it's a dupe of bug 840855.
Duplicate of this bug: 843461
Duplicate of this bug: 843885
Blocks: 831615
No longer blocks: metrov1triage
Summary: Connect to Sync shows waiting for other device forever → Work - Connect to Sync shows waiting for other device forever
Whiteboard: feature=work
Attached patch patchSplinter Review
The root cause was an exception thrown by this line, which prevents WeaveGlue._init from running and leaves our sync UI code in a bad state:
http://hg.mozilla.org/mozilla-central/file/f2ec16a9feea/browser/metro/base/content/sync.js#l31

This line was broken because of the ".bind(this)" two lines further down.  This means that "onReady" is not actually added as the observer; the observer is actually a bound function that calls onReady, and there's no way to remove it.

This patch fixes the exception by adding WeaveGlue as the observer instead of a bound function.
Assignee: nobody → mbrubeck
Status: NEW → ASSIGNED
Attachment #717356 - Flags: review?(ally)
Comment on attachment 717356 [details] [diff] [review]
patch

Review of attachment 717356 [details] [diff] [review]:
-----------------------------------------------------------------

r+*
This reasoning sounds sensible, the patch looks good, and patch works for me as expected. 

However, I'm going to double check with the sync owners to make sure this doesn't break a sync assumption. *(I doubt it, but please don't land until we hear back)
Attachment #717356 - Flags: review?(ally) → review+
talked to the sync group, you're good to go
Comment on attachment 717356 [details] [diff] [review]
patch

Setting an unowned r? flag as a reminder to myself to wait for a second review.
Attachment #717356 - Flags: review?
Comment on attachment 717356 [details] [diff] [review]
patch

Review of attachment 717356 [details] [diff] [review]:
-----------------------------------------------------------------

Hoping I suffice as the second reviewer.

::: browser/metro/base/content/sync.js
@@ +25,5 @@
>      if (service.ready) {
>        this._init();
> +    } else {
> +      Services.obs.addObserver(this, "weave:service:ready", false);
> +      service.ensureLoaded();

I have a gentle preference for early return, rather than else, but nbd.
Attachment #717356 - Flags: review? → review+
(Oops, obviously I missed comment 7 when adding comment 8.  But thanks for the review, Richard!)
https://hg.mozilla.org/mozilla-central/rev/f14dc834f18e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Beautiful! I just found this bug in my metro awesome bar after successfully syncing.
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.