916542 - Removing object nodes can trigger broken doorhanger

Status: VERIFIED FIXED

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

Description

[nightstalkerz]

Attachment: 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

Comment 1

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirm with Firefox 24 and newer. Caused by Test 16?

Comment 2

[Paul Silaghi, QA [:pauly]]

This looks similar to bug 916593.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: 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.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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

Comment 5

[Benjamin Smedberg]

Perhaps this just means that we need to sanity-check the things in windowutils.plugins before considering them for the doorhanger...

Comment 6

[Georg Fritzsche [:gfritzsche]]

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.

Comment 7

[Georg Fritzsche [:gfritzsche]]

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.

Comment 8

[John Schoenick [:johns]]

(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.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Front-end fix

Fix the front-end code by guarding against non-plugin elements in windowutils.plugins.

https://tbpl.mozilla.org/?tree=Try&rev=78b51d6076da

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Front-end fix

Sorry, missed stripping out a left-over.

Comment 11

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

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.

Comment 12

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 13

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

Ok, sounds good.

Comment 14

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/688d314d5c2f

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/688d314d5c2f

Comment 17

[Georg Fritzsche [:gfritzsche]]

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.

Comment 18

[Paul Silaghi, QA [:pauly]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87138e223c91

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Georg Fritzsche [:gfritzsche]]

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

Comment 22

[Georg Fritzsche [:gfritzsche]]

All green, pushed:
https://hg.mozilla.org/releases/mozilla-aurora/rev/0c2d59fa1189

Comment 23

[Paul Silaghi, QA [:pauly]]

(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