Closed Bug 1153145 Opened 5 years ago Closed 5 years ago

Keystrokes Flash Player do not get handled in pop ups

Categories

(Core :: Plug-ins, defect)

37 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- verified
firefox40 --- verified
firefox41 --- verified

People

(Reporter: florian, Assigned: smichaud)

References

()

Details

(Keywords: regression, Whiteboard: [STR in comment #8])

Attachments

(4 files, 6 obsolete files)

10.10 KB, patch
Details | Diff | Splinter Review
12.10 KB, patch
Details | Diff | Splinter Review
15.59 KB, patch
Details | Diff | Splinter Review
1.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

OS: MacOs 10.10.3
Flash Player Version: 17.0.0.134
Firefox Version: 37.0.1

Open following URL:
http://balsamiq.com
Below the big blue buttons is a link called "launch demo version".
You will get the Balsamiq editor running in the Flash Player
Double click on the "Welcome" thumbnail on the left.
You will get a pop up.
Try changing the name


Actual results:

The keystrokes do not get handled.

Other Balsamiq products are also affected by this. Some of them did not receive a change to the Flash Player part for a long time.


Expected results:

The keystrokes should get handled ;-)

Observations:
Opening the developer tools cures the issue.
Safari with the same Flash Player version works fine.
Chrome no issue with the same version.
Another observation:
Swapping to another application and back to the editor loaded in Firefox heals the issue as well.

Did you change anything how the focus gets set?
Here is the direct link to the editor: http://webdemo.balsamiq.com/
Component: Untriaged → Plug-ins
Product: Firefox → Core
smichaud, if this is a regression from the recent Mac plugin changes, is this something that you could mentor spohl to help with?
Flags: needinfo?(smichaud)
> smichaud, if this is a regression from the recent Mac plugin changes

As best I can tell this bug (if it is a bug) also happens in FF 36.  So it's not a consequence of the patches for bug 1092630 or bug 1110888 (which are only on the 37 branch and up).  I'll look at this more closely next week, to see if it's something that would make sense to work on with spohl.

> Try changing the name

What does this mean?  Which "name"?

> Opening the developer tools cures the issue.

What do you mean by "opening"?  Which developer tool or tools?
Flags: needinfo?(smichaud)
Florian, please also give specific, detailed steps to reproduce this bug at http://webdemo.balsamiq.com/.
Florian, please let us know whether or not this bug happens with Firefox 36.0.4.
Flags: needinfo?(florian)
Hi Steven and Benjamin,

Thanks for looking into the issue so quickly!

I tested 34.0.5 (which I had until it updated ;-) ), 36.0 which I got from the archive and 37.0.1

I made a video showing the issue in 37.0.1. In 36.0 and 34.0.5 the focus is set correctly.

Here is the link to the video:
https://www.dropbox.com/s/vfmfo8sieoj8mnj/firefox_focus_issue.mov?dl=0

Can you let me know where I can get the version 36.0.4?

With "try changing the name" I meant entering a new value for the mockup name "Welcome".

Alternatively you can click into the search box in the top right corner (called quick add). It has the same issue.

Let me know if you need additional information.
Flags: needinfo?(florian)
Thanks for your info, Florian.  I find I can reproduce this bug in Firefox 37.0.1 but not 36.0.4.

Here's improved STR from your video:

1) Visit http://webdemo.balsamiq.com/ and wait for the page to finish loading.
2) Click in the Quick Add box in the upper right and try to type something.  Notice that no characters appear.
3) Double-click on the blue Welcome box under Mockups to the left.  A "Rename Mockup" tab-modal dialog will appear.  Notice that you can use the arrow keys to move the text-entry cursor.  But when you type "ordinary" characters, nothing appears.

Later I'll find a more precise regression range -- probably tomorrow.

