Closed Bug 1092630 Opened 7 years ago Closed 7 years ago
get rid of native widgets for OS X NPAPI plugins
We should get rid of native widgets for OS X NPAPI plugins. The patch here gives us cleaner code, less code, and code that works with e10s. Currently, we're at: 38 files changed, 766 insertions(+), 2192 deletions(-) Plugins are working much better in e10s mode. Things that still need to be fixed: 1. Our ConvertPoint code is not working 100% yet. This can cause windows like context menus to appear in the wrong place. 2. Complex text input isn't working in e10s mode. We're going to have to remote this to the parent process to get it to work. This patch includes contributions from Markus Stange, Steven Michaud, and David Parks. Since this is a big patch and will need multiple reviewers, I'd like to start the review process now while David Parks and I finish fixing the ConvertPoint code. Once David and I have fixed that, and reviews are complete, we should land this. We can fix the complex text stuff after we land, it doesn't regress non-e10s.
I'd like to hold off on my review until I can do fairly comprehensive manual tests in non-e10s mode. > 1. Our ConvertPoint code is not working 100% yet. This can cause > windows like context menus to appear in the wrong place. It sounds to me that this needs to be fixed before that's possible.
This resolves the NPN_ConvertPoint issues.
Comment on attachment 8516474 [details] [diff] [review] fix v0.9 >nsSubDocumentFrame.h ... >+ nsIntPoint GetChromeDisplacement(); This methods needs good comment about what coordinates it is returning and in which coordinate space. And perhaps even comment how it changes when the document in which the relevant nsIFrame object is in is zoomed (if it changes). Same with GetChromeDisplacement in TabChild.
How does the patch deal with the case when one just moves the window without causing any reflows?
Will this removal have an impact for Thunderbird? If I know correctly, there is no e10s version of Thunderbird.
This updated patch includes the following fixes: 1. We were accidentally still creating a widget for plugins. We weren't using it, because we were assuming there was no widget. In addition to wasting memory, this bug made nsPluginInstanceOwner's mWidget member non-null which was producing surprise results when testing various code paths. In this updated patch, we no longer create the widget. 2. The code path for in-process plugin context menus depended on having a widget, so it was broken. There's no reason we can't use the OOP approach for in-process context menus though, so I just got rid of the old in-process code path and switched in-process plugins over to using the OOP code. Works great, less code. 3. I killed off "layout/generic/nsPluginUtilsOSX.*" entirely. None of this code is necessary any more with the changes to the in-process context menu code path. 4. I re-implemented non-10s ConvertPoint so as not to depend on having a native widget of any kind. It's also now alongside the e10s method, instead of being in the layout utils I killed off. Tests pass, context menus work. We're now at: 43 files changed, 961 insertions(+), 2471 deletions(-)
I'd hoped to get to this this week, but I was sidelined by bug 1092855. Sorry for the delay. I should have time next week.
This update includes fixes for the e10s version of ConvertPoint, and resolves the earlier review comments from smaug.
I could review this after Steven has gone through this.
Comment on attachment 8519360 [details] [diff] [review] mac-e10s-plugins-full-Nov-7-2.patch Josh, I'm having trouble getting your patch to build in 32-bit mode (for universal builds). Did you ever do universal builds, or test them?
There is a fix for 32-bit builds on my branch. https://github.com/bdaehlie/gecko-dev/tree/macplugins-e10s-5 I just haven't updated the patch here yet, was going to wait for fixes for test failures first. You can look at the commit log for that branch to see the fix.
This is a somewhat-sloppy WIP for the test failures. They were: > TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_mouse_coords.html | p1 mouse move Y - got 18, expected 38 > TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_mouse_coords.html | p2 mouse move Y - got 18, expected 38 > TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_plugin_mouse_coords.html | p3 mouse move Y - got 11, expected 31 and > TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | Plugin should not have focus. - got true, expected false > TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | Focus event count should be 4 - got 3, expected 4 > TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | Plugin does not know its initial top-level window activation state! - expected PASS > TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | uncaught exception - Error: Error calling method on NPObject! at http://mochi.test:8888/tests/dom/plugins/test/mochitest/cocoa_window_focus.html:68 test_plugin_mouse_coords has been fixed and the others are improved but a new batch of errors have also popped up. They are: > 15 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | (6) Plugin should not have focus. - got true, expected false > 16 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_focus.html | Focus event count should be 4 - got 3, expected 4 > 9 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | Activation state should be: deactivated - got true, expected false > 10 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | Window focus event count should be 2 - got 1, expected 2 > 14 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | Window focus event count should be 3 - got 1, expected 3 This patch also fixes an issue that caused an ASSERT that was spamming the console. test_cocoa_window_focus is only failing because the plugin is not in-view and is not being told when the top-level window gets focus. I'm stuck trying to find a way to get the event from the nsChildView to the nsPluginInstanceOwner (something is swallowing the event for the off-screen view). If I make the plugin windows 50x50 (so they fit on-screen in the test) then the test passes. The issue with test_cocoa_focus is even dumber.
I can now make 32-bit builds. It took more than just your patch, Josh ... but not much more. So I can now start testing. I'm afraid it will go into next week, though. I've already found one crash bug. More info in later comments.
The crash bug is that the content process crashes when you press and release the option key while the keyboard focus is in a plugin. I tested with my Debug Events Plugin from bug 441880.
Here's Josh's patch, fixed to allow 32-bit (and universal) builds, and also updated to current trunk. Unfortunately it now doesn't allow plugins to take focus. Other changes to plugin code on the trunk are presumably responsible. I'll deal with this next week ... if Josh doesn't beat me to it.
I should mention that so far all my tests have been in e10s mode. But I think it's even more important to make sure these changes don't break things in non-e10s mode. I'll get to that next week.
Still working on test failures. This patch fixes this test failure: > 10 INFO TEST-UNEXPECTED-FAIL | /tests/dom/plugins/test/mochitest/test_cocoa_window_focus.html | Window focus event count should be 2 - got 1, expected 2 leaving the other four. Also fixes a bug with missing video introduced in last version of this patch. For example, this video: http://www.ted.com/talks/james_howard_kunstler_dissects_suburbia?language=en
Attachment #8522588 - Attachment is obsolete: true
More work on test failures. This fixes test_cocoa_window_focus.html completely, leaving only test_cocoa_focus.html failures.
Attachment #8523276 - Attachment is obsolete: true
Here's my patch from comment #15, updated again to current trunk. Somehow the plugin focus issue I reported in comment #15 has now been resolved ... though not through any change in the patch itself. The crash (in the content process) when pressing and releasing the option key still happens. I'll continue testing, and looking at the patch, over the course of this week ... and probably also into next week.
Attachment #8523263 - Attachment is obsolete: true
(In reply to Steven Michaud from comment #19) > The crash (in the content process) when pressing and releasing the option > key still happens. I'll continue testing, and looking at the patch, over > the course of this week ... and probably also into next week. Hey Steven, we were under the impression you were going to do cursory reviews to get this landed, then follow up with more exhaustive reviews. Is that still the plan? Also if we have plugin specific test failures on mac I think those can be disabled and fixed in follow ups as well.
As I understood it, someone else (Johnny Stenback?) was going to rubber stamp Josh's patch so it can land on m-c, and I'd do a detailed, thorough review while it's in the tree. Josh's last patch *can't* land as is, because it won't build. And this isn't only because of bitrotting -- Josh's patch doesn't support building in 32-bit mode (so it won't build in universal mode). My update of Josh's patch from comment #19 *does* build in universal mode. And so we *could* land that, at least in principle. However, I suspect doing that is asking for trouble. Let me at least first track down the option key crash I've been talking about, and see if there are any other showstoppers (especially in non-e10s mode). That will take a couple of days, at least.
(In reply to Steven Michaud from comment #21) > As I understood it, someone else (Johnny Stenback?) was going to rubber > stamp Josh's patch so it can land on m-c, and I'd do a detailed, thorough > review while it's in the tree. jst has rubber-stamped the landing
(In reply to Steven Michaud from comment #21) > My update of Josh's patch from comment #19 *does* build in universal mode. > And so we *could* land that, at least in principle. However, I suspect > doing that is asking for trouble. Let me at least first track down the > option key crash I've been talking about, and see if there are any other > showstoppers (especially in non-e10s mode). That will take a couple of > days, at least. Ok sounds great. We'll wait for your initial "show stopper" investigation. Please let us know when its complete.
This patch fixes the remaining test failures (the ones in test_cocoa_focus).
Attachment #8524105 - Attachment is obsolete: true
OK, I've now fixed the option-key-crash showstopper (by adding code to Josh's patch). But I've now found another, which I don't think will be so easy to fix: As best I can tell, Josh's patch breaks keyboard input in all Flash "movies", whether or not e10s mode is on. This could easily take me several days to fix. And it may turn out to be a design flaw (of Josh's patch), and therefore be even harder to fix. The example I tested with is on the following page. Scroll down to "What is your first name", and look just above the first match. http://multimedia.journalism.berkeley.edu/tutorials/actionscript3-2/ By the way, this example works fine in today's m-c nightly, in non-e10s mode. (If anyone knows of other examples of Flash movies that accept keyboard input, please post links here.)
I'll start a tryserver build of Josh's patch as it now stands (with my fix for the option key crash), so that others can also test with it. I won't bother with tests, for the time being.
The build, when it's done, should appear here: https://tbpl.mozilla.org/?tree=Try&rev=20a1f3d2b7e9
I updated my branch to current trunk. It does not include the additional fixes here from Steven and David. https://github.com/bdaehlie/gecko-dev/tree/macplugins-e10s-6 I'd like to get them committed individually to my branch.
The crash was actually in my Debug Events Plugin. The underlying problem is that (without this fix) Josh's patch sends an NPCocoaEventFlagsChanged event to the plugin as an NPCocoaEventKeyDown event.
Text input doesn't work in Silverlight, either (with or without e10s): http://samples.msdn.microsoft.com/Silverlight/SampleBrowser/index.htm#/?sref=RTAFormat2 But for the moment I'll count this together with the Flash problem, and not as a separate showstopper.
Another showstopper, or at least a semi-showstopper: All of the Shockwave examples here open is separate windows. All of them also crash when you close one of those windows. This doesn't happen in yesterday's or today's m-c nightly. Fortunately, in e10s mode it's the content process that crashes (in non-e10s mode the main/chrome process crashes). http://dreamsteep.com/projects/shockwave-examples.html
More crashiness, this time closing a tab containing the following Unity plugin example. Once again the crashes don't happen with today's or yesterday's m-c nightlies. I'd say the crashes-on-closing-window-or-tab now rate as a full-fledged showstopper. Which means the count of unfixed showstoppers is up to two. http://unity3d.com/showcase/live-demos#butterfly
(In reply to Steven Michaud from comment #32) > Another showstopper, or at least a semi-showstopper: > > All of the Shockwave examples here open is separate windows. All of them > also crash when you close one of those windows. This doesn't happen in > yesterday's or today's m-c nightly. Fortunately, in e10s mode it's the > content process that crashes (in non-e10s mode the main/chrome process > crashes). > > http://dreamsteep.com/projects/shockwave-examples.html eh, I wouldn't block on broken shockwave. I think we can follow up this work if it's isolated to just shockwave apps.
> I wouldn't block on broken shockwave Like I said, it's not just Shockwave. It's also Unity (a completely unrelated plugin), and probably also other plugins. I'd say this definitely is another showstopper.
I'm going to have to stop working on this bug for at least few days, to try to deal with an even worse problem -- bug 1093683.
It would be very helpful if others could try to diagnose and fix the two showstoppers: 1) Text input doesn't work in (at least) Flash and Silverlight 2) Windows/tabs containing (at least) Shockwave and Unity plugins crash when closed.
I just pushed the current fixes here from Steven and David to my branch, along with another fix from me. All that remains, that I know of, is the two problems Steven points out in comment 37.
With this patch (applied atop the github macplugins-e10s-7 branch), OSX tests on try are all green. There are still a handful of Linux failures: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a70c7a5f5d53 This does not fix the input issues Steven reported.
Current full patch, including fixes from me, David Parks, and Markus Stange. This passes try server and includes a probably fix for the crash Steven was seeing, as well as a fix for Flash keyboard input.
Attachment #8519360 - Attachment is obsolete: true
Attachment #8524145 - Attachment is obsolete: true
Attachment #8524972 - Attachment is obsolete: true
Attachment #8525440 - Attachment is obsolete: true
Attachment #8525443 - Attachment is obsolete: true
Attachment #8530972 - Attachment is obsolete: true
Attachment #8519360 - Flags: review?(smichaud)
Bad news. Josh's patch does *not* fix text input in Flash or Silverlight, even in non-e10s mode. I tested with the urls from comment #25 and comment #31. It also doesn't fix the crashes that happen closing a tab containing the Unity plugin example from comment #33. It does, however, appear to fix the Shockwave crashes described in comment #32. I'd appreciate seeing others' results, testing with Josh's patch from comment #41. I wasn't able to apply Josh's patch to current trunk, because I can't do universal builds in current trunk code. Instead I applied it to m-c as of http://hg.mozilla.org/mozilla-central/rev/ff4a63d66806.
I've started a tryserver build (without tests) of Josh's patch on current trunk, in case that makes a difference: https://tbpl.mozilla.org/?tree=Try&rev=34ad7ea12799 I'll test with it tomorrow morning.
I've just tested my tryserver build from comment #43, and it fixes *none* of the showstoppers. Unlike my self build, it also crashes closing a window that contains a Shockwave plugin. Josh, are you sure you posted the right patch? :-)
Here's a crash stack generated using my tryserver build from comment #43 and the STR from comment #32. I tested in non-e10s mode on OS X 10.7.5.
Here's a crash stack generated using my tryserver build from comment #43 and the STR from comment #33. I tested in non-e10s mode on OS X 10.7.5.
Josh did have the right patch, and things aren't quite as bad as I first thought ... but they're still bad. For some reason, Flash plugin text input *does* work on OS X 10.9.5 and 10.10.1 -- though it doesn't on OS X 10.7.5 (and presumably also 10.6.8 and 10.8.5). Josh thought up a couple of possible quick fixes, but neither of them worked. I'm going to take on the task of fixing plugin text input. I won't have the time or mental energy to work on this until after I get home, next week, after the end of the cooincident work week. Even then it could easily take me the whole week. But I will make it my top priority once I get home. Josh is going to take on the crashes.
Here are a couple of Socorro crash stacks: bp-dad6f33c-8f94-4608-be3d-4f79b2141205 bp-6a627baf-0bab-4292-acb9-5b93b2141205 Here's the output of otool -t -V -p for the top of nsNPAPIPluginInstance::ConvertPoint(), where the crashes take place at offset '62'. The last line is where the crashes appear to take place (and which appear to be a genuine null-dereference): __ZN21nsNPAPIPluginInstance12ConvertPointEdd17NPCoordinateSpacePdS1_S0_: 0000000001849ce0 pushq %rbp 0000000001849ce1 movq %rsp, %rbp 0000000001849ce4 pushq %r15 0000000001849ce6 pushq %r14 0000000001849ce8 pushq %r13 0000000001849cea pushq %r12 0000000001849cec pushq %rbx 0000000001849ced subq $24, %rsp 0000000001849cf1 movl %r8d, -44(%rbp) 0000000001849cf5 movq %rcx, %r15 0000000001849cf8 movq %rdx, %r12 0000000001849cfb movl %esi, %r13d 0000000001849cfe movsd %xmm1, -56(%rbp) 0000000001849d03 movsd %xmm0, -64(%rbp) 0000000001849d08 movq 104(%rdi), %r14 0000000001849d0c xorb %al, %al 0000000001849d0e testq %r14, %r14 0000000001849d11 je 0x1849d71 0000000001849d13 callq _XRE_GetProcessType 0000000001849d18 movl %eax, %ebx 0000000001849d1a movq 64(%r14), %rdi For the moment I'm stumped. I don't see any of this stuff in the source code for nsNPAPIPluginInstance::ConvertPoint().
Oops, *this* block's last line is where the crashes take place: __ZN21nsNPAPIPluginInstance12ConvertPointEdd17NPCoordinateSpacePdS1_S0_: 0000000001849ce0 pushq %rbp 0000000001849ce1 movq %rsp, %rbp 0000000001849ce4 pushq %r15 0000000001849ce6 pushq %r14 0000000001849ce8 pushq %r13 0000000001849cea pushq %r12 0000000001849cec pushq %rbx 0000000001849ced subq $24, %rsp 0000000001849cf1 movl %r8d, -44(%rbp) 0000000001849cf5 movq %rcx, %r15 0000000001849cf8 movq %rdx, %r12 0000000001849cfb movl %esi, %r13d 0000000001849cfe movsd %xmm1, -56(%rbp) 0000000001849d03 movsd %xmm0, -64(%rbp) 0000000001849d08 movq 104(%rdi), %r14 0000000001849d0c xorb %al, %al 0000000001849d0e testq %r14, %r14 0000000001849d11 je 0x1849d71 0000000001849d13 callq _XRE_GetProcessType 0000000001849d18 movl %eax, %ebx 0000000001849d1a movq 64(%r14), %rdi 0000000001849d1e movq CONFIG_EMULATE_HARDWARE_HIGHBITDEPTH(%rdi), %rax
Among other things, this resolves the crashes Steven was seeing.
I had a lucky break, and found a quick and dirty way to fix plugin text input ... sort of. Plugin IME still doesn't work on OS X 10.7 and below, but both Josh and I agree we can leave this problem to be fixed later. With this patch, non-IME plugin text input works "everywhere" -- in e10s mode and non-e10s mode, on all supported versions of OS X. Plugin IME text input works in non-e10s mode on OS X 10.8 and up. It doesn't work in e10s mode, and never will with the current design -- which processes keyboard events in a background process. (Both Chrome and Safari/WebKit process keyboard events in the main process while doing plugin IME. We'll eventually have to do the same ... but the change will be a lot of work.)
Here's Josh's patch, with my patch added and updated to current trunk. I've started a tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=9db4da2460a1 I still haven't fully reviewed Josh's patch. But I'm comfortable landing this on the trunk. Over the next few weeks I can go through the patch carefully and resolve any issues I find with Josh.
> Plugin IME still doesn't work on OS X 10.7 and below Even in non-e10s mode.
Note that plugin IME is broken in Flash -- even in Safari. When you test plugin IME with the example from comment #25, composition takes place in a special window below the plugin. But no text appears in the Flash text field when you hit Enter (and commit the text you've been composing). It does work properly (in non-e10s mode, on OS X 10.8 and up) with the Silverlight example from comment #31.
I pushed the latest full patch to mozilla-inbound. https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a6db8f54f5aa
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
> Note that plugin IME is broken in Flash -- even in Safari. Interestingly, this is only true for the example from comment #25 -- not for the example from comment #39. Possibly the example from comment #25 has some kind of filter that prevents non-Roman input.
Congrats on landing this everyone! Does anyone know the status of bug 1044182 or bug 1051842? Also, on filing follow ups - 1) if there's a regression that affects non-e10s, please remember to mark the appropriate release tracking flags plus tracking-e10s:? 2) bugs in e10s or incomeplete work TBD - tracking-e10s:? so we can find it. Thanks!
You need to log in before you can comment on or make changes to this bug.