745187 - Click-to-activate plugins which are removed immediately after adding don't trigger the plugin notification (Google Earth Plugin doesn't work with click-to-play enabled)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Paul Silaghi, QA [:pauly]]

1. Start Firefox 14 or higher on a clean profile
2. Make sure plugins.click_to_play pref is set on TRUE in about:config
3. Go to http://maps.google.com/ and select the "Earth" view

Expected results:
"Click to activate the plugin" notification should occur

Actual results:
"There was a problem with the Google Earth Plugin.
Exit the browser completely (Cmd-Q on Mac; Alt-F x on Windows), then re-open this page to see it in action.
If that doesn't help, you can re-install the Google Earth Plugin using this link."

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Is this reproduced on Nightly 18?

Comment 2

[Paul Silaghi, QA [:pauly]]

Yes, on Nightly 18.0a1 (2012-09-13).

Comment 3

[Dimas]

The same in Beta 21

Comment 5

[Benjamin Smedberg]

The right long-term solution here is bug 926605. In the meantime, this site is high-profile enough that we should see if we can get Google to deploy a page fix to leave the GE plugin visible and have a callback.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> The right long-term solution here is bug 926605. In the meantime, this site
> is high-profile enough that we should see if we can get Google to deploy a
> page fix to leave the GE plugin visible and have a callback.

It's tech evangelism so it can't be a hard blocker but I know you're already talking on the mozilla-google discussion list and we can see if there's a solution offered in time for FF26.

Comment 7

[Paul Silaghi, QA [:pauly]]

Should this be fixed by bug 926605? Because it isn't on 28.0a1 (2013-10-29).

Comment 8

[u279076]

(In reply to Paul Silaghi, QA [:pauly] from comment #7)
> Should this be fixed by bug 926605? Because it isn't on 28.0a1 (2013-10-29).

I believe it was expected to resolve this, Benjamin?

Comment 9

[Benjamin Smedberg]

It did not resolve it. I debugged this and the sequence of events is this:

* GEarth creates the plugin using a document.writing an empty <embed type="...">
* If the plugin is not scripted, it is immediately removed from the document

This breaks the plugin UI code because the way we currently detect that a plugin was used is by waiting for the XBL fallback binding to attach and having it fire PluginBindingAttached in its constructor.

Johns, do you know why we are doing it this way instead of firing the event at the document? I think it has something to do with completing layout so that we can tell whether the instance is hidden or not.

This kind of bug may also affect Java sites, I'm seeing something that looks similar on https://www.mojebanka.cz/InternetBanking/

Comment 10

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] (Away 24-25 October) from comment #9)
> This breaks the plugin UI code because the way we currently detect that a
> plugin was used is by waiting for the XBL fallback binding to attach and
> having it fire PluginBindingAttached in its constructor.

We did it this way because the front end needs to know when the XBL items exist in order to install event handlers and the like -- previously OBJLC would fire a plugin-entered-fallback event and it would race with layout and cause much sadness.

Bug 920527 exists to add properly paired PluginAdded/PluginRemoved events, which I think would satisfy this use case.

Comment 11

[Benjamin Smedberg]

Further debugging shows that we are creating the XBL binding (synchronously), so the primary problem is the setTimeout at http://hg.mozilla.org/mozilla-central/annotate/35b73bb96ca0/toolkit/mozapps/plugins/content/pluginProblem.xml#l83 which is dispatching the event after the plugin has been removed from the doc.

I added this setTimeout, so I'm going to try and remove it and work around the layout/hidden instance issues elsewhere.

Comment 12

[Benjamin Smedberg]

Attachment: Not requesting review yet, because I haven't actually verified that this fixes google earth, just my testcase based on some debugging evidence. Try run at https://tbpl.mozilla.org/?tree=Try&rev=be9e6b8d507e

Comment 13

[Benjamin Smedberg]

Attachment: Don't introduce a delay notifying the frontend about new plugins added to the document, because script may immediately remove them from the page. To fix the delayed layout of XBL, introduce a separate method to calculate the notification icon visibility,

Note: this patch still has one potential hole: if content creates a plugin and destroys it without forcing reflow, the XBL binding will not be instantiated. This doesn't appear to be a problem in practice, because every site that wants to use a plugin also has to trigger frame construction to get the plugin to instantiate (document.write of the embed element appears to do this automatically).

Comment 14

[Benjamin Smedberg]

Note: this patch fixes Google Earth. It does not completely fix www.mojebanka.cz, but that is because of bug 932633.

Comment 15

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

Comment on attachment 824404 [details] [diff] [review]
Don't introduce a delay notifying the frontend about new plugins added to the document, because script may immediately remove them from the page. To fix the delayed layout of XBL, introduce a separate method to calculate the notification icon visibility,

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

