905084 - "Activate this plugin" doesn't work

Status: VERIFIED FIXED

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

Description

[paul]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130812173056

Steps to reproduce:

1. Enable plugins.click_to_play
2. Set plugin to "Ask to Activate"
3. Right click on a blocked plugin and select "Activate this plugin"


Actual results:

Nothing


Expected results:

The plugin should activate

Comment 1

[John Schoenick [:johns]]

This works in 23, so it was probably broken by the doorhanger changes. Tracking due to broken UI

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix "Activate this plugin" context menu action

It looks like that context menu action just wasn't updated to the gPluginHandler interface changes.

Comment 3

[bhavana bajaj [:bajaj]]

proactively adding verify me on this bug so it is on QA's radar for verification once this lands.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix "Activate this plugin" context menu action

Added test coverage for this.

Comment 5

[Georg Fritzsche [:gfritzsche]]

https://tbpl.mozilla.org/?tree=Try&rev=2b560c2f3f24

Comment 6

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

Comment on attachment 790943 [details] [diff] [review]
Fix "Activate this plugin" context menu action

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

::: browser/base/content/test/browser_CTP_context_menu.js
@@ +10,5 @@
> +
> +// This listens for the next opened tab and checks it is of the right url.
> +// opencallback is called when the new tab is fully loaded
> +// closecallback is called when the tab is closed
> +function TabOpenListener(url, opencallback, closecallback) {

This TabOpenListener code looks unused and dead. Can this just be deleted?

@@ +96,5 @@
> +// Due to layout being async, "PluginBindAttached" may trigger later.
> +// This wraps a function to force a layout flush, thus triggering it,
> +// and schedules the function execution so they're definitely executed
> +// afterwards.
> +function runAfterPluginBindingAttached(func) {

This function should be moved to head.js since it is duplicated in multiple test files.

@@ +108,5 @@
> +    executeSoon(func);
> +  };
> +}
> +
> +// Test that the click-to-play doorhanger still works when navigating to data URLs

