Closed
Bug 745187
Opened 13 years ago
Closed 11 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 Graveyard :: Plug-ins, defect, P1)
Tracking
(firefox26+ verified, firefox27+ verified, firefox28 verified, b2g-v1.2 fixed)
VERIFIED
FIXED
mozilla28
People
(Reporter: pauly, Assigned: benjamin)
References
()
Details
(Whiteboard: [CtpDefault:P2])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jaws
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•13 years ago
|
Component: General → Plug-ins
Product: Firefox → Core
QA Contact: general → plugins
Version: 14 Branch → Trunk
Comment 1•12 years ago
|
||
Is this reproduced on Nightly 18?
Reporter | ||
Comment 2•12 years ago
|
||
Yes, on Nightly 18.0a1 (2012-09-13).
Assignee | ||
Updated•12 years ago
|
Priority: -- → P3
Whiteboard: not important for CTP blocklisting
Updated•12 years ago
|
Whiteboard: not important for CTP blocklisting → not important for CTP blocklisting [CtpDefault:P3]
Assignee | ||
Comment 5•11 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]
Comment 6•11 years ago
|
||
(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.
Reporter | ||
Comment 7•11 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•11 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)
Comment 10•11 years ago
|
||
(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•11 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•11 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•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
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•11 years ago
|
||
Note: this patch fixes Google Earth. It does not completely fix www.mojebanka.cz, but that is because of bug 932633.
Comment 15•11 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,
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•11 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•11 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
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9da009cbc817
https://hg.mozilla.org/mozilla-central/rev/4f6e927ac8f4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•11 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?
Comment 20•11 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,
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+
Comment 21•11 years ago
|
||
Paul, please ensure this gets verified across all fixed branches.
Keywords: verifyme
Reporter | ||
Comment 22•11 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•11 years ago
|
||
No, the error message is presented by the page and we cannot change it.
Reporter | ||
Comment 24•11 years ago
|
||
Verified fixed 28.0a1 (2013-11-04), Win 7, Mac OS X 10.8. Google Earth is N/A for Linux.
Comment 25•11 years ago
|
||
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)
Comment 26•11 years ago
|
||
Ben told me a couple days ago that he was going to handle the uplifts on these...
Flags: needinfo?(ryanvm) → needinfo?(benjamin)
Assignee | ||
Comment 27•11 years ago
|
||
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•11 years ago
|
||
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
status-b2g-v1.2:
--- → fixed
Reporter | ||
Comment 29•11 years ago
|
||
based on comment 24
Comment 30•11 years ago
|
||
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•11 years ago
|
||
Verified as fixed on Win 7 64-bit with latest Aurora, build ID: 20131121004004
Assignee | ||
Comment 32•11 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
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•