Closed Bug 1092630 Opened 10 years ago Closed 10 years ago

get rid of native widgets for OS X NPAPI plugins

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla37

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(2 files, 17 obsolete files)

Patch to fix plugin text input
1.76 KB, patch
Details | Diff | Splinter Review
Full current patch
249.37 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
Attached patch fix v0.8 (obsolete) — — Splinter Review 
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.
Attachment #8515549 - Flags: review?(smichaud)
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.
Attached patch fix v0.9 (obsolete) — — Splinter Review 
This resolves the NPN_ConvertPoint issues.
Attachment #8515549 - Attachment is obsolete: true
Attachment #8515549 - Flags: review?(smichaud)
Attachment #8516474 - Flags: review?(smichaud)
Attachment #8516474 - Flags: review?(enndeakin)
Attachment #8516474 - Flags: review?(bugs)
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.
Attached patch fix v1.0 (obsolete) — — Splinter Review 
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(-)
Attachment #8516474 - Attachment is obsolete: true
Attachment #8516474 - Flags: review?(smichaud)
Attachment #8516474 - Flags: review?(enndeakin)
Attachment #8516474 - Flags: review?(bugs)
Attachment #8519307 - Flags: review?(smichaud)
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.
Attachment #8519307 - Attachment is obsolete: true
Attachment #8519307 - Flags: review?(smichaud)
Attachment #8519360 - Flags: review?(smichaud)
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.
Attached patch WIP: Fix test failures (obsolete) — — Splinter Review 
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.
Blocks: e10s-plugins
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.
Attached patch WIP: Fix test failures (obsolete) — — Splinter Review 
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
Attached patch WIP: Fix test failures (obsolete) — — Splinter Review 
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.
Flags: needinfo?(smichaud)
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.
Flags: needinfo?(smichaud)
(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.
Attached patch Fix test failures (obsolete) — — Splinter Review 
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.
Here's another test case for Flash text input, from bug 1103255:  attachment 8527076 [details].
Attached patch WIP: Fix more test failures (obsolete) — — Splinter Review 
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.
Attached patch macplugins-e10s-8.patch (obsolete) — — Splinter Review 
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
Attached patch full current patch (obsolete) — — Splinter Review 
Among other things, this resolves the crashes Steven was seeing.
Attachment #8531598 - Attachment is obsolete: true
Attachment #8532213 - Attachment is obsolete: true
Attachment #8532215 - Attachment is obsolete: true
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.
Attachment #8533310 - Attachment is obsolete: true
Attachment #8534695 - Flags: review+
> 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
https://hg.mozilla.org/mozilla-central/rev/a6db8f54f5aa
Status: NEW → RESOLVED
Closed: 10 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.
Depends on: 1110641
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!
Blocks: 1121811
Blocks: 1130435
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: