1241851 - “Fetching Synced Tabs” panel is continuously displayed

Status: VERIFIED FIXED

(Firefox :: Sync, defect, P1)

Description

[Vasilica Tamas, QA [:vtamas]]

Reproducible on: Firefox 46.0a1 and Firefox 45.0a2 across all platforms

1.Launch Firefox with a clean profile.
2.Move the "Synced tabs" icon to tab bar using “Customize" mode.
3.Click on “Synced tabs” button from tab bar.
4.Click on “Sign in to Sync” button.
5.Drag the “Synced tabs” icon back to Panel Menu using “Customize” mode.
6.Create a new Firefox Account using the “about:preferences?entrypoint=syncbutton#sync” page which has been opened on step 4.
7.Confirm the account and check the “Synced tabs” panel.

ER
“No client” panel is successfully displayed.
See screenshot: http://i.imgur.com/5vlYNxs.png


AR
“Fetching Synced Tabs” panel is continuously displayed and the “No client” panel never appears.
See screenshot: http://i.imgur.com/9c0kbzk.png


Additional notes:
- This issue is reproducible on Firefox 46.0a1 (2016-01-21) and Firefox 45.0a2 (2015-01-21) under Windows 10 64-bit, Mac OS X 10.10.5 and Ubuntu 14.04 32-bit.
- There are no errors thrown in Browser Console.
- A browser restarting solves the problem.

Comment 1

[Chris Karlof [:ckarlof]]

Attachment: fetching synced tabs.png

Comment 2

[Chris Karlof [:ckarlof]]

I can also trigger this by:

1) Start Nightly (20160121030208) with a Sync profile with 2+ connected devices and the Synced Tabs icon in the toolbar. 
2) Immediately and repeatedly click on the on the Synced Tabs icon. Eventually it shows an expanded Synced Tabs panel, but it's empty and gets stuck on showing "Fetching Synced Tabs" (https://bug1241851.bmoattachments.org/attachment.cgi?id=8711116). 

If I had to guess, I suspect this might be related to how Sync initializes on browser start.

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

[Tracking Requested - why for this release]:
Marked as a major issue by QA. This is the type of quality issue that is noticeable by users and that we want to fix to make the product great.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-1241851-avoid-using-XBL-property-setters-before-.patch

Note that this only reproduces when the panel is anchored to a toolbar button (ie, not when the item is in the hamburger menu). Any attempt to change the .selectedIndex on the deck after the panel is closed for the first time fails to work.

This appears to be caused by the interaction between the panel UI and XBL bindings. Specifically, the first time the panel is shown the bindings are created before onViewShowing is called. The binding is then destroyed just after onViewHiding is called, but the *next* onViewHiding call the binding is yet to be created. In onViewShowing our code does |deck.selectedIndex = something;| and if the binding is yet to be created, it seems we end up with .selectedIndex being an "own property" which neuters the property setter in the XBL.

I came to the above conclusion by adding a constructor and destructor in general.xml like so:

      <constructor><![CDATA[
        dump("DECK CREATED: " + this.getAttribute("id") + "\n");
      ]]></constructor>
      <destructor><![CDATA[
        dump("DECK DESTROYED: " + this.getAttribute("id") + "\n");
      ]]></destructor>

and dump() in the selectedIndex getter and setter. I also added statements in onViewShowing/onViewHiding:

      dump("VS has setter: " + !!deck.__lookupGetter__("selectedIndex") + "/" + deck.hasOwnProperty("selectedIndex") + "\n");

If the constructor has fired when onViewShowing is called the __lookupGetter__ finds the XBL setter (and hasOwnProperty returns false). If we set .selectedIndex, hasOwnProperty() then starts returning true (which it never should) and __lookupGetter__ returns undefined.

The simplest way around this seems to be to skip the setter and do |deck.setAttribute("selectedIndex", selectedIndex);| - when the binding is created that attribute is used.

Gijs, does this sound OK to you, or do you have any other ideas?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8714175 [details] [diff] [review]
0003-Bug-1241851-avoid-using-XBL-property-setters-before-.patch

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +358,5 @@
>        let doc = aEvent.target.ownerDocument;
>        this._tabsList = doc.getElementById("PanelUI-remotetabs-tabslist");
>        Services.obs.addObserver(this, SyncedTabs.TOPIC_TABS_CHANGED, false);
>  
>        let deck = doc.getElementById("PanelUI-remotetabs-deck");

oops - this local var can die (and also one in __showTabs that isn't shown here)

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8714175 [details] [diff] [review]
0003-Bug-1241851-avoid-using-XBL-property-setters-before-.patch

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

Nice detective work. r=me

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ae8f65f0c076

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae8f65f0c076

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8714175 [details] [diff] [review]
0003-Bug-1241851-avoid-using-XBL-property-setters-before-.patch

Approval Request Comment
[Feature/regressing bug #]: Synced Tabs menu
[User impact if declined]: The menu may get stuck in a state and thus not show the user's synced tabs.
[Describe test coverage new/current, TreeHerder]: Existings tests pass.
[Risks and why]: Simple patch, risk limited to synced tabs menu.
[String/UUID change made/needed]: None

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8714175 [details] [diff] [review]
0003-Bug-1241851-avoid-using-XBL-property-setters-before-.patch

Fix a tracked bug, taking it.
Should be in 45 beta 3.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96e334716a64

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b4faf24a314

Comment 13

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 47.0a1 (2016-01-11) under Ubuntu 12.04 64-bit.

Verified fixed on Firefox 47.0a1 (216-02-18), Firefox 46.0a2 (2016-02-18) and Firefox 45 beta 7 (20160218171844) under Ubuntu 12.04 64-bit, Mac OS X 10.9.5 and Windows 10 64-bit.