Closed
Bug 853694
Opened 12 years ago
Closed 11 years ago
Cannot send crash report for click-to-play plugins
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Core Graveyard
Plug-ins
Tracking
(firefox26+ verified, firefox27+ verified)
VERIFIED
FIXED
mozilla27
People
(Reporter: c.ascheberg, Assigned: gfritzsche)
References
()
Details
Attachments
(1 file, 3 obsolete files)
8.56 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 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.
See Also: → 843671
Comment 2•12 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 :-(
Comment 3•12 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?)
Reporter | ||
Comment 4•12 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.
Comment 5•12 years ago
|
||
ok, untracking if this only shows up in the non-default configuration.
tracking-firefox20:
+ → ---
Keywords: regression
Comment 6•12 years ago
|
||
Does this appear with blocklist induced click-to-play or specific plugins users have made click to play via bug 549697?
See Also: 843671 →
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → georg.fritzsche
Priority: P1 → P2
Assignee | ||
Comment 7•12 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?
Keywords: qawanted
Comment 8•12 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.
Assignee | ||
Comment 9•12 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.
Comment 10•12 years ago
|
||
This should have been fixed by bug 843671.
Comment 11•12 years ago
|
||
But it was already stated that this is not related to bug 843671 (comments 4,5)
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".
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.
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
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.
status-firefox26:
--- → affected
status-firefox27:
--- → affected
tracking-firefox26:
--- → ?
tracking-firefox27:
--- → ?
Assignee | ||
Updated•11 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
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
Attachment #818999 -
Flags: review?(jaws)
Assignee | ||
Comment 17•11 years ago
|
||
Now hopefully with all files.
Attachment #818999 -
Attachment is obsolete: true
Attachment #818999 -
Flags: review?(jaws)
Attachment #819000 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Summary: cannot send plugin crash report with plugins.click_to_play=true → Cannot send crash report for click-to-play plugins
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?
Attachment #819000 -
Flags: review?(jaws) → review+
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
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.
Assignee | ||
Comment 21•11 years ago
|
||
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.
Attachment #819000 -
Attachment is obsolete: true
Attachment #819352 -
Flags: approval-mozilla-aurora?
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.
Comment 24•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #819352 -
Flags: approval-mozilla-aurora?
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
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().
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
Comment 28•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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.
Attachment #819352 -
Attachment is obsolete: true
Attachment #819658 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Updated•11 years ago
|
Updated•11 years ago
|
Attachment #819658 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Assignee | ||
Comment 31•11 years ago
|
||
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
Comment 33•11 years ago
|
||
Verified fixed FF 26.0a2 (2013-10-24) win 7
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•