Closed Bug 1147521 Opened 5 years ago Closed 5 years ago

Cannot type into comment area of plugin crash UI

Categories

(Core :: Plug-ins, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 + verified
firefox38 + fixed
firefox39 + fixed

People

(Reporter: mconley, Assigned: smichaud)

References

Details

Attachments

(3 files, 2 obsolete files)

STR:

1) In a new profile in Firefox 37 or later, visit http://benjamin.smedbergs.us/tests/ctptests/flash-solo.html
2) If showing the click-to-play notification, activate the plugin so that you see the snake game Flash applet loaded
3) Make note of the process ID of the Firefox Plugin Content process that's running Flash
4) Open up a terminal, and type:

kill -SIGABRT [process number]

to crash Flash

5) In the crash report UI, attempt to type into the "Add a comment" text area

ER:

I should be able to add comments to my crash report

AR:

The text area does not appear to respond to my text input. I cannot type or paste any text.
[Tracking Requested - why for this release]:

For any plugin crashes where we generate a crash report, users will not be able to add additional commentary to their crashes. Not sure how important that is, but we should probably at the very least add a release note so that users are not confused when they can't type into the input.
Which exact version and OS did you try this on?

It seems to work for me in 37.0b7 on Linux, see bp-2b1efe9b-11fc-4e56-9da5-9f1d32150325
Actully ni? mconley for the question in comment #2
Flags: needinfo?(mconley)
Also is fine on 37.0b7 on a Win7 VM: bp-b22dbe41-4243-469a-9b42-fcb622150325
mconley, as I suppose you are on Mac, I wonder if this works in 37 RC build1, as this reminds me of bug 1137229.
Yes, I'm on OS X. I'll try with the patch in bug 1137229 applied.
Flags: needinfo?(mconley)
I can reproduce this, even in FF 37RC1.  And it has the same regression range as bug 1137229:

firefox-2015-02-20-03-02-02-mozilla-central
firefox-2015-02-21-03-02-08-mozilla-central

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1b4c5daa7b7a&tochange=5de3af90c494

Bug 1137229 was caused by a design flaw in nsFocusManager code -- an NS_BLUR_CONTENT wasn't sent that should have been.  I was able to work around the problem by hooking a call (to UnbindFromTree()) that *is* still made when the bug is triggered.  I won't be able to use that strategy for this bug.

In the case of this bug, the NS_BLUR_CONTENT event isn't sent because the plugin-container process (for the Flash plugin) crashes.  I'll try to think of some way to work around *that*.
Yes, smichaud is right - even with bug 1137229's patch applied, this bug still reproduces.
I'll be working on this, as my top priority, until I can think of some kind of fix/workaround.

As bad as this nightmare is, it'll get worse if we try to back out my patch for bug 1110888.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Blocks: 1110888
I've confirmed, using hg bisect, that this bug is triggered by my patch for bug 1110888.
Tracking another keyboard related issue in 37.
Actually this bug is quite different from bug 1137229:

1) Even after the bug has been triggered, other text input boxes on the page continue to work just fine:  You can click on the location bar or the search box and enter text there.

2) The bug happens even if the plugin didn't first have keyboard focus.

In fact the Element on the page that once contained the plugin (and which now contains the crash report UI) is still an HTMLObjectElement/HTMLSharedObjectElement (and still shows up as an ordinary plugin in the Inspector)!  I didn't realize (writing my patch for bug 1110888) that such an object might exist *without* containing a plugin.  So now I need to figure out how to know this in the dom/html implementations of those objects.
Of course this problem doesn't arise on platforms (like Windows and Linux) that still have per-plugin widgets -- there you can always know when a plugin has keyboard focus by knowing when its widget (a native object) has keyboard focus.  So this is more fallout from bug 1092630, which removed per-plugin widgets on the Mac.
I believe you can determine if the plugin element is in the crashed state by QI'ing it to nsIObjectLoadingContent, and reading its pluginFallbackType - if it matches nsIObjectLoadingContent::PLUGIN_CRASHED, then it's in the crashed state.

Not sure if that's helpful for this situation.
> Not sure if that's helpful for this situation.