I suspect you're right that this is a focus issue -- that we're not sending Flash the correct focus/blur notifications.  The fact that the arrow keys still work shows that we *are* sending keystrokes (and presumably that bug 1110888 isn't involved here).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [STR in comment #8]
Here's this bug's regression range in mozilla-central nightlies:

firefox-2015-03-27-03-02-12-mozilla-central
firefox-2015-03-28-03-02-09-mozilla-central

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e046475a75cb&tochange=ad587ca628cf

I suspect my patch for bug 1147521 was the trigger here, though I don't think it can have been the cause -- I suspect the focus problem already existed, and that my patch was just the trigger.  I'll dig further tomorrow.
By the way, Florian, all Mozilla distros can be found here (including releases and all the different kinds of nightlies, going back many years):

http://ftp.mozilla.org/pub/mozilla.org/

All Firefox distros are here:

http://ftp.mozilla.org/pub/mozilla.org/firefox/
See Also: → 1158839
I'm going to be working on this bug (and related bugs) with Stephen Pohl.  As I preliminary to that, I'm going to post some debug logging patches, as an illustration of some of my debugging techniques.

I haven't really started looking into this bug.  But I have some reason to think it has to do with us not sending NPAPI focus events (NPCocoaEventFocusChanged and NPCocoaEventWindowFocusChanged) to plugins when we should.  So my patches will help track when these events get sent to Flash, in Firefox and other browsers.

My first patch is an interpose library that hooks the Flash plugin's own NPP_HandleEvent() function and logs the focus events it receives and logs a stack trace.  It can in principle work in any browser -- and it does work without complications in Firefox and Opera.  But the technique it uses means that it needs to be tailored to a particular version of Flash (17.0.0.169), and might break with a new version.

My second patch tracks (in a very general way) how NPAPI focus events are currently handled in Firefox.
This patch does a binary patch of running Flash code.  So, though I've tested it with the current Flash version (17.0.0.169) and it works fine, it might need to be tweaked when the next Flash version comes out.

It works "out of the box" in Firefox and Opera.  With some effort it can also be made to work in Safari -- at least on OS X 10.8.5 and 10.7.5.

Recent versions of Safari have "entitlements" embedded in their signatures.  The presence of entitlements in any Mac app prevents the DYLD_INSERT_LIBRARIES environment variable from working with it (which we need to load an interpose library).  These entitlements can be removed from Safari without breaking any essential functionality.  So you can make a copy of Safari from which they have been removed, and test with that.

For this you need to re-sign Safari with your own signing certificate, and make it replace all existing signatures:

sudo codesign -s "Steven Michaud" -f /Applications/SafariAlt.app

For this you need a code signing cert from Apple.

To get one you need an account (i.e. to create an Apple ID), as per:

https://developer.apple.com/register/index.action

Once you have this account, sign in, choose "Certificates, Identities and Profiles", choose Certificates under Mac Apps, then click on the "+" (Plus) button.  You'll be walked through using the Keychain Access utility to create a certificate request, plus all the other necessary steps.

On OS X 10.9 and up there's a different problem running Safari with interpose libraries, which I haven't yet figured out how to get around.  But with luck I'll have a solution within the next few days.

(On OS X 10.9.5 and up, Safari's non-main processes (equivalent to Firefox's plugin and content processes) are XPC services.  These set their own DYLD_INSERT_LIBRARY variables (in their Info.plist files), and at least some of them are sandboxed (which prevents us using the CoreSymbolication framework in an interpose library).  We would need to edit these apps' Info.plist files, and to figure out how to relax their sandbox rules.)
Here's a revision of my original interpose library.  It now logs to both stdout and the system console.  This is needed for XPC services, which run as root and for which it's not possible to access stdout.

I've also figured out how to get this to work on OS X 10.9 and 10.10.  It's a rather nasty hack, which I hope eventually to replace with something better.  In fact I've already come up with something better, but that works only on OS X 10.9 (not 10.10).  It relies on setting EnvironmentVariables in an XPC service's Info.plist.  But this doesn't work the same way on 10.10 as it does on 10.9.  And I probably won't be able to figure out why until Apple releases the source for its launchd for 10.10 (it's currently only available for 10.9.5 ad below).

Here's my nasty hack:

1) Rename PluginProcessShim.dylib in the appropriate WebKit framework's PluginProcess.app to something like PluginProcessShim-org.dylib.

Here are the locations of this file on OS X 10.9.5 and 10.10.3:

OS X 10.9.5:
/System/Library/StagedFrameworks/Safari/WebKit2.framework/PluginProcess.app/Contents/MacOS

OS X 10.10.3:
/System/Library/Frameworks/WebKit.framework/PluginProcess.app/Contents/MacOS

2) Copy interpose.dylib to the appropriate location as PluginProcessShim.dylib.

