1047316 - Fix SharedFrame docshell swapping for SocialAPI (not Loop)

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproducible with:
Firefox 32 Beta 3 - 20140731191115 - User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0

Environment: Windows 7 x64, Windows XP x86, Windows 8 x64

Steps to reproduce:
1. Open Firefox with new profile.
2. Go to http://mixedpuppy.github.io/socialapi-demo/ and choose to activate the Demo Social Service.
3. Click the "Sign in" button to automatically sign in.
4. Open a new tab, go to facebook.com, and click the flag (social marking) icon at the top right.
5. Open a new tab and go to facebook.com ---> the social marking icon at the top right will display the red flag as expected = OK
6. Ctrl+N to open a new window and go to facebook.com ---> the social marking icon at the top right will display the red flag as expected = OK
7. Click the social marking button in the new window ---> nothing happens (expected a panel to open) = NOT OK
8. Click the social marking button again in the new window ---> the buttons turns from the red flag icon to a mozilla firefox icon (expected a panel to open) = NOT OK
9. Close the new window, then click the social marking button in the two tabs (steps #4 & #5) ---> a panel opens as expected but the red flag icon also temporarily turns into the mozilla firefox icon = NOT OK (note than when doing this on Win 8 x64, the whole bar and content moves, which is another issue)

Issues summary:
a) the social marking icon does not open the panel when clicked in a new window
b) the social marking icon changes to mozilla Firefox icon when clicked for a second time in a new window
c) after a) & b) the mozilla Firefox icon will continue to appear for a second after clicking the social marking icon
d) on Windows 8 x64, at step c), the whole menu bar and content move whenever clicking the social marking icon

Notes:
- see "http://www.screencast.com/t/Lcm1Emx5N5" for a), b) and c) - Win 7
- see "http://www.screencast.com/t/ZPWIXnm1gsqY" for d) - Win 8
- this issue seems to also reproduce in Firefox 29

Comment 1

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

The issue also reproduces in the latest Firefox 33 Aurora and latest Firefox 34 Nightly.

Comment 2

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

[Tracking Requested - why for this release]:

Comment 3

[Shane Caraveo (:mixedpuppy)]

console error:

JavaScript error: resource:///modules/SharedFrame.jsm, line 137: NS_ERROR_NOT_IMPLEMENTED:

This is failing in our SharedFrame code, which also affects Loop (I tested).  I'm pretty certain this has something to do with docshell swapping since back in the day we got these errors.

Comment 4

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

Spoke with Shane, who said that this needs to be fixed. Marking as tracking+ and assigning to Shane.

Comment 5

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

Shane - If this fix really needs to be in Firefox 32, we need a fix immediately as the final beta goes to build on Thu. Any progress?

Comment 6

[Shane Caraveo (:mixedpuppy)]

I've tracked this down to failing here:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1228

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: fix social marks button

This patch removes frame swapping from the social marks button. I'm opting to fix the marks button by not using docshell swapping.  The primary reason to swap is that the content is the same in every window, so we avoid the overhead of multiple iframes with the same content.  Since share and marks are loaded per-tab, and unloaded when closed, swapping doesn't help here anyway. (share already does not swap docshells). 

This does not fix the doscshell swap issue for the status button or for Loop.

Comment 8

[Shane Caraveo (:mixedpuppy)]