I don't see any mention of data URLs for this specific test.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Jared Wein [:jaws] from comment #6)
> @@ +96,5 @@
> > +// Due to layout being async, "PluginBindAttached" may trigger later.
> > +// This wraps a function to force a layout flush, thus triggering it,
> > +// and schedules the function execution so they're definitely executed
> > +// afterwards.
> > +function runAfterPluginBindingAttached(func) {
> 
> This function should be moved to head.js since it is duplicated in multiple
> test files.

We ran into this previously in bug 896965:

(from bug 896965, comment #57)
> > ::: browser/base/content/test/browser_CTP_data_urls.js
> > @@ +94,5 @@
> > >    gNextTest = nextTest;
> > >    gTestBrowser.contentWindow.location = url;
> > >  }
> > >  
> > > +function runAfterPluginBindingAttached(func, doc) {
> > 
> > can this be put in some sort of head.js?
> 
> I was thinking about that, but AFAICT this function couldn't depend on
> gTestBrowser and would always have to receive the doc or browser explicitly.
> See also e.g. prepareTest() being duplicated.
> 
> Any better suggestions or do you think i can go with this?

For the rest, two bad cases of copy'n'paste left-overs. Will fix.

Comment 8

[Georg Fritzsche [:gfritzsche]]

needinfo? for comment 7 - i don't see how i could move runAfterPluginBindingAttached() to head.js without the problem mentioned above.

Comment 9

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

One way would be to move runAfterPluginBindingAttached, prepareTest to a plugin_helpers.inc.js file and then #include the file in the various tests.

I am not sure if this would be worth the future overhead of trying to understand line numbers and pulling in the full test context when debugging.

What are your thoughts?

Comment 10

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

I'm not even sure if test files can #include something. Don't worry about this issue.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Ok. I'd personally also like to avoid more #includes, it's quite a stopper for quickly checking into test-failures.

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix "Activate this plugin" context menu action - v2

Addressed the other two review issues.

Comment 13

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

Comment on attachment 793066 [details] [diff] [review]
Fix "Activate this plugin" context menu action - v2

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

::: browser/base/content/test/browser_CTP_context_menu.js
@@ +98,5 @@
> +  waitForCondition(() => objLoadingContent.activated, test3, "Waited too long for plugin to activate");
> +}
> +
> +function test3() {
> +  clearAllPluginPermissions();

clearAllPluginPermissions() is already called in the first line of finishTest. It's also called by the anonymous function passed to registerCleanupFunction. It seems that it should only remain in the anonymous function.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Right, fixed, thanks for catching my copy'n'paste artifacts :-/

https://hg.mozilla.org/integration/mozilla-inbound/rev/20e064f3e8e0

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The newly added test has been failing off and on since it was added <https://tbpl.mozilla.org/php/getParsedLog.php?id=26792799&tree=Mozilla-Inbound>, so this was backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/53853b86f19b

Comment 16

[Georg Fritzsche [:gfritzsche]]

Weird, this is apparently failing only on Linux. test_contextmenu.html is disabled on Linux for bug 513558, but those failures don't seem related.

One other failure type i've seen on TBPL:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26794601&tree=Mozilla-Inbound

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: gContextMenu is null at chrome://browser/content/browser.xul:1
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test 2, The click-to-play notification should not be dismissed
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: PopupNotifications.panel.firstChild is null at chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js:94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Found a tab after previous test timed out: http://127.0.0.1:8888/browser/browser/base/content/test/plugin_test.html

Comment 17

[Georg Fritzsche [:gfritzsche]]

... the others being of the kind of comment 15:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | uncaught exception - TypeError: PopupNotifications.panel.firstChild is null at chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js:94
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_context_menu.js | Found a tab after previous test timed out: http://127.0.0.1:8888/browser/browser/base/content/test/plugin_test.html

Comment 18

[bhavana bajaj [:bajaj]]

Just a quick reminder that we will be entering the fourth week of betas for Fx24 starting upcoming week with Beta's going to build Monday and Thursday morning PT and this will be the last opportunity for any low risk fixes.

Given this is a Fx24 regression we should try to get it in by then so that we atleast have a week's bake time on beta.

Comment 19

[Georg Fritzsche [:gfritzsche]]

I'm looking into the Linux failures, Monday is currently looking unrealistic though unless i'd land with the Linux test disabled for now.

Comment 20

[Georg Fritzsche [:gfritzsche]]

We want to land with the test disabled on Linux for now. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=f415ebf69d00

Comment 21

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix "Activate this plugin" context menu action - v3, disabled on Linux

The only change here is disabling the test on Linux as i won't be able to look into those failures soon enough. Bug 909342 is the follow-up bug for that.

jaws, is that ok with you?

Comment 22

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d84dcbdd8b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6d84dcbdd8b

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 795465 [details] [diff] [review]
Fix "Activate this plugin" context menu action - v3, disabled on Linux

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Plugin doorhanger redesign (bug 880735 et al).
User impact if declined: "Activate this plugin" context action doesn't work for blocked & CTP plugins.
Testing completed (on m-c, etc.): On m-c, awaiting verification.
Risk to taking this patch (and alternatives if risky): Low-risk - the menu entry is already broken, this patch just fixes one line to make it work again.
String or IDL/UUID changes made by this patch: None.

Comment 25

[Paul Silaghi, QA [:pauly]]

"Activate this plugin" works fine in nightly 26.0a1 (2013-08-27) on Win 7, Ubuntu 12.04 and Mac OS X 10.7.5

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 795465 [details] [diff] [review]
Fix "Activate this plugin" context menu action - v3, disabled on Linux

Approving on branches given the nightly verification and as we want this regression to be fixed for Fx24.

Requesting QA verification on Beta once it lands there.

Comment 27

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eddf5b70bd74
https://hg.mozilla.org/releases/mozilla-beta/rev/8d8061c0f2e5

Comment 28

[Georg Fritzsche [:gfritzsche]]

(In reply to bhavana bajaj [:bajaj] from comment #26)
> Requesting QA verification on Beta once it lands there.

Paul, can you check for this when the build is out?

Comment 29

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi [QA] from comment #25)
> "Activate this plugin" works fine in nightly 26.0a1 (2013-08-27) on Win 7,
> Ubuntu 12.04 and Mac OS X 10.7.5
Verified fixed FF 24b7, 25.0a2 (2013-08-30)