Christian Ascheberg Reporter
	Description • 11 years ago

A flash plugin crashed and the plugin crash UI was shown.

The following problem occurs if plugins.click_to_play is set to true:
I am unable toggle the checkbox, click the question icon or send the crash report. Focusing the elements is possible.

I can click "Reload the page" and I can add a comment.

	Christian Ascheberg Reporter
	Comment 1 • 11 years ago

I am using the latest nightly, built from http://hg.mozilla.org/mozilla-central/rev/a73a2b5c423b, so the fix for bug 843671 is already included. I am not sure if this is actually related.

	Benjamin Smedberg
	Comment 2 • 11 years ago

* Did this work correctly before bug 843671 landed?
* Can you use these elements if plugins.click_to_play is in its default state?

I'm beginning to thoroughly hate our event handling with plugins :-(

	John Schoenick [:johns]
	Comment 3 • 11 years ago

I doubt this is related - typing still works and input is clearly getting through, but nothing is happening in response to the checkbox clicks or crash report button.

(Did you mean to track this for 21?)

	Christian Ascheberg Reporter
	Comment 4 • 11 years ago

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #2)
> Did this work correctly before bug 843671 landed?

No. I quickly tried yesterdays nightly, same problem.

> Can you use these elements if plugins.click_to_play is in its default state?

Yes.

	Benjamin Smedberg
	Comment 5 • 11 years ago

ok, untracking if this only shows up in the non-default configuration.

	John Schoenick [:johns]
	Comment 6 • 11 years ago

Does this appear with blocklist induced click-to-play or specific plugins users have made click to play via bug 549697?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 7 • 11 years ago

(comment 6)
> Does this appear with blocklist induced click-to-play or specific plugins
> users have made click to play via bug 549697?

	Paul Silaghi, QA [:pauly]
	Comment 8 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Does this appear with blocklist induced click-to-play
Yes

> or specific plugins users have made click to play via bug 549697?
What should be tested here? Bug 549697 is not fixed yet.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 9 • 11 years ago

(In reply to Paul Silaghi [QA] from comment #8)
> > or specific plugins users have made click to play via bug 549697?
> What should be tested here? Bug 549697 is not fixed yet.

Sorry, i missed that, just leave it then.

	Benjamin Smedberg
	Comment 10 • 11 years ago

This should have been fixed by bug 843671.

	Paul Silaghi, QA [:pauly]
	Comment 11 • 11 years ago

But it was already stated that this is not related to bug 843671 (comments 4,5)

	Christian Ascheberg Reporter
	Comment 12 • 11 years ago

Note that plugins.click_to_play=true meanwhile became default in the current nightly builds.

This alone does not trigger the bug for flash plugins though because they are set to "Always Activate" by default. So I set the flash plugin to "Ask to Activate" and tried to reproduce the bug.
Result:
Still the same problem as before + whenever I try to focus the plugin crashed UI, a doorhanger appears, asking to "Block Plugin" or "Continue Allowing".

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 13 • 11 years ago

Thanks for the heads-up. This should be just the event handlers not being properly set for some reason, will take a look tomorrow.

	John Schoenick [:johns]
	Comment 14 • 11 years ago

So does this mean all non-flash plugin crash reporting is broken? We probably want to track this in that case

	Benjamin Smedberg
	Comment 15 • 11 years ago

Ah, I know what's going on here. With click-to-play we're attaching the click event listener here: http://hg.mozilla.org/mozilla-central/annotate/855da6d8a327/browser/base/content/browser-plugins.js#l535

When we activate the plugin, we never remove or neuter this event listener, which we should probably do here: http://hg.mozilla.org/mozilla-central/annotate/855da6d8a327/browser/base/content/browser-plugins.js#l795

So I think we have two options:
* remove the event listener at line 795
* In _overlayClickListener.handleEvent at http://hg.mozilla.org/mozilla-central/annotate/855da6d8a327/browser/base/content/browser-plugins.js#l546 we could check the current state of the nsIObjectLoadingContent and if it's not a click-to-play type, ignore the click.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 16 • 11 years ago

Properly removing the event handlers is sensible anyway, so i did that.

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

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 17 • 11 years ago

Now hopefully with all files.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 18 • 11 years ago

Comment on attachment 819000 [details] [diff] [review]
Properly remove click-to-play event handler, add test coverage

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

r+ with questions below addressed.

::: browser/base/content/browser-plugins.js
@@ +802,5 @@
>        if (gPluginHandler.canActivatePlugin(plugin) &&
>            aPluginInfo.permissionString == pluginHost.getPermissionStringForType(plugin.actualType)) {
> +        let overlay = this.getPluginUI(plugin, "main");
> +        if (overlay) {
> +          overlay.removeEventListener("click", gPluginHandler._overlayClickListener, true); 

nit, trailing whitespace

::: browser/base/content/test/general/browser_CTP_crashreporting.js
@@ +11,5 @@
> +// Test that plugin crash submissions still work properly after
> +// click-to-play activation.
> +
> +function test() {
> +  // Crashing the plugin takes up a lot of time, so extend the test timeout.

I don't see anything here that is extending the timeout, just the general waitForExplicitFinish call. This comment can be removed.

@@ +46,5 @@
> +  gTestBrowser.contentWindow.location = gTestRoot + "plugin_big.html";
> +}
> +function onPageLoad() {
> +  let plugin = gTestBrowser.contentDocument.getElementById("test");
> +  test.clientTop;

Shouldn't this be plugin.clientTop? |test| doesn't exist. Might be good to add a comment that says it is forcing the binding to attach.

@@ +70,5 @@
> +function pluginActivated() {
> +  let plugin = gTestBrowser.contentDocument.getElementById("test");
> +  try {
> +    plugin.crash();
> +  } catch (e) {

Should add a failWithException(e) call here.

@@ +81,5 @@
> +    let elt = gPluginHandler.getPluginUI.bind(gPluginHandler, plugin);
> +    let style =
> +      gBrowser.contentWindow.getComputedStyle(elt("pleaseSubmit"));
> +    is(style.display, "block", "Submission UI visibility should be correct");
> +    

nit, trailing whitespace

@@ +86,5 @@
> +    elt("submitComment").value = "a test comment";
> +    is(elt("submitURLOptIn").checked, true, "URL opt-in should default to true");
> +    EventUtils.synthesizeMouseAtCenter(elt("submitURLOptIn"), {}, gTestBrowser.contentWindow);
> +    EventUtils.synthesizeMouseAtCenter(elt("submitButton"), {}, gTestBrowser.contentWindow);
> +    // And now wait for the submission status notification.

What do we do about timeouts here? Is the test just going to hang here if the button can't be clicked?

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 19 • 11 years ago

Hm, thanks for the catches i was a bit too hurried today :-/

(In reply to Jared Wein [:jaws] from comment #18)
> @@ +70,5 @@
> > +function pluginActivated() {
> > +  let plugin = gTestBrowser.contentDocument.getElementById("test");
> > +  try {
> > +    plugin.crash();
> > +  } catch (e) {
> 
> Should add a failWithException(e) call here.

The plugin crashes there and doesn't return from the call, an exception is expected here.

> @@ +86,5 @@
> > +    elt("submitComment").value = "a test comment";
> > +    is(elt("submitURLOptIn").checked, true, "URL opt-in should default to true");
> > +    EventUtils.synthesizeMouseAtCenter(elt("submitURLOptIn"), {}, gTestBrowser.contentWindow);
> > +    EventUtils.synthesizeMouseAtCenter(elt("submitButton"), {}, gTestBrowser.contentWindow);
> > +    // And now wait for the submission status notification.
> 
> What do we do about timeouts here? Is the test just going to hang here if
> the button can't be clicked?

Yes, i don't really know of a better approach. FWIW, our existing tests for crash report submission do the same thing, e.g.:
http://hg.mozilla.org/mozilla-central/annotate/855da6d8a327/dom/plugins/test/mochitest/test_crash_submit.xul#l97

	Jared Wein [:jaws] (please needinfo? me)
	Comment 20 • 11 years ago

(In reply to Georg Fritzsche [:gfritzsche] from comment #19)
> Hm, thanks for the catches i was a bit too hurried today :-/
> 
> (In reply to Jared Wein [:jaws] from comment #18)
> > @@ +70,5 @@
> > > +function pluginActivated() {
> > > +  let plugin = gTestBrowser.contentDocument.getElementById("test");
> > > +  try {
> > > +    plugin.crash();
> > > +  } catch (e) {
> > 
> > Should add a failWithException(e) call here.
> 
> The plugin crashes there and doesn't return from the call, an exception is
> expected here.

Ok, please add a comment saying that the exception there is expected.

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 21 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/14789271ad20

Yesterdays try push:
https://tbpl.mozilla.org/?tree=Try&rev=9db062b2b338

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 22 • 11 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign
User impact if declined: Can't send and configure plugin crash report if it crashed/hung after click-to-play activation.
Testing completed (on m-c, etc.): Should be on m-c soon, verification pending. 
Risk to taking this patch (and alternatives if risky): Low risk, this just removes an event listener when activating a plugin.
String or IDL/UUID changes made by this patch: None.

	Phil Ringnalda (:philor)
	Comment 23 • 11 years ago

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2d46b79963e8 - two things, permaorange on ASan, which I'm pretty sure is because that has crashreporter disabled and you need to ifdef the test for crashreporter, and frequent (twice across twelve pushes) timeouts on OS X 10.8, for which I have no explanation.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 24 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/14789271ad20

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 25 • 11 years ago

Example for the OS X 10.8 timouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29373166&tree=Mozilla-Inbound

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 26 • 11 years ago

... and another one:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29379406&tree=Mozilla-Inbound#error0

Both time out within a second of the "PluginCrashed" event, so this apparently just needs requestLongerTimeout().

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 27 • 11 years ago

Looking good on try now: https://tbpl.mozilla.org/?tree=Try&rev=ea7b53138447

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/48717bae9d96

	Phil Ringnalda (:philor)
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/48717bae9d96

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 29 • 11 years ago

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play redesign
User impact if declined: Can't send and configure plugin crash report if it crashed/hung after click-to-play activation.
Testing completed (on m-c, etc.): On m-c, verification pending. 
Risk to taking this patch (and alternatives if risky): Low risk, this just removes a click event listener when the user activates a plugin.
String or IDL/UUID changes made by this patch: None.

	Paul Silaghi, QA [:pauly]
	Comment 30 • 11 years ago

Verified fixed FF 27.0a1 2013-10-21 Win 7, Ubuntu 12.04 and Mac OS X 10.8.4

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 31 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/86b5bcccc989

	Georg Fritzsche [:gfritzsche] Assignee
	Comment 32 • 11 years ago

Test location fix-up: https://hg.mozilla.org/releases/mozilla-aurora/rev/3dce1991faee

I pushed the right version to try [1] but the wrong one to Aurora :(

[1] https://tbpl.mozilla.org/?tree=Try&rev=223281831139

	Paul Silaghi, QA [:pauly]
	Comment 33 • 11 years ago

Verified fixed FF 26.0a2 (2013-10-24) win 7