1070053 - Notification to allow plugins on the next page

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: Screenshot 2014-09-19 13.56.44.png

When I visit gmail I get a notification to allow Google Task. When I visit the next page I again get a notification to enable it. This regressed this week in the nightlies from what I can tell.

:aklotz do you have any guess as to what regressed this?

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

I don't. Perhaps johns knows?

Comment 2

[Benjamin Smedberg]

Did you interact with the notification? Once you've clicked a button or closed the notification, it shouldn't pop up again.

Comment 3

[Benoit Girard (:BenWa)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Did you interact with the notification? Once you've clicked a button or
> closed the notification, it shouldn't pop up again.

No, I often don't touch it because it will trigger my discrete GPU if I active it. In the rare event where I want to make a phone call I active the plugin (triggering my discrete GPU).

Comment 4

[Benoit Girard (:BenWa)]

I was able to reproduce the problem with a clean profile on OSX.

Comment 5

[Benjamin Smedberg]

This behavior is intentional. We will keep showing the notification bar on pages with the hidden plugin until you interact with it: either closing the notification with the X or using the buttons to activate the plugin.

Once you've closed it with the X, it shouldn't come back for that domain.

Can you verify that this is the behavior you're seeing?

Comment 6

[Benoit Girard (:BenWa)]

Attachment: bug-allow-plugin.gif

I think you’re misunderstanding my bug. I go to gmail.com, see the pop-up which I don’t care about so I ignore it. I then navigate to another other site that doesn’t use the plugin which closes the notification implicity. On visiting the new random site (any site) I get a notification asking me to allow the plugin on that new site, which doesn’t use the plugin.

I've attach a GIF to demonstrate.

Comment 7

[John Schoenick [:johns]]

Given the timing, this is likely a bug 899347 regression -- but I haven't touched the browser side of the CTP code enough to say

Comment 8

[Mike Conley (:mconley) (:⚙️)]

I'll look into it.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

I'm able to reproduce this pretty reliably by browsing back and forth from GMail to Reddit in a non-e10s window, and I'm 99% certain I caused this in bug 899347.

While kinda confusing / erroneous, I'm reluctant to back out bug 899347 for a few reasons:

1) The Plugin Finder Service removal depended on bug 899347, and so this would end up being a multi-layered back-out
2) While I'm not a plugin security expert, I can't rightly see how this could put our users at risk. I'm certainly open to argument there though.
3) This doesn't appear to break general plugin / browser functionality, at least at this time.

I'll see if I can come up with a solution this weekend - but if there is sufficient pushback, I can put the wheels in motion and back 899347 out.

bsmedberg - are there any security risks for our users here that I'm not considering that might warrant a backout of bug 899347?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Don't update click-to-play notifications for PluginRemoved event if document is being unloaded. r=?

Comment 11

[Mike Conley (:mconley) (:⚙️)]

I've confirmed that bug 899347 was the cause, and I think I've found the problem.

Before bug 899347 landed, when the PluginRemoved event was fired (on document unload), we used to fail out by trying to access a dead object (the document). That'd cause an exception in the console, and we'd avoid updating the notification.

With this patch, we check to see if we're destroying the document before attempting to show the hidden plugin notification.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8492674 [details] [diff] [review]
Don't update click-to-play notifications for PluginRemoved event if document is being unloaded. r=?

I can't seem to push to try right now - it's hanging after "searching for changes"... I'll post a link to a try push in a few hours if this doesn't end up working straight away.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8805e329a789

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #9)
> bsmedberg - are there any security risks for our users here that I'm not
> considering that might warrant a backout of bug 899347?

I don't think there are and given this is only on Nightly this shouldn't be a major issue. Some fallout from bug 899347 was expected.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8492674 [details] [diff] [review]
Don't update click-to-play notifications for PluginRemoved event if document is being unloaded. r=?

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

Is there some earlier stage where we could unregister from the event here instead?
It seems like it would be great to avoid explicitly checking the document state if possible.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Avoid spurious hidden-plugin notifications when changing locations by doing a Principal comparison. r=?