This prevents Safari (and WebKit) from using the original PluginProcessShim.dylib.  But as best I can tell this doesn't really matter -- at least in 64-bit mode.
Attachment #8600640 - Attachment is obsolete: true
> I've also figured out how to get this to work on OS X 10.9 and 10.10.

I've also figured out how to get this to work with Safari on OS X 10.9 and 10.10.
Updated for Flash 17.0.0.188, which is now the current version.
Attachment #8605543 - Attachment is obsolete: true
Oops, that last one contained a mistake.  This fixes it.
Attachment #8608910 - Attachment is obsolete: true
After all this work, it turns out that, when exercising the STR from comment #8 in Firefox (a current m-c nightly) and Safari, Flash receives exactly the same NPAPI focus events from each.  So *that*'s not the problem.

I revised my interpose library to log *all* events sent to Flash.  Then I discovered that, when the bug happens in Firefox, Flash *doesn't* receive an NPCocoaEventTextInput event which it does receive when running in Safari.  So that's the most likely trigger for this bug.

Of course now we need to find out *why* Firefox doesn't send that event.  For this it may help to see call stacks for when Flash receives the NPCocoaEventTextInput event (in both Safari and Firefox).  And since all such events are sent from the main process to the plugin process, it should also help to see call stacks for when Firefox calls CallNPP_HandleEvent() (in the main process) on an NPCocoaEventTextInput event.

That's what this new interpose library does.

Note that this interpose library now binary patches both Flash and Firefox.  So, just as it's likely to break (to crash) with newer versions of Flash, it's also likely to crash with anything but recent m-c nightlies (with which I've been using it).
Attachment #8608997 - Attachment is obsolete: true
(Following up comment #18)

I should have added that you can get call stacks of Firefox (correctly) sending the NPCocoaEventTextInput event if (while the Flash plugin is running) you click on the Desktop and then back again on the browser window containing the Flash plugin.  As mentioned above, this works around the bug.
Looking at my interpose library's logging output (including of Firefox successfully sending NPCocoaEventTextInput events to Flash), plus the results of grepping the Mozilla tree for "NPCocoaEventTextInput", it's pretty clear that the following code is important to this bug:

http://hg.mozilla.org/mozilla-central/annotate/b0315d00af9e/dom/plugins/base/nsPluginInstanceOwner.cpp#l1970

So I added logging to the Mozilla tree, starting with this code.  The logging was intended to find out which code paths where taken and why.  Working backwards from this code block (by adding additional logging), I eventually found out that this bug's trigger is nsChildView::IsPluginFocused() returning the "wrong" answer -- false instead of true.

So apparently something in the Mozilla tree causes nsChildView::mPluginFocused to be set incorrectly when a plugin is first loaded, but before the page containing the plugin has been unfocused and refocused.

I'll continue my analysis, using more interpose libraries and/or debug logging patches.  But at this point I strongly suspect we've bumped into a bug in cross-platform code.
Attachment #8600641 - Attachment is obsolete: true
(Following up comment #20)

Forgot to add that my debug logging patch uses RTTI (http://en.wikipedia.org/wiki/Run-time_type_information).  So to build it you need to add the following to your mozconfig:

ac_add_options --enable-cpp-rtti
(Following up comment #20)

Note also that you must not strip symbols when building my debug logging patch, or its call stacks will be meaningless.

ac_add_options --disable-strip
ac_add_options --disable-install-strip
All calls to set nsChildView::mPluginFocused are made through nsChildView::SetPluginFocused():

http://hg.mozilla.org/mozilla-central/annotate/517791f65cd9/widget/cocoa/nsChildView.mm#l1691

But this bug's STR (from comment #8) doesn't trigger a call to this method.

At first I thought this might be because cross-platform code wasn't sending the appropriate event or making the appropriate call.  But then I noticed that HTMLObjectElement::OnFocusBlurPlugin() (the origin of all calls to nsChildView::SetPluginFocused()) *is* getting called, but that (in this bug's case) it doesn't call nsChildView::SetPluginFocused().  Furthermore, this is because nsIObjectLoadingContext::GetHasRunningPlugin() returns the "wrong" result -- it returns false even when a plugin is running.

nsObjectLoadingContent::HasRunningPlugin() returns true if nsObjectLoadingContent::mInstanceOwner is non-null:

http://hg.mozilla.org/mozilla-central/annotate/517791f65cd9/dom/base/nsObjectLoadingContent.h#l223

So at first I thought nsObjectLoadingContent::mInstanceOwner must have been set incorrectly.  But then I proved that's not the case, and noticed that nsIObjectLoadingContent::GetHasRunningPlugin() can do an error return (and in effect return false), when it's called from JavaScript code in web content:

http://hg.mozilla.org/mozilla-central/annotate/517791f65cd9/dom/base/nsObjectLoadingContent.cpp#l3144

That's exactly what's happening in this bug's case.

I've attached the debug logging patch I used to reach these conclusions.  I also added a workaround for the problem, which is a possible fix.
Attachment #8616158 - Attachment is obsolete: true
Attachment #8616158 - Attachment is obsolete: false
Attached patch Fix (obsolete) — Splinter Review
This patch fixes the bug in my tests.

Johns, if you don't have time for this review, please pass it along to another plugins person :-)

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca51473c8f37
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8620388 - Flags: review?(john)
Attachment #8620388 - Flags: review?(bugs)
My patch also fixes bug 1158839.
Comment on attachment 8620388 [details] [diff] [review]
Fix

Why don't we just fix GetHasRunningPlugin() ?
It is called only here, and its behavior is super weird. Why does it depend on
caller not being chrome? Perhaps some leftover from pre-webidl times?

Or, actually, do you need to use the com version of the method.
Why not just HasRunningPlugin() which is there on nsObjectLoadingContent
Attachment #8620388 - Flags: review?(bugs) → review-
> Why not just use HasRunningPlugin() which is there on nsObjectLoadingContent?

Fine with me.  New patch coming up.
Attachment #8620388 - Attachment is obsolete: true
Attachment #8620388 - Flags: review?(john)
Attached patch Fix rev1Splinter Review
How's this?
Attachment #8620471 - Flags: review?(bugs)
Comment on attachment 8620471 [details] [diff] [review]
Fix rev1

assuming this fixes the bug, looks good ;)
Attachment #8620471 - Flags: review?(bugs) → review+
I've started another set of tryserver builds with my rev1 patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=59498c6cb683
Comment on attachment 8620471 [details] [diff] [review]
Fix rev1

Since this doesn't change the plugin-specific logic at all, I figure I don't need additional review from a plugins person.

I'll land it on mozilla-inbound when it reopens.
I'll let this bake for a while, then (if all goes well) request uplift to aurora and beta.
https://hg.mozilla.org/mozilla-central/rev/0a716b0dee44
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8620471 [details] [diff] [review]
Fix rev1

Approval Request Comment
[Feature/regressing bug #]: bug 1147521
[User impact if declined]: Bad UI bug will effect some plugin users
[Describe test coverage new/current, TreeHerder]: Baked for a week on m-c
[Risks and why]: Very low risk -- very minor change to code logic
[String/UUID change made/needed]: None

This has baked on m-c for about a week with no reported problems.  Also the patch is very simple and straightforward, with almost no risk.  Thanks again, Smaug, for your suggestion :-)
Attachment #8620471 - Flags: approval-mozilla-beta?
Attachment #8620471 - Flags: approval-mozilla-aurora?
Comment on attachment 8620471 [details] [diff] [review]
Fix rev1

Taking it for 40, Liz will make the call for 39.
Attachment #8620471 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8620471 [details] [diff] [review]
Fix rev1

Sounds good to me, approved for beta uplift
Attachment #8620471 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Reproduced the initial issue using Firefox 37.0.1 on Mac OS X 10.10, verified that the issue does not reproduce anymore using Firefox 39 beta 7, latest Aurora and latest Nightly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.