Closed Bug 843671 Opened 7 years ago Closed 7 years ago

cannot enter comment in plugin crash UI

Categories

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

x86_64
Windows 7
defect

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox20 --- unaffected
firefox21 + verified
firefox22 + verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: c.ascheberg, Assigned: johns)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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.
Component: General → Plug-ins
Priority: -- → P2
Product: Firefox → Core
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?
Assignee: nobody → adw
Oh, and to reproduce this in a non-release build you will need to set MOZ_CRASHREPORTER=1 in your environment.
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?
Assignee: adw → benjamin
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 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.
Attachment #717250 - Flags: review?(jschoenick) → review-
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.
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.
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.
Assignee: benjamin → jschoenick
Depends on: 851378
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.
Attachment #717250 - Attachment is obsolete: true
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
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.
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
(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 on attachment 725218 [details] [diff] [review]
Fix plugin instance owner teardown

This does seem to fix the issue for me on windows
Attachment #725218 - Flags: review?(benjamin)
Ack, move InstanceOwner->Destroy() into DoStopPlugin so it is not called early
for delayed stop...
Attachment #725218 - Attachment is obsolete: true
Attachment #725218 - Flags: review?(benjamin)
Attachment #725582 - Flags: review?(benjamin)
Attachment #725582 - Flags: review?(benjamin) → review+
Rebase on updated bug 851378
Attachment #725582 - Attachment is obsolete: true
Comment on attachment 726952 [details] [diff] [review]
Fix plugin instance owner teardown. r=bsmedberg

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1be1a068fb
Attachment #726952 - Flags: review+
Attachment #726952 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/cc1be1a068fb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
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
Attachment #726952 - Flags: approval-mozilla-aurora?
I neglected to rev the nsIObjectLoadingContent IID, followup landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9bc9047e04d
See Also: → 853694
Depends on: 853694
No longer depends on: 853694
See Also: 853694
Depends on: 854082
No longer depends on: 854082
[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.
Attachment #726952 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
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?
Keywords: qawanted
(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
Confirmed that I can enter comments in Aurora 21.0a2 2013-03-28.
Status: RESOLVED → VERIFIED
Keywords: verifyme
No longer blocks: click-to-play
You need to log in before you can comment on or make changes to this bug.