When the PluginRemoved event is fired when changing locations, it's fired
asynchronously such that the document that the plugin belongs to has already
been unloaded. This was causing the hidden plugin notification to appear in
some cases when users browsed away from documents that had hidden plugins
in them. Now we pass the Principal for the unloading document back to the
parent and do a comparison with the current browser Principal to ensure
that they match before showing the hidden plugin notification.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8493168 [details] [diff] [review]
Avoid spurious hidden-plugin notifications when changing locations by doing a Principal comparison. r=?

How about this?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Avoid spurious hidden-plugin notifications when changing locations by doing a Principal comparison. r=?

When the PluginRemoved event is fired when changing locations, it's fired
asynchronously such that the document that the plugin belongs to has already
been unloaded. This was causing the hidden plugin notification to appear in
some cases when users browsed away from documents that had hidden plugins
in them. Now we pass the Principal for the unloading document back to the
parent and do a comparison with the current browser Principal to ensure
that they match before showing the hidden plugin notification.

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8493199 [details] [diff] [review]
Avoid spurious hidden-plugin notifications when changing locations by doing a Principal comparison. r=?

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

Looks good & per IRC test-coverage for this is coming up later.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8493251 [details] [diff] [review]
Regression test

I hope it's cool I went all Promise-y with this test. :)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8493439 [details] [diff] [review]
Regression test

Whoops - a silly typo in that patch caused my try-push to orange.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=c0fe509e59c4

Comment 25

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8493439 [details] [diff] [review]
Regression test

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

Yay for task-ification :) The plugin tests simply predate promises/tasks usage.

Given the underlying bug fixed here, can we add another test here that
* navigates to a page with "application/x-test", then
* removes the plugin and
* immediately navigates to a page with "application/x-second-test" on a different host/principal (e.g. via a data: or file: url) and
* checks that the correct plugin and host/principal are used with the notification there?

::: browser/base/content/test/plugins/browser.ini
@@ +48,5 @@
>  [browser_bug797677.js]
>  [browser_bug812562.js]
>  [browser_bug818118.js]
>  [browser_bug820497.js]
> +[browser_bug1070053.js]

I think we want to generally have more descriptive test-names, maybe something like |browser_CTP_remove_navigate.js|?.

::: browser/base/content/test/plugins/browser_bug1070053.js
@@ +25,5 @@
> +  yield loadPage(browser, gHttpTestRoot + "plugin_small.html")
> +  yield forcePluginBindingAttached(browser);
> +  yield notificationPromise;
> +
> +  // Activate the plugin...

From the original report i don't think we need to activate the plugin to trigger the bug, do we? I think this may actually cover the bug here.

