Closed Bug 853694 Opened 7 years ago Closed 6 years ago

Cannot send crash report for click-to-play plugins

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 + verified
firefox27 + verified

People

(Reporter: c.ascheberg, Assigned: gfritzsche)

References

()

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
See Also: → 843671
* 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 :-(
Blocks: 843671
Keywords: regression
Priority: -- → P1
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?)
(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.
ok, untracking if this only shows up in the non-default configuration.
Keywords: regression
Does this appear with blocklist induced click-to-play or specific plugins users have made click to play via bug 549697?
Blocks: click-to-play
No longer blocks: 843671
See Also: 843671
Assignee: nobody → georg.fritzsche
Priority: P1 → P2
(comment 6)
> Does this appear with blocklist induced click-to-play or specific plugins
> users have made click to play via bug 549697?
Keywords: qawanted
(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.
(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.
Keywords: qawanted
This should have been fixed by bug 843671.
But it was already stated that this is not related to bug 843671 (comments 4,5)
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".
Thanks for the heads-up. This should be just the event handlers not being properly set for some reason, will take a look tomorrow.
So does this mean all non-flash plugin crash reporting is broken? We probably want to track this in that case
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.
OS: Windows 7 → All
Hardware: x86_64 → All
Properly removing the event handlers is sensible anyway, so i did that.

https://tbpl.mozilla.org/?tree=Try&rev=f0bf4a120dae
Attachment #818999 - Flags: review?(jaws)
Now hopefully with all files.
Attachment #818999 - Attachment is obsolete: true
Attachment #818999 - Flags: review?(jaws)
Attachment #819000 - Flags: review?(jaws)
Summary: cannot send plugin crash report with plugins.click_to_play=true → Cannot send crash report for click-to-play plugins
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?
Attachment #819000 - Flags: review?(jaws) → review+
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
(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.
Keywords: verifyme
[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.
Attachment #819000 - Attachment is obsolete: true
Attachment #819352 - Flags: approval-mozilla-aurora?
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.
Attachment #819352 - Flags: approval-mozilla-aurora?
... 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().
https://hg.mozilla.org/mozilla-central/rev/48717bae9d96
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
[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.
Attachment #819352 - Attachment is obsolete: true
Attachment #819658 - Flags: approval-mozilla-aurora?
Attachment #819658 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 929301
Verified fixed FF 27.0a1 2013-10-21 Win 7, Ubuntu 12.04 and Mac OS X 10.8.4
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
Verified fixed FF 26.0a2 (2013-10-24) win 7
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.