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)

VERIFIED FIXED in Firefox 26, Firefox OS v1.2

Status

()

P1
normal
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: pauly, Assigned: benjamin)

Tracking

Trunk
mozilla28
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26+ verified, firefox27+ verified, firefox28 verified, b2g-v1.2 fixed)

Details

(Whiteboard: [CtpDefault:P2], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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."

Updated

6 years ago
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Version: 14 Branch → Trunk
Is this reproduced on Nightly 18?
(Reporter)

Comment 2

6 years ago
Yes, on Nightly 18.0a1 (2012-09-13).
(Assignee)

Updated

6 years ago
Priority: -- → P3
Whiteboard: not important for CTP blocklisting

Comment 3

5 years ago
The same in Beta 21
Whiteboard: not important for CTP blocklisting → not important for CTP blocklisting [CtpDefault:P3]
(Reporter)

Updated

5 years ago
Duplicate of this bug: 924013
(Assignee)

Comment 5

5 years ago
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.
tracking-firefox26: --- → ?
Depends on: 926605
Whiteboard: not important for CTP blocklisting [CtpDefault:P3] → [CtpDefault:P3]
(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.
tracking-firefox26: ? → +
(Reporter)

Comment 7

5 years ago
Should this be fixed by bug 926605? Because it isn't on 28.0a1 (2013-10-29).
(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?
Flags: needinfo?(benjamin)
(Assignee)

Comment 9

5 years ago
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/
Flags: needinfo?(benjamin) → needinfo?(jschoenick)
(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.
Flags: needinfo?(jschoenick)
(Assignee)

Comment 11

5 years ago
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.
(Assignee)

Updated

5 years ago
Assignee: nobody → benjamin
Priority: P3 → P1
Summary: Google Earth Plugin doesn't work with click-to-play enabled → 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)
Whiteboard: [CtpDefault:P3] → [CtpDefault:P2]
(Assignee)

Comment 12

5 years ago
Created attachment 824307 [details] [diff] [review]
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
(Assignee)

Comment 13

5 years ago
Created 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,

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).
Attachment #824307 - Attachment is obsolete: true
Attachment #824404 - Flags: review?(jaws)
(Assignee)

Comment 14

5 years ago
Note: this patch fixes Google Earth. It does not completely fix www.mojebanka.cz, but that is because of bug 932633.
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);
Attachment #824404 - Attachment description: 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, → 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,
Attachment #824404 - Flags: review?(jaws) → review+
(Assignee)

Comment 16

5 years ago
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!
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/9da009cbc817
Followup discovered via tests: https://hg.mozilla.org/integration/fx-team/rev/4f6e927ac8f4
Target Milestone: --- → mozilla28
(Assignee)

Comment 19

5 years ago
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.
Attachment #824404 - Flags: approval-mozilla-beta?
Attachment #824404 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Depends on: 934503
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.
Attachment #824404 - Flags: approval-mozilla-beta?
Attachment #824404 - Flags: approval-mozilla-beta+
Attachment #824404 - Flags: approval-mozilla-aurora?
Attachment #824404 - Flags: approval-mozilla-aurora+
Paul, please ensure this gets verified across all fixed branches.
Keywords: verifyme
(Reporter)

Comment 22

5 years ago
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?
(Assignee)

Comment 23

5 years ago
No, the error message is presented by the page and we cannot change it.
(Reporter)

Comment 24

5 years ago
Verified fixed 28.0a1 (2013-11-04), Win 7, Mac OS X 10.8. Google Earth is N/A for Linux.
Setting status flags in the hopes that RyanVM will see this and we'll get uplift (but ni? him just in case)
status-firefox26: --- → affected
status-firefox27: --- → affected
status-firefox28: --- → fixed
tracking-firefox27: --- → +
Flags: needinfo?(ryanvm)
Ben told me a couple days ago that he was going to handle the uplifts on these...
Flags: needinfo?(ryanvm) → needinfo?(benjamin)
(Reporter)

Comment 29

5 years ago
based on comment 24
status-firefox28: fixed → verified
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
status-firefox26: fixed → verified
Verified as fixed on Win 7 64-bit with latest Aurora, build ID: 20131121004004
status-firefox27: fixed → verified
(Assignee)

Comment 32

5 years ago
FWIW, Google Earth also pushed out a server-side fix for this today so that Firefox users will see the plugin activation inline.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

5 years ago
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.