r=me with the following nits addressed.

::: browser/base/content/browser-plugins.js
@@ +868,5 @@
> +      let fallbackType = pluginInfo.fallbackType;
> +      if (fallbackType == Ci.nsIObjectLoadingContent.PLUGIN_VULNERABLE_UPDATABLE ||
> +          fallbackType == Ci.nsIObjectLoadingContent.PLUGIN_VULNERABLE_NO_UPDATE ||
> +          fallbackType == Ci.nsIObjectLoadingContent.PLUGIN_BLOCKLISTED) {
> +        iconClasses.toggle("plugin-blocked", true);

We should only be using classList.toggle when the second argument is a variable and not a constant. In the constant case, it is easier to read if the code is just classList.add() or classList.remove().

@@ +880,5 @@
> +    let contentWindow = aBrowser.contentWindow;
> +    let contentDoc = aBrowser.contentDocument;
> +    let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                           .getInterface(Ci.nsIDOMWindowUtils);
> +    for (let plugin of cwu.plugins) {

cwu.plugins is computed each time it is read, so store the result of cwu.plugins in a local variable and then iterate on that.

@@ +900,5 @@
> +          break;
> +        }
> +      }
> +    }
> +    

nit, whitespace

@@ +905,5 @@
> +    if (actions.size != 0) {
> +      iconClasses.toggle("plugin-hidden", true);
> +    }
> +    else {
> +      iconClasses.toggle("plugin-hidden", false);

These four lines can be merged to just:
iconClasses.toggle("plugin-hidden", !actions.size);

Comment 16

[Benjamin Smedberg]

Didn't change the cwu.plugins usage, because for .. of only computes the operand once, so we're not going to be refetching cwu.plugins each time and it isn't used elsewhere in the function. Fixed the rest, thanks!

Comment 17

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/9da009cbc817
Followup discovered via tests: https://hg.mozilla.org/integration/fx-team/rev/4f6e927ac8f4

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9da009cbc817
https://hg.mozilla.org/mozilla-central/rev/4f6e927ac8f4

Comment 19

[Benjamin Smedberg]

Comment on attachment 824404 [details] [diff] [review]
Don't introduce a delay notifying the frontend about new plugins added to the document, because script may immediately remove them from the page. To fix the delayed layout of XBL, introduce a separate method to calculate the notification icon visibility,

[Approval Request Comment] Approval summary being sent to firefox-dev and r-d.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 824404 [details] [diff] [review]
Don't introduce a delay notifying the frontend about new plugins added to the document, because script may immediately remove them from the page. To fix the delayed layout of XBL, introduce a separate method to calculate the notification icon visibility,

Approving for early beta uplift to increase time with population & with QA testing.

Comment 21

[u279076]

Paul, please ensure this gets verified across all fixed branches.

Comment 22

[Paul Silaghi, QA [:pauly]]

I was almost going to say this is not fixed, only then I noticed the CTP doorhanger. "There was a problem with the Google Earth Plugin" error message is still displayed over the plugin content, but clicking on the doorhanger/Allow will make the Google Earth work. Isn't there something that can be done with the error message?

Comment 23

[Benjamin Smedberg]

No, the error message is presented by the page and we cannot change it.

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed 28.0a1 (2013-11-04), Win 7, Mac OS X 10.8. Google Earth is N/A for Linux.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Setting status flags in the hopes that RyanVM will see this and we'll get uplift (but ni? him just in case)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Ben told me a couple days ago that he was going to handle the uplifts on these...

Comment 27

[Benjamin Smedberg]

https://hg.mozilla.org/releases/mozilla-aurora/rev/91a15288b911
https://hg.mozilla.org/releases/mozilla-aurora/rev/95b5e9fcccee
https://hg.mozilla.org/releases/mozilla-beta/rev/422b062772d1
https://hg.mozilla.org/releases/mozilla-beta/rev/566e58c7e559
https://hg.mozilla.org/releases/mozilla-beta/rev/2651bf796544

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/422b062772d1
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/566e58c7e559
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/2651bf796544

Comment 29

[Paul Silaghi, QA [:pauly]]

based on comment 24

Comment 30

[Mihai Morar, (:MihaiMorar)]

Verified fixed on FF26b3 using Windows 7 x64 BuildID: 20131107161719
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0

Comment 31

[Manuela Muntean [Away]]

Verified as fixed on Win 7 64-bit with latest Aurora, build ID: 20131121004004

Comment 32

[Benjamin Smedberg]

FWIW, Google Earth also pushed out a server-side fix for this today so that Firefox users will see the plugin activation inline.