::: browser/base/content/test/plugins/head.js
@@ +144,5 @@
> + *        The browser to check for the notification bar.
> + *
> + * @return Promise
> + */
> +function promiseNotificationBar(notificationID, browser) {

Can't we just change waitForNotificationBar() to support both a callback and return a promise instead?

@@ +206,5 @@
> + */
> +function pluginActivated(plugin) {
> +  let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +  return new Promise((resolve, reject) => {
> +    waitForCondition(() => objLoadingContent.activated, resolve,

Wouldn't this hang on timeouts?
Can we also just make waitForCondition() support both callback and promise style and just |return waitForCondition(...)|?

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8493929 [details] [diff] [review]
Regression test

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> Comment on attachment 8493439 [details] [diff] [review]
> Regression test
> 
> Review of attachment 8493439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yay for task-ification :) The plugin tests simply predate promises/tasks
> usage.

I figured. :) I might get a chance to update them when we start trying to get these tests to work for e10s.

> 
> Given the underlying bug fixed here, can we add another test here that
> * navigates to a page with "application/x-test", then
> * removes the plugin and
> * immediately navigates to a page with "application/x-second-test" on a
> different host/principal (e.g. via a data: or file: url) and
> * checks that the correct plugin and host/principal are used with the
> notification there?
> 

Done.

> ::: browser/base/content/test/plugins/browser.ini
> @@ +48,5 @@
> >  [browser_bug797677.js]
> >  [browser_bug812562.js]
> >  [browser_bug818118.js]
> >  [browser_bug820497.js]
> > +[browser_bug1070053.js]
> 
> I think we want to generally have more descriptive test-names, maybe
> something like |browser_CTP_remove_navigate.js|?.
> 

Done.

> ::: browser/base/content/test/plugins/browser_bug1070053.js
> @@ +25,5 @@
> > +  yield loadPage(browser, gHttpTestRoot + "plugin_small.html")
> > +  yield forcePluginBindingAttached(browser);
> > +  yield notificationPromise;
> > +
> > +  // Activate the plugin...
> 
> From the original report i don't think we need to activate the plugin to
> trigger the bug, do we? I think this may actually cover the bug here.
> 

Ok, removed activations. You're right that it is not necessary to reproduce the bug, and it helped shrink my tests some. Nice!

> ::: browser/base/content/test/plugins/head.js
> @@ +144,5 @@
> > + *        The browser to check for the notification bar.
> > + *
> > + * @return Promise
> > + */
> > +function promiseNotificationBar(notificationID, browser) {
> 
> Can't we just change waitForNotificationBar() to support both a callback and
> return a promise instead?
> 

Done - though I'm antsy about having a thing behave in two completely different ways like this...

> @@ +206,5 @@
> > + */
> > +function pluginActivated(plugin) {
> > +  let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> > +  return new Promise((resolve, reject) => {
> > +    waitForCondition(() => objLoadingContent.activated, resolve,
> 
> Wouldn't this hang on timeouts?
> Can we also just make waitForCondition() support both callback and promise
> style and just |return waitForCondition(...)|?

I'm not using pluginActivated anymore, and while I still rely on waitForCondition here and there... I think I want to hold off on making it support both Promises and callbacks. Again, I'm antsy about making a function like this return both a Promise *and* take a callback. Plus, I don't think this will hang on timeouts because waitForCondition, once it times out, will resolve anyhow, since it "moves on".

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Try push with new test: https://tbpl.mozilla.org/?tree=Try&rev=c216dad781cb

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Attachment: Regression test

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8494041 [details] [diff] [review]
Regression test

Some clean-up that I'd forgotten.

The try push has some scary perma-orange in there, but that's unrelated - it's from something that landed on fx-team that I rebased on top of that has since been backed out.

Comment 31

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #27)
> > ::: browser/base/content/test/plugins/head.js
> > @@ +144,5 @@
> > > + *        The browser to check for the notification bar.
> > > + *
> > > + * @return Promise
> > > + */
> > > +function promiseNotificationBar(notificationID, browser) {
> > 
> > Can't we just change waitForNotificationBar() to support both a callback and
> > return a promise instead?
> > 
> 
> Done - though I'm antsy about having a thing behave in two completely
> different ways like this...
> 
> > @@ +206,5 @@
> > > + */
> > > +function pluginActivated(plugin) {
> > > +  let objLoadingContent = plugin.QueryInterface(Ci.nsIObjectLoadingContent);
> > > +  return new Promise((resolve, reject) => {
> > > +    waitForCondition(() => objLoadingContent.activated, resolve,
> > 
> > Wouldn't this hang on timeouts?
> > Can we also just make waitForCondition() support both callback and promise
> > style and just |return waitForCondition(...)|?
> 
> I'm not using pluginActivated anymore, and while I still rely on
> waitForCondition here and there... I think I want to hold off on making it
> support both Promises and callbacks. Again, I'm antsy about making a
> function like this return both a Promise *and* take a callback. Plus, I
> don't think this will hang on timeouts because waitForCondition, once it
> times out, will resolve anyhow, since it "moves on".

Hm, ok. I'm not totally fixed on making helpers support both callbacks & promises, but i wonder if we can come up with a common scheme here in tests and/or browser code.
As the helpers are now, naming etc. is really inconsistent. Nothing to hold to patch with neccessarily though :)

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8494041 [details] [diff] [review]
Regression test

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

::: browser/base/content/test/plugins/browser_CTP_remove_navigate.js
@@ +56,5 @@
> +  let newTab = gBrowser.addTab();
> +  gBrowser.selectedTab = newTab;
> +  let browser = gBrowser.selectedBrowser;
> +
> +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY);

setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test Plug-in");

@@ +74,5 @@
> +  let notification = yield waitForNotificationBar("plugin-hidden", browser);
> +  ok(notification, "There should be a notification shown for the new page.");
> +  ok(notification.label.contains(SECOND_PLUGIN_NAME), "Should mention the second plugin");
> +  ok(!notification.label.contains("127.0.0.1"), "Should not refer to old principal");
> +  ok(notification.label.contains("null"), "Should refer to the new principal");

You should be able to just check notification.options.* instead of relying on label strings.

@@ +76,5 @@
> +  ok(notification.label.contains(SECOND_PLUGIN_NAME), "Should mention the second plugin");
> +  ok(!notification.label.contains("127.0.0.1"), "Should not refer to old principal");
> +  ok(notification.label.contains("null"), "Should refer to the new principal");
> +  // Ensure that the notification is showing information about
> +  // the x-second-test plugin.

This comment should be moved up a little?

Comment 33

[Mike Conley (:mconley) (:⚙️)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #32)
> Comment on attachment 8494041 [details] [diff] [review]
> Regression test
> 
> Review of attachment 8494041 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/test/plugins/browser_CTP_remove_navigate.js
> @@ +56,5 @@
> > +  let newTab = gBrowser.addTab();
> > +  gBrowser.selectedTab = newTab;
> > +  let browser = gBrowser.selectedBrowser;
> > +
> > +  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY);
> 
> setTestPluginEnabledState(Ci.nsIPluginTag.STATE_CLICKTOPLAY, "Second Test
> Plug-in");
> 

Ah, yep, thanks.

> @@ +74,5 @@
> > +  let notification = yield waitForNotificationBar("plugin-hidden", browser);
> > +  ok(notification, "There should be a notification shown for the new page.");
> > +  ok(notification.label.contains(SECOND_PLUGIN_NAME), "Should mention the second plugin");
> > +  ok(!notification.label.contains("127.0.0.1"), "Should not refer to old principal");
> > +  ok(notification.label.contains("null"), "Should refer to the new principal");
> 
> You should be able to just check notification.options.* instead of relying
> on label strings.

notification.options doesn't exist for notification boxes (they do for PopupNotification notifications... soooo many notifications). Notification box notifications only get the label, a priority, and some button functions.

> 
> @@ +76,5 @@
> > +  ok(notification.label.contains(SECOND_PLUGIN_NAME), "Should mention the second plugin");
> > +  ok(!notification.label.contains("127.0.0.1"), "Should not refer to old principal");
> > +  ok(notification.label.contains("null"), "Should refer to the new principal");
> > +  // Ensure that the notification is showing information about
> > +  // the x-second-test plugin.
> 
> This comment should be moved up a little?

Yep, good idea.

Thanks for the review!

Comment 34

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #33)
> > @@ +74,5 @@
> > > +  let notification = yield waitForNotificationBar("plugin-hidden", browser);
> > > +  ok(notification, "There should be a notification shown for the new page.");
> > > +  ok(notification.label.contains(SECOND_PLUGIN_NAME), "Should mention the second plugin");
> > > +  ok(!notification.label.contains("127.0.0.1"), "Should not refer to old principal");
> > > +  ok(notification.label.contains("null"), "Should refer to the new principal");
> > 
> > You should be able to just check notification.options.* instead of relying
> > on label strings.
> 
> notification.options doesn't exist for notification boxes (they do for
> PopupNotification notifications... soooo many notifications). Notification
> box notifications only get the label, a priority, and some button functions.

Ah, right, i was thinking of those, sorry.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/7d7a63f2c39a

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7d7a63f2c39a

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

Bugnotes:

http://people.mozilla.org/~mconley2/bugnotes/bug-1070053.html