Closed Bug 1241851 Opened 4 years ago Closed 4 years ago
“Fetching Synced Tabs” panel is continuously displayed
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.
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.
Priority: -- → P1
[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.
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?
Assignee: nobody → markh
Status: NEW → ASSIGNED
Attachment #8714175 - Flags: review?(gijskruitbosch+bugs)
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 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
Attachment #8714175 - Flags: review?(gijskruitbosch+bugs) → review+
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 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.
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.
You need to log in before you can comment on or make changes to this bug.