Thanks, Mike.  That may be the key to fixing this bug!
Mike, do you know of any other cases where we need to be able to enter text inside an object that *can* contain a plugin, but doesn't currently?  For example a plugin visible on a page that's click-to-play.
Looking at the "plugin problem" binding, which is what overtakes a plugin when it's not displaying normally: https://hg.mozilla.org/mozilla-central/file/cc0950b7a369/toolkit/mozapps/plugins/content/pluginProblem.xml

I believe the only time we ever ask for text from the user is via that textarea here:

https://hg.mozilla.org/mozilla-central/file/cc0950b7a369/toolkit/mozapps/plugins/content/pluginProblem.xml#l47

The other inputs are a button and a checkbox.
Perhaps obvious, but navigation via the tab key is important in many variants of that UI, even though actually typing into the textbox only happens in this one variant.
Actually there's *lots* of stuff in nsIObjectLoadingContent.idl that I need to pay attention to, which I haven't been.

The main thing is that I don't want to call nsIWidget::SetPluginFocused(true) for something that isn't a plugin.

I don't think I have to pay attention to how the HTMLObjectElement/HTMLSharedObjectElement gets (or loses) the focus -- whether this happens as a result of mouse events, keyboard events, or some other kind of event.  In dom/html code I'll be working at a much higher level.
Can you enter text in SVG documents?  If so, this bug probably also effects them.
Attached patch Fix (obsolete) — Splinter Review
I've managed to come up with a simple fix for this bug, and for other bugs (as yet unreported) having to do with HTMLObjectElement/HTMLSharedObjectElement objects that don't contain running plugins.  As best I can tell, nsIObjectLoadingContext::GetHasRunningPlugin() tells me all I need to know.

I also need to listen to "PluginInstantiated" events to deal with the case of a focused click-to-play XBL binding that gets turned into a loaded plugin.  There doesn't seem to be any "PluginCrashed" event I can listen for ... but it turns out I don't really need it.

I need a plugins person to review this patch, so I've asked you, Georg.  Please feel free to pass the review along if you'd prefer.  And Smaug, I've also asked you to review it since you reviewed my last couple of changes to this code, and to make sure I haven't inadvertently done something terrible.

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a9c959700b9
Attachment #8583611 - Flags: review?(gfritzsche)
Attachment #8583611 - Flags: review?(bugs)
Comment on attachment 8583611 [details] [diff] [review]
Fix

