1147521 - Cannot type into comment area of plugin crash UI

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

[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.

Comment 2

[Robert Kaiser]

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

Comment 3

[Robert Kaiser]

Actully ni? mconley for the question in comment #2

Comment 4

[Robert Kaiser]

Also is fine on 37.0b7 on a Win7 VM: bp-b22dbe41-4243-469a-9b42-fcb622150325

Comment 5

[Robert Kaiser]

mconley, as I suppose you are on Mac, I wonder if this works in 37 RC build1, as this reminds me of bug 1137229.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Yes, I'm on OS X. I'll try with the patch in bug 1137229 applied.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

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*.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Yes, smichaud is right - even with bug 1137229's patch applied, this bug still reproduces.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

I've confirmed, using hg bisect, that this bug is triggered by my patch for bug 1110888.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

Tracking another keyboard related issue in 37.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

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

Thanks, Mike.  That may be the key to fixing this bug!

Comment 16

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 18

[Benjamin Smedberg]

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.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Can you enter text in SVG documents?  If so, this bug probably also effects them.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

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

Comment 22

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

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.)

Comment 23

[Steven Michaud [:smichaud] (Retired)]

>>+  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.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

>>>+  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.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

> 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.

Comment 26

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

Calling HTMLObjectElement::OnFocusBlurPlugin() from objectloadingcontent sounds ok.

Comment 27

[Georg Fritzsche [:gfritzsche]]

(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 28

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8583611 [details] [diff] [review]
Fix

Cancelling for now given the outstanding changes.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> HTMLObjectElement::OnFocusBlurPlugin()

Actually I'm going to write HTMLObjectElement::HandlePluginCrashed(Element*) and HTMLObjectElement::HandlePluginInstantiated(Element*) and call those from nsObjectLoadingContent.cpp.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1

OK, let's try this again.

Comment 31

[Steven Michaud [:smichaud] (Retired)]

I've started another set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6bde7660bed

Comment 32

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

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.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Thanks.  I'll make these changes.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2

This includes Smaug's changes.  Carrying forward his r+.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

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 36

[Steven Michaud [:smichaud] (Retired)]

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

Comment 37

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2, aurora

Comment 39

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2, beta

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e737ae27cfd2

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/e737ae27cfd2

Comment 43

[Georg Fritzsche [:gfritzsche]]

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.

Comment 44

[Georg Fritzsche [:gfritzsche]]

The main remaining question was whether the use of GetHasRunningPlugin() here is sane.

Comment 45

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

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 46

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ea27ee1d96d

Comment 48

[John Schoenick [:johns]]

Comment on attachment 8584147 [details] [diff] [review]
Fix rev2

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

Much belated r+ :-P