Closed Bug 916542 Opened 6 years ago Closed 6 years ago

Removing object nodes can trigger broken doorhanger

Categories

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

24 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 --- verified
firefox27 --- verified

People

(Reporter: nightstalkerz, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached image Untitled.png
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130915030208

Steps to reproduce:

1. Go to http://acid3.acidtests.org/
2. Wait for the test to finish

Firefox build: 26.0a1 (2013-09-15) Nightly on Windows 7


Actual results:

Plugin placeholder appeas. Clicking on it shows blank details. See screenshot


Expected results:

No plugin placeholder appears in the url bar
Confirm with Firefox 24 and newer. Caused by Test 16?
Component: Untriaged → Plug-ins
Product: Firefox → Core
Summary: acid3 plugin appears → acid3 test triggers broken plugin doorhanger
Version: 26 Branch → 24 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: 880735
This looks similar to bug 916593.
Attached file test-916542.html
Narrowed it down to a rather simple test-case.

* add at least two <object data="x.png"> to the DOM
* remove one of the objects
=> broken doorhanger

The notification is there until the last of the object is gone, so it looks like the plugin notification front-code does something wrong when re-checking the object/embed/... after one is removed.
Assignee: nobody → georg.fritzsche
Priority: -- → P2
Summary: acid3 test triggers broken plugin doorhanger → Removing object nodes can trigger broken doorhanger
(1) In |nsObjectLoadingContent::BindToTree()| we currently always call |aDocument->AddPlugin()| [1] without checking if the object type is actually a plugin.
(2) This means that non-plugin object elements show up in nsIDOMWindowUtils.plugins [2].
(3) Those things surfaced here because the "PluginRemoved" event also fires without checking whether the object is a plugin.

I'm assuming that (1) and probably (3) are bugs John?
Does it sound correct to fix (1) by calling nsIDocument::AddPlugin()/RemovePlugin():
* in BindToTree()/UnbindFromTree() only when mType==eType_Plugin
* whenever mType goes from or to eType_Plugin and the element is bound to a tree
... plus fire events to trigger the CtP frontend code.


[1] http://hg.mozilla.org/mozilla-central/annotate/f97307cb4c95/content/base/src/nsObjectLoadingContent.cpp#l685
[2] http://hg.mozilla.org/mozilla-central/annotate/8b4d14afc4f6/browser/base/content/browser-plugins.js#l794
Flags: needinfo?(jschoenick)
Perhaps this just means that we need to sanity-check the things in windowutils.plugins before considering them for the doorhanger...
Maybe we can do that short-term and fix the above behavior later?
At least to me it doesn't seem right to have non-plugin-elements listed in windowutils.plugins.
Alright, we'll do the front-end check now (and uplift to 26).
If the behavior from comment 4 is a bug it will be addressed in a follow-up.
(In reply to Georg Fritzsche [:gfritzsche] from comment #4)
> (1) In |nsObjectLoadingContent::BindToTree()| we currently always call
> |aDocument->AddPlugin()| [1] without checking if the object type is actually
> a plugin.
> (2) This means that non-plugin object elements show up in
> nsIDOMWindowUtils.plugins [2].

That list has always really been a "plugin-capable tags" list. It wouldn't be too hard to make it only contain tags configured as a plugin, but we'd need to audit its users.

> (3) Those things surfaced here because the "PluginRemoved" event also fires
> without checking whether the object is a plugin.

As I noted in bug 889788 comment 6 when PluginRemoved was added, it doesn't catch every case and can fire when no plugins were really removed from the page, among other things. The proper fix here is to setup a pair of PluginAdded PluginRemoved events that actually fire when we change type to/from eType_Plugin, and not enumerate every plugin on the page every time the doorhanger appears, or we're going to be chasing edge cases for a while.

> I'm assuming that (1) and probably (3) are bugs John?
> Does it sound correct to fix (1) by calling
> nsIDocument::AddPlugin()/RemovePlugin():
> * in BindToTree()/UnbindFromTree() only when mType==eType_Plugin
> * whenever mType goes from or to eType_Plugin and the element is bound to a
> tree
> ... plus fire events to trigger the CtP frontend code.

Assuming other users of document.plugins don't mind, yes - but there are numerous ways to spawn/stop a plugin, LoadObject/UnloadObject would be the proper place, or InstantiatePluginInstance/StopPluginInstance if we only want "live" plugins.
Flags: needinfo?(jschoenick)
Attached patch Front-end fix (obsolete) — Splinter Review
Fix the front-end code by guarding against non-plugin elements in windowutils.plugins.

https://tbpl.mozilla.org/?tree=Try&rev=78b51d6076da
Attachment #809216 - Flags: review?(jaws)
Attached patch Front-end fixSplinter Review
Sorry, missed stripping out a left-over.
Attachment #809216 - Attachment is obsolete: true
Attachment #809216 - Flags: review?(jaws)
Attachment #809217 - Flags: review?(jaws)
Blocks: 920527
Comment on attachment 809217 [details] [diff] [review]
Front-end fix

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

r=me with nits below fixed.

::: browser/base/content/browser-plugins.js
@@ +806,5 @@
>                             .getInterface(Ci.nsIDOMWindowUtils);
> +    // cwu.plugins may contain non-plugin <object>s, filter them out
> +    let plugins = cwu.plugins.filter(function(plugin) {
> +      return (plugin.getContentTypeForMIMEType(plugin.actualType) ==
> +              Ci.nsIObjectLoadingContent.TYPE_PLUGIN);

We can use a fat-arrow function here.

let plugins = cwu.plugins.filter((plugin) =>
  plugin.getContentTypeForMIMEType(plugin.actualType) == Ci.nsIObjectLoadingContent.TYPE_PLUGIN);

::: browser/base/content/test/general/browser_CTP_nonplugins.js
@@ +130,5 @@
> +  ok(popupNotification, "Test 1, Should have a click-to-play notification");
> +
> +  let plugin = gTestBrowser.contentDocument.getElementById("secondtestA");
> +  plugin.parentNode.removeChild(plugin);
> +  let plugin = gTestBrowser.contentDocument.getElementById("secondtestB");

You're redeclaring `plugin` here, you can just drop the `let ` at the beginning of this line.

@@ +146,5 @@
> +  let popupNotification = PopupNotifications.getNotification("click-to-play-plugins", gTestBrowser);
> +  ok(popupNotification, "Test 2, Should have a click-to-play notification");
> +
> +  let plugin = gTestBrowser.contentDocument.getElementById("test");
> +  plugin.parentNode.removeChild(plugin);

Why not remove all three plugin elements during test1? I'm just curious, that's all. This is fine by me how you have it currently.
Attachment #809217 - Flags: review?(jaws) → review+
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86 → All
(In reply to Jared Wein [:jaws] from comment #11)
> Why not remove all three plugin elements during test1? I'm just curious,
> that's all. This is fine by me how you have it currently.

I wanted to go through & check having both non-plugin and plugin object elements on the page as well.
https://hg.mozilla.org/mozilla-central/rev/688d314d5c2f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 921411
Comment on attachment 809217 [details] [diff] [review]
Front-end fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): CtP UI implementation
User impact if declined: Broken CtP doorhanger if plugin is removed and non-plugin objects are on the page (images, videos, ...)
Testing completed (on m-c, etc.): Fine on m-c the last days, no incidents.
Risk to taking this patch (and alternatives if risky): Low risk, just a minor change + test that guards against non-plugin elements.
String or IDL/UUID changes made by this patch: None.
Attachment #809217 - Flags: approval-mozilla-aurora?
Verified fixed using the testcase and the link in comment 0 on FF 27.0a1 (2013-10-03) Win 7, Mac OS X 10.8.4, Ubuntu 13.04
Status: RESOLVED → VERIFIED
Attachment #809217 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Backed out for mochitest-bc failures. Please post a branch-specific patch for uplift or get approval for any other bugs this depends on.
https://hg.mozilla.org/releases/mozilla-aurora/rev/decf58f0d596

https://tbpl.mozilla.org/php/getParsedLog.php?id=28805290&tree=Mozilla-Aurora
Flags: needinfo?(georg.fritzsche)
This depends on the addition of a helper-function to head.js from another patch, i'll just add it to the test here for the backport:
https://tbpl.mozilla.org/?tree=Try&rev=19fe8961a78f
Flags: needinfo?(georg.fritzsche)
(In reply to Paul Silaghi [QA] from comment #18)
> Verified fixed using the testcase and the link in comment 0 on FF 27.0a1
> (2013-10-03) Win 7, Mac OS X 10.8.4, Ubuntu 13.04
Verified fixed FF 26.0a2 (2013-10-14) Win 7
You need to log in before you can comment on or make changes to this bug.