843671 - cannot enter comment in plugin crash UI

Status: VERIFIED FIXED

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

Description

[Christian Ascheberg]

A flash plugin crashed and the plugin crash UI was shown. It provides an option to enter a comment and to send the URL.

Problem:
I am unable to enter text into the textarea or even focus it. Unchecking the URL checkbox is not possible either.

It is possible to click the question mark icon, send the crash report or reload the page using the given link. I can reproduce this every time I manually kill the flash plugin on Youtube. The problem occurs in safe-mode as well.

Comment 1

[Benjamin Smedberg]

I can reproduce this very easily. I think it's important to fix this or we have unusable UI.

STR (Windows):

* Go to a page with a large Flash applet (http://benjamin.smedbergs.us/tests/test-flash-blackbackground.html)
* In the task manager, kill FlashPluginPlugin_*.exe

This seems like it might be a focus issue with the XBL; when I try to tab through the elements, the focus gets stuck on the close button. smaug, do you have any clues?

Comment 2

[Benjamin Smedberg]

Oh, and to reproduce this in a non-release build you will need to set MOZ_CRASHREPORTER=1 in your environment.

Comment 3

[Benjamin Smedberg]

Hey, I figured something out ;-)

The key presses are being thrown out because of a check at this location:

http://hg.mozilla.org/mozilla-central/annotate/67f2a2816651/content/events/src/nsEventStateManager.cpp#l3510

if (nsEventStatus_eConsumeNoDefault != *aStatus)

And the event has mDefaultPrevented being set on it because of this stack:

>	xul.dll!nsDOMEvent::PreventDefault()  Line 431	C++
 	xul.dll!nsDOMKeyboardEvent::PreventDefault()  Line 25	C++
 	xul.dll!nsPluginInstanceOwner::DispatchKeyToPlugin(aKeyEvent=0x0569a3f0)  Line 1834	C++
 	xul.dll!nsPluginInstanceOwner::HandleEvent(aEvent=0x0569a3f0)  Line 1942	C++

johns/smaug: I don't know a lot about event dispatch, but since this plugin is no longer "active" can we call nsObjectLoadingContent::DoStopPlugin -> nsPluginInstanceOwner::Destroy as soon as the instance crashes?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I don't understand the question since I have no idea what nsObjectLoadingContent::DoStopPlugin -> nsPluginInstanceOwner::Destroy do.
But sounds like plugin stuff shouldn't call preventDefault() in this case.

Comment 5

[Benjamin Smedberg]

Attachment: Call StopPluginInstance from nsPluginCrashedEvent, and deal with ordering issues that result from that, rev. 1

Comment 6

[Benjamin Smedberg]

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

Comment 7

[John Schoenick [:johns]]

Comment on attachment 717250 [details] [diff] [review]
Call StopPluginInstance from nsPluginCrashedEvent, and deal with ordering issues that result from that, rev. 1

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

I don't think this is correct. PluginCrashed is called by pluginhost here:

http://dxr.mozilla.org/mozilla-central/dom/plugins/base/nsPluginHost.cpp#l3680

Which then destroys the instance. This is why OBJLC simply drops its reference to the plugin. As the plugin is already presumably stopped, Destroy() shouldn't be a reentrance risk, so we might be able to just re-order the calls in pluginhost.

Comment 8

[Rob]

Attachment: Firefox Aurora 21.0a2 Adobe Crash Reporter Comment Box

Added Image of the "Adobe Flash Crash Reporter Dialog".


On Firefox Aurora 21.0a2 the "Adobe Crash Reporter's" "Comment Box" is non-clickable (as is the "Privacy Issue" of the "Include the page's URL").

The (?) is clickable and takes you to this URL:
http://support.mozilla.org/en-US/kb/send-plugin-crash-reports-help-improve-firefox?redirectlocale=en-US&as=u&redirectslug=Plugin+crash+reports&utm_source=inproduct

and fortunately the [Send crash report] Button works (as does the "Reload the page" Link).

Thus, the 'Plugin Reporter Area' is not completely 'dead' but has a bit of functionality.

Comment 9

[Rob]

I would add "privacy" to the "Keywords" area (see prior comment 8), but for some reason I do not have permission (and I've been here more than a few years); otherwise I would make the change myself. Thanks whomever.

I also can not change "Whiteboard", "QA Contact", "Depends On", "Blocks", "See also" or "Crash Signature", etc. -- a few weeks ago I did have permission to change all those things. Thanks (again) if that is also fixed.

Comment 10

[Benjamin Smedberg]

johns, can you take this? I can't figure out a way to get the sequencing right without this ordering, but I'm not 100% sure about what was objectionable about this sequencing.

Comment 11

[John Schoenick [:johns]]

Attachment: Fix plugin instance owner teardown

Calling StopPluginInstance at all is wrong in this case, the plugin is already
dead. It calls into PluginHost to stop the mid-destruction plugin. Doing it in
an event further complicates ObjectLoadingContent's crazy state changing and
re-entrance mess. Ideally, we just want to throw out mInstanceOwner - but we
must kill its frame first, which causes XBL to touch the dead plugin's
prototype. Previously, for crashes, were just letting the ObjectFrame
incidentally remove itself from the instanceowner and never properly calling
Destroy(). This also breaks my patches in bug 784131.

This applies on top of the patch for bug 851378, which will let us throw out the
prototype earlier. I haven't had a chance to test this yet (linux doesn't have
this UI) so there might be more issues.