> HTMLObjectElement::OnFocusBlurPlugin(Element* aElement, bool aFocus)
> {
>+  // In general we don't want to call nsIWidget::SetPluginFocused() for any
>+  // content that doesn't have a plugin running.  But if SetPluginFocused(true)
>+  // was just called for aElement while it had a plugin running, we want to
>+  // make sure nsIWidget::SetPluginFocused(false) gets called for it now, even
>+  // if aFocus is true.
>+  nsCOMPtr<nsIObjectLoadingContent> olc = do_QueryInterface(aElement);
>+  bool hasRunningPlugin = false;
>+  if (olc) {
>+    olc->GetHasRunningPlugin(&hasRunningPlugin);
>+  }
>+  if (!hasRunningPlugin) {
>+    aFocus = false;
>+  }
Only do all this when aFocus is true.


>+HTMLObjectElement::HandlePluginStateChange(Element* aElement,
>+                                           nsIDOMEvent* aDOMEvent)
>+{
>+  if (!aDOMEvent) {
>+    return;
>+  }
>+  bool trusted = false;
>+  aDOMEvent->GetIsTrusted(&trusted);
>+  if (!trusted) {
>+    return;
>+  }


if (!aDOMEvent || !aDOMEvent->InternalDOMEvent()->IsTrusted()) {
  return;
}


>+  nsAutoString type;
>+  aDOMEvent->GetType(type);
>+  if (type.Equals(NS_LITERAL_STRING("PluginInstantiated"))) {
Feels hackish to rely on events in this kind of case.
All this should happen in ObjectLoadingContent or in nsNPAPIPluginInstance or somewhere.
(and in fact OnFocusBlurPlugin(..., false) should also happen when we actually kill the plugin instance)



So what happens to the focus when a plugin crashes? I mean in case the plugin had focus when it crashed.
I hope something in nsNPAPIPluginInstance or somewhere calls SetPluginFocused(false)



(Also, I still think we should check that we're in a sane state when Cocoa code calls IsPluginFocused(),
 so that if there isn't anymore a plugin which should be in focus, we just clear the flag. But that isn't about this bug.)
Attachment #8583611 - Flags: review?(bugs) → review-
>>+  nsAutoString type;
>>+  aDOMEvent->GetType(type);
>>+  if (type.Equals(NS_LITERAL_STRING("PluginInstantiated"))) {
>
> Feels hackish to rely on events in this kind of case.  All this
> should happen in ObjectLoadingContent or in nsNPAPIPluginInstance or
> somewhere.

Fair enough.  I'll dig around in nsObjectLoadingContent and
nsNPAPIPluginInstance to see what I can find.  Maybe I can find the
equivalent of "OnPluginCrashed" there.

> (and in fact OnFocusBlurPlugin(..., false) should also happen when
> we actually kill the plugin instance)

I already tried this (a long time ago) from
nsPluginInstanceOwner::Destroy().  It doesn't work -- by then the
plugin content has long since been unfocused (sometimes, as we've
seen, without an NS_BLUR_CONTENT event having been sent).

> (Also, I still think we should check that we're in a sane state when
> Cocoa code calls IsPluginFocused(), so that if there isn't anymore a
> plugin which should be in focus, we just clear the flag. But that
> isn't about this bug.)

Remember that OS X no longer has per-plugin widgets.  So it's
difficult to know much about plugins in widget code.
>>>+  nsAutoString type;
>>>+  aDOMEvent->GetType(type);
>>+  if (type.Equals(NS_LITERAL_STRING("PluginInstantiated"))) {
>>
>> Feels hackish to rely on events in this kind of case.  All this
>> should happen in ObjectLoadingContent or in nsNPAPIPluginInstance or
>> somewhere.
>
> Fair enough.  I'll dig around in nsObjectLoadingContent and
> nsNPAPIPluginInstance to see what I can find.  Maybe I can find the
> equivalent of "OnPluginCrashed" there.

Actually I've now changed my mind about this.  Only in
HTMLObjectElement/HTMLSharedObjectElement code do we know the value of
sLastFocused -- which we need to know if we're to call
nsIWidget::SetPluginFocused() intelligently.  You don't really mean
for me to call HTMLObjectElement::OnFocusBlurPlugin() from plugin
code, do you?  (From nsObjectLoadingContent or nsNPAPIPluginInstance.)

There *is* a PluginCrashed DOM event.  Let me see if I can figure out
why we're not receiving it.
Flags: needinfo?(bugs)
> There *is* a PluginCrashed DOM event.  Let me see if I can figure
> out why we're not receiving it.

It's because mFlags.mOnlyChromeDispatch is set on it, here:

https://hg.mozilla.org/mozilla-central/annotate/37d3dcbf23a9/dom/base/nsObjectLoadingContent.cpp#l350

So I've changed my mind yet again, and now I *am* going to call
HTMLObjectElement::OnFocusBlurPlugin() from nsObjectLoadingContent
code.  Ultimately this should be a method on the
nsIObjectLoadingContent interface -- which both HTMLObjectElement and
HTMLSharedObjectElement inherit from.  But I don't want to mess with
interfaces in code that will need to land soon on the 37 branch.
Calling HTMLObjectElement::OnFocusBlurPlugin() from objectloadingcontent sounds ok.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #26)
> Calling HTMLObjectElement::OnFocusBlurPlugin() from objectloadingcontent
> sounds ok.

Yeah, triggering this from ObjectLoadingContent sounds much cleaner.
Comment on attachment 8583611 [details] [diff] [review]
Fix

Cancelling for now given the outstanding changes.
Attachment #8583611 - Flags: review?(gfritzsche)
> HTMLObjectElement::OnFocusBlurPlugin()

Actually I'm going to write HTMLObjectElement::HandlePluginCrashed(Element*) and HTMLObjectElement::HandlePluginInstantiated(Element*) and call those from nsObjectLoadingContent.cpp.
Attached patch Fix rev1 (obsolete) — Splinter Review
OK, let's try this again.
Attachment #8583611 - Attachment is obsolete: true
Attachment #8584107 - Flags: review?(gfritzsche)
Attachment #8584107 - Flags: review?(bugs)
Comment on attachment 8584107 [details] [diff] [review]
Fix rev1

>+#ifdef XP_MACOSX
>+  nsCOMPtr<Element> thisElement =
>+    do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
>+  HTMLObjectElement::HandlePluginInstantiated(thisElement);

I would just do
#ifdef XP_MACOSX
HTMLObjectElement::HandlePluginInstantiated(thisContent->AsElement())
#endif



>+#ifdef XP_MACOSX
>+  nsCOMPtr<Element> thisElement =
>+    do_QueryInterface(static_cast<nsIObjectLoadingContent*>(this));
>+  HTMLObjectElement::HandlePluginCrashed(thisElement);
>+#endif


Move the (which is currently few lines below)
nsCOMPtr<nsIContent> thisContent =
    do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));
to be above your new code and you can use thisContent->AsElement();
again.
Attachment #8584107 - Flags: review?(bugs) → review+
Thanks.  I'll make these changes.
Attached patch Fix rev2Splinter Review
This includes Smaug's changes.  Carrying forward his r+.
Attachment #8584107 - Attachment is obsolete: true
Attachment #8584107 - Flags: review?(gfritzsche)
Attachment #8584147 - Flags: review?(gfritzsche)
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

Because this is so very urgent, I've landed it on mozilla-inbound before seeing Georg's review:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9ea27ee1d96d

Georg, is you want any changes we can do them as a followup either here or in another bug.
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

Approval Request Comment
[Feature/regressing bug #]: 1092630/1110888
[User impact if declined]: It won't be possible to enter comments in the plugin crash UI
[Describe test coverage new/current, TreeHerder]: Fairly comprehensive testing by myself
[Risks and why]: Low to moderate. Simple patch, but the code it changes can be tricky.
[String/UUID change made/needed]: None
Attachment #8584147 - Flags: approval-mozilla-release?
Attachment #8584147 - Flags: approval-mozilla-beta?
Attachment #8584147 - Flags: approval-mozilla-aurora?
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

Steven has tested this with and without e10s and ctp. He thinks he's been fairly thorough at this point. Our best option is likely to build an RC2 and have QE perform exploratory testing on text input on various versions of OS X. Beta+ Aurora+

Note: I cleared the release nom as this still reflects Firefox 36 at this point.
Attachment #8584147 - Flags: approval-mozilla-release?
Attachment #8584147 - Flags: approval-mozilla-beta?
Attachment #8584147 - Flags: approval-mozilla-beta+
Attachment #8584147 - Flags: approval-mozilla-aurora?
Attachment #8584147 - Flags: approval-mozilla-aurora+
Attached patch Fix rev2, betaSplinter Review
Attachment #8584225 - Flags: review+
Attachment #8584223 - Attachment is patch: true
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

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

This looks sane to me, but i haven't dug into nsObjectLoadingContent myself much.
johns, can you maybe still take a look here? :)

::: dom/base/nsObjectLoadingContent.cpp
@@ +93,5 @@
>  
> +#ifdef XP_MACOSX
> +// HandlePluginCrashed() and HandlePluginInstantiated() needed from here to
> +// fix bug 1147521.  Should later be replaced by proper interface methods,
> +// maybe on nsIObjectLoadingContext.

Nit: Content, not Context.
Attachment #8584147 - Flags: review?(john)
Attachment #8584147 - Flags: review?(gfritzsche)
Attachment #8584147 - Flags: feedback+
The main remaining question was whether the use of GetHasRunningPlugin() here is sane.
Original issue was easily reproduced on Mac OS X (same as specified in comment 0), when using Firefox 37 RC build 1.

The issue no longer reproduces with Firefox 37 RC build 2 (BuildID=20150326190726) on:
- Mac OS X 10.7.5
- Mac OS X 10.8.5
- Mac OS X 10.9.5
- Mac OS X 10.10.3
Tested with both "Always Activate" and "Ask to Activate" for the Flash plugin, with the same results. Now the user can type without any issues and the report appears to be sent without issues. Windows and Linux also did not encounter any issues in typing a comment and sending the report (original issue never reproduced here).

Marking this as verified for Firefox 37.
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

>> +// maybe on nsIObjectLoadingContext.
>
> Nit: Content, not Context.

Oops, I'll fix this in a followup patch.
https://hg.mozilla.org/mozilla-central/rev/9ea27ee1d96d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

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

Much belated r+ :-P
Attachment #8584147 - Flags: review?(john) → review+
You need to log in before you can comment on or make changes to this bug.