Closed Bug 745187 Opened 8 years ago Closed 6 years ago

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)

Categories

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

x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 + verified
firefox27 + verified
firefox28 --- verified
b2g-v1.2 --- fixed

People

(Reporter: pauly, Assigned: benjamin)

References

()

Details

(Whiteboard: [CtpDefault:P2])

Attachments

(1 file, 1 obsolete file)

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."
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Version: 14 Branch → Trunk
Is this reproduced on Nightly 18?
Yes, on Nightly 18.0a1 (2012-09-13).
Priority: -- → P3
Whiteboard: not important for CTP blocklisting
The same in Beta 21
Whiteboard: not important for CTP blocklisting → not important for CTP blocklisting [CtpDefault:P3]
Duplicate of this bug: 924013
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.
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.
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)
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)
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: 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]
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)
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+
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 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?
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
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?
No, the error message is presented by the page and we cannot change it.
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)
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)
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
Verified as fixed on Win 7 64-bit with latest Aurora, build ID: 20131121004004
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
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.