See also bug 767635 for cleaning up the start/stop plugin code further.

Comment 12

[John Schoenick [:johns]]

Comment on attachment 725218 [details] [diff] [review]
Fix plugin instance owner teardown

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

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2553,5 @@
>  
> +  // This can/will re-enter
> +  DoStopPlugin(ownerGrip, delayedStop);
> +
> +  ownerGrip->Destroy();

Arrgh, this will break delayed stop. Which is probably already broken. We should probably just tear it out until(if) bug 818785 lands

Comment 13

[Benjamin Smedberg]

johns, is this something which we can land on aurora? It seems that bug 851378 is pretty risky, and not something that we'd normally think of taking on Aurora, and I really don't want to have to back out the comment/URL UI, I'm looking for something that solves the issue with minimum risk.

Comment 14

[John Schoenick [:johns]]

Looks good on try now, despite orange factor
https://tbpl.mozilla.org/?tree=Try&rev=dea28098a353

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #13)
> johns, is this something which we can land on aurora? It seems that bug
> 851378 is pretty risky, and not something that we'd normally think of taking
> on Aurora, and I really don't want to have to back out the comment/URL UI,
> I'm looking for something that solves the issue with minimum risk.

I'm not sure if 851378 is too risky for aurora - it's mainly just moving a block of code between files and changing ordering - the latter being the far riskier part, but messing with ordering is what this bug is all about unfortunately :-/

The comment 5 approach leaves us in |mType == Plugin| state post-crash, and leaves us open to events on the loop doing bad things -- including touching the dead plugin prototype or trying to respawn the plugin - we'd have to add some kind of mPluginCrashing flag and riddle checks around the class, which is probably easier to get wrong than 851378

Comment 15

[John Schoenick [:johns]]

(In reply to John Schoenick [:johns] from comment #12)
> Arrgh, this will break delayed stop. Which is probably already broken. We
> should probably just tear it out until(if) bug 818785 lands

Nevermind, StopPluginRunnable keeps this reference itself, it doesn't look back at the class for it.

Comment 16

[John Schoenick [:johns]]

Comment on attachment 725218 [details] [diff] [review]
Fix plugin instance owner teardown

This does seem to fix the issue for me on windows

Comment 17

[John Schoenick [:johns]]

Attachment: Fix plugin instance owner teardown

Ack, move InstanceOwner->Destroy() into DoStopPlugin so it is not called early
for delayed stop...

Comment 18

[John Schoenick [:johns]]

Attachment: Fix plugin instance owner teardown. r=bsmedberg

Rebase on updated bug 851378

Comment 19

[John Schoenick [:johns]]

Comment on attachment 726952 [details] [diff] [review]
Fix plugin instance owner teardown. r=bsmedberg

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

Comment 20

[Ed Morley [:emorley]]

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

Comment 21

[John Schoenick [:johns]]

Comment on attachment 726952 [details] [diff] [review]
Fix plugin instance owner teardown. r=bsmedberg

[Approval Request Comment]
Note: This requires bug 851378 also be uplifted, which is where most of the significant changes are.


Bug caused by (feature/regressing bug #):
N/A, bug was not relevant until advent of plugin crashed UI

User impact if declined:
Cannot interact with comment field in plugin crashed UI, which would need to be disabled on aurora w/o this patch.

Testing completed (on m-c, etc.):
On m-c

Risk to taking this patch (and alternatives if risky): 
Very low, bug 851378 has most of the significant changes.

String or UUID changes made by this patch: 
None

Comment 22

[John Schoenick [:johns]]

I neglected to rev the nsIObjectLoadingContent IID, followup landed:

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

Comment 23

[Ed Morley [:emorley]]

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

Comment 24

[bhavana bajaj [:bajaj]]

[I was waiting to get some stability on nightly before uplifting on aurora]

As discussed in crash-kill meeting today, there were no new plugin related stability issues on nightly coinciding the timing of landing of the patch here & 851378 .Approving the uplift for aurora & requesting QA verification here.

Comment 25

[John Schoenick [:johns]]

I rebased these for aurora, but 21 lacks the webidl conversion for object tags, so I'm going to wait for a try run before I burn the tree:

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

Comment 26

[u279076]

I can confirm that the ability to type a comment in the plugin crash reporter is now fixed. However, I'm not seeing the comment make it through to my crash report:
https://crash-stats.mozilla.com/report/index/7488846e-85cb-4124-af45-62b9a2130325

For this report I entered a comment of "here's a test comment" but the User Comment field appears to be empty. Should I file a new bug on that or is that something this bug should address?

Comment 27

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9aca7fdb287

Comment 28

[John Schoenick [:johns]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #26)
> I can confirm that the ability to type a comment in the plugin crash
> reporter is now fixed. However, I'm not seeing the comment make it through
> to my crash report:
> https://crash-stats.mozilla.com/report/index/7488846e-85cb-4124-af45-
> 62b9a2130325
> 
> For this report I entered a comment of "here's a test comment" but the User
> Comment field appears to be empty. Should I file a new bug on that or is
> that something this bug should address?

This would be a new bug

Comment 29

[u279076]

Confirmed that I can enter comments in Aurora 21.0a2 2013-03-28.