docshell wrapping requires that the iframe has been visible at least once (I'm assuming that some initialization happens at that time), and that we do not make it non-visible (e.g. style="display: none").  

In the first case, where the button (loop or socialapi) is in the toolbar, the first attempt at docshell swap fails.  I'm able to fix this by delaying the swap until popupshown has happened.

In the second case, where the button is in the menu-panel, the issue is a bit more severe.  The parent panelview gets its style set to display:none.  I've been able to show that I can make docshell swap work by setting display:block on the panelview then initiating the swap (this is done via browser console).  This makes a solution much more difficult.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: dont use menupanel

Actually, this is a simple fix.  We just wont use panelview in the menu panel, instead our own panels appear after clicking on the buttons (this is what addons in general do).  

If Loop really wants that UI, there is a longer time frame to figure out how to address the problem.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8475453 [details] [diff] [review]
fix social marks button

alternate reviewer

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8475555 [details] [diff] [review]
dont use menupanel

alternate reviewer

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8475555 [details] [diff] [review]
dont use menupanel

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

Tested it and it works fine. It's too bad we can't use a subview but given the time constraints this is reasonable.

::: browser/modules/PanelFrame.jsm
@@ +116,5 @@
>      let widgetGroup = CustomizableUI.getWidget(aToolbarButton.getAttribute("id"));
>      let widget = widgetGroup.forWindow(aWindow);
>      let anchorBtn = widget.anchor;
>  
>      // if we're a slice in the hamburger, use that panel instead

this comment seems out of date now (but i do like the "slice in the hamburger")

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8475453 [details] [diff] [review]
fix social marks button

>diff --git a/browser/base/content/socialmarks.xml b/browser/base/content/socialmarks.xml
>-              "origin": this.getAttribute("origin"),
>-              "src": "about:blank",
>-              "style": "width: " + width + "px; height: " + height + "px;",
>+          iframe.setAttribute("origin", provider.origin);
>+          iframe.setAttribute("style", "width: " + width + "px; height: " + height + "px;");

How come "src" isn't being set anymore?

>-        // Clear dimensions on all browsers so the panel size will
>-        // only use the selected browser.
>-        let frameIter = panel.firstElementChild;
>-        while (frameIter) {
>-          frameIter.collapsed = (frameIter != this.content);
>-          frameIter = frameIter.nextElementSibling;
>-        }

Why is this not needed anymore?

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> Comment on attachment 8475453 [details] [diff] [review]
> fix social marks button
> 
> >diff --git a/browser/base/content/socialmarks.xml b/browser/base/content/socialmarks.xml
> >-              "origin": this.getAttribute("origin"),
> >-              "src": "about:blank",
> >-              "style": "width: " + width + "px; height: " + height + "px;",
> >+          iframe.setAttribute("origin", provider.origin);
> >+          iframe.setAttribute("style", "width: " + width + "px; height: " + height + "px;");
> 
> How come "src" isn't being set anymore?

missed it, will add it.  it's actually not necessary since it is set upon use, but better to keep it the same for now.

> >-        // Clear dimensions on all browsers so the panel size will
> >-        // only use the selected browser.
> >-        let frameIter = panel.firstElementChild;
> >-        while (frameIter) {
> >-          frameIter.collapsed = (frameIter != this.content);
> >-          frameIter = frameIter.nextElementSibling;
> >-        }
> 
> Why is this not needed anymore?

That code handles panels that hold more than one iframe, which only happens if you're using SharedFrame.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8475453 [details] [diff] [review]
fix social marks button

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

r=me with src added back

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: fix social marks button

updated from review comments.   note: beta patch will be slightly different, will post shortly

Comment 17

[Shane Caraveo (:mixedpuppy)]

Attachment: dont use menupanel

comment updated

Comment 18

[Shane Caraveo (:mixedpuppy)]

fx-team try https://tbpl.mozilla.org/?tree=Try&rev=af1a5bcb773c
aurora try https://tbpl.mozilla.org/?tree=Try&rev=c178e0cba008

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: fix social marks button (beta patch)

beta-specific version of patch, depends on bug 1056415

try: https://tbpl.mozilla.org/?tree=Try&rev=17368a857389

Comment 20

[Shane Caraveo (:mixedpuppy)]

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

Comment 21

[Shane Caraveo (:mixedpuppy)]

That last push was the patch that doesn't need beta uplift (just aurora), here's the one that beta needs:

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

Comment 22

[u279076]

Florin, this should be landing across branches and uplifting to Beta tonight for the 32.0b9. Please make sure this gets tested as part of your sign offs.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 24

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8476182 [details] [diff] [review]
fix social marks button

Approval Request Comment
[Feature/regressing bug #]: socialmarks
[User impact if declined]: will only work in first used window for multiple window users
[Describe test coverage new/current, TBPL]: manual testing
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8476372 [details] [diff] [review]
fix social marks button (beta patch)

Approval Request Comment
[Feature/regressing bug #]: socialmarks
[User impact if declined]: will only work in first used window for multiple window users
[Describe test coverage new/current, TBPL]: manual testing
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8476184 [details] [diff] [review]
dont use menupanel

Approval Request Comment
[Feature/regressing bug #]: loop and social api status buttons
[User impact if declined]: will only work in first used window for multiple window users
[Describe test coverage new/current, TBPL]: manual testing
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 27

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

Comment on attachment 8476372 [details] [diff] [review]
fix social marks button (beta patch)

Thanks to Shane for verifying this fix on fx-team. beta+

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b6635ee27fbe
https://hg.mozilla.org/releases/mozilla-aurora/rev/7e73d586709b

https://hg.mozilla.org/releases/mozilla-beta/rev/58eb677e55f3

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9dc1be8e11e2

Comment 30

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Windows 7 32bit, Windows 8 64bit, Windows XP 32bit using Firefox 32 Beta 9 (buildID: 20140822024446), latest Aurora 33.0a2 (buildID: 20140823004001) and latest Nightly 34.0a1 (buildID: 20140824030210).