Closed Bug 1153145 Opened 5 years ago Closed 5 years ago
Keystrokes Flash Player do not get handled in pop ups
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
|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: 126.96.36.199 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/
smichaud, if this is a regression from the recent Mac plugin changes, is this something that you could mentor spohl to help with?
> 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?
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.
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.
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/
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 (188.8.131.52), 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 (184.108.40.206) 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 220.127.116.11, 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
Attachment #8616158 - Attachment is obsolete: true
Attachment #8616158 - Attachment is obsolete: false
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
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 #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.
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 :-)
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 Landed on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/fc04c54efcdb
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+
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.
You need to log in before you can comment on or make changes to this bug.