1130435 - [e10s] Mouse cursor doesn't disappear over Flash object when it should. Works in non-e10s.

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Stephen A Pohl [:spohl]]

Attachment: Reduced testcase

When the mouse hovers over the Flash object in the attachment (see index.html), the mouse cursor is supposed to disappear. This used to work prior to turning on e10s[1]. It continued to work properly in non-e10s until bug 1092630 landed[2]. At that point, both e10s and non-e10s started displaying the cursor permanently (this may or may not have been for two separate reasons). Bug 1121811[3] later fixed this again for non-e10s leaving us with 'just' e10s to fix.

Note that once you launch index.html in an e10s window, it will also be broken in non-e10s until the browser is restarted!


[1] https://hg.mozilla.org/mozilla-central/rev/a75897e664dd (bug 1093691)
[2] https://hg.mozilla.org/mozilla-central/rev/a6db8f54f5aa (bug 1092630)
[3] https://hg.mozilla.org/mozilla-central/rev/b5842b906435 (bug 1121811)

Comment 1

[David Parks [:handyman]]

Attachment: Stack showing Flash calling Cocoa method

Hey Steven,

The problem here is that Flash is calling +[NSCursor hide] to hide the cursor, which can be seen in the attached call stack taken from the plugin process.
  
It appears that they could set NPPVpluginUsesDOMForCursor to true and use scriptaccess to hide the cursor... Or is there something else they should be doing?  (NPPVpluginUsesDOMForCursor isn't documented... anywhere).

And if this is something they need to change, do you know how we let them know?

Comment 2

[David Parks [:handyman]]

I think I misunderstood the NPPVpluginUsesDOMForCursor stuff (I guess its dead) but I guess they still need to switch to using scripts for mouse pointer control...?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

The problem is that +[NSCursor hide] is being called from a background process (the plugin process).

This issue came up once before, and was worked around at bug 621117:  Calls to +[NSCursor hide] and similar functions from plugins are hooked and proxied to the main process ... at least in non-e10s mode.  But (from your log in attachment 8563848 [details]) it seems this is broken in e10s mode.

It'll probably be at least a month before I have time to work on this myself (I've got lots of other stuff on my plate).  But in the meantime I can try to answer your questions (as long as they don't require too much work on my part).  Stephen may also be able to help.

I'm very surprised to hear the NPPVpluginUsesDOMForCursor stuff is "dead".  I'm also surprised that Flash doesn't use it (it was already mentioned in bug 621117 comment #57 and here https://wiki.mozilla.org/NPAPI:DOMCursors).  Maybe we broke it, too, and Flash is just falling back to older usage.

Jeromie, do you know?

Comment 4

[Steven Michaud [:smichaud] (Retired)]

David, do check for proxied calls to +[NSCursor hide] from the content process.  Perhaps the proxying does work in e10s mode, but just to the wrong process (the content process).  The underlying problem is that none of the native cursor manipulation calls work from a background process (which, of course, the content process also is).

Comment 5

[Jeromie Clark]

Oops, didn't actually mean to clear that flag.  I don't.  I'll ask around and get some more detail.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I've been assuming that NPAPI plugins are launched from the content process in e10s mode.  That's wrong -- they're actually launched from the main process.  But it still seems to be true that whatever we do to launch NPAPI plugins in e10s mode has broken my fix from bug 621117.

Comment 7

[David Parks [:handyman]]

Attachment: Avoid multiple swaps of Cocoa routine implementations in plugin process

Sorry, this completely fell off my radar.  But I have good news.

The patch is simple and you can immediately see what went wrong.  We were initiating the Cocoa method swap in PluginModuleChild construction, but the plugin process has *two* PluginModuleChildren... so we swap... and then unswap.  (Which begs the question... why doesn't this exist in non-e10s?)  Regardless, this fixes the issue.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8569548 [details] [diff] [review]
Avoid multiple swaps of Cocoa routine implementations in plugin process

I don't understand why this works.  I'm going to need to test it (with my own added debug logging).

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Better fix

Yes, two PluginModuleChild objects are created in every plugin process.  And yes, that means (in current code) that SetUpCocoaInterposing() gets called twice -- the second call reversing the effects of the first.

But one of those PluginModuleChild objects is specifically for talking to the chrome process -- the one constructed with aIsChrome == true, which is returned by calls to PluginModuleChild::GetChrome().  This is the one that gets used in PluginInterposeOSX.mm, and so is the one from which we want to call SetUpCocoaInterposing().

http://hg.mozilla.org/mozilla-central/annotate/b94bcbc389e8/dom/plugins/ipc/PluginModuleChild.cpp#l198

Comment 10

[Steven Michaud [:smichaud] (Retired)]

This is where the "chrome" PluginModuleChild object gets created.  I mention it because I had a hard time finding the code:

http://hg.mozilla.org/mozilla-central/annotate/b94bcbc389e8/dom/plugins/ipc/PluginProcessChild.h#l23

Comment 11

[David Parks [:handyman]]

Comment on attachment 8570690 [details] [diff] [review]
Better fix

Makes sense to me but I don't have review permission.  I imagine Josh can handle this?

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8570690 [details] [diff] [review]
Better fix

Landed on mozilla-inbound:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5d16f16e693c

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d16f16e693c

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8570690 [details] [diff] [review]
Better fix

This bug only effects e10s mode, which is only on by default on the trunk (currently the 39 branch).  But it's also possible to turn on e10s mode on the other branches.  And this is a pretty bad bug.

Furthermore this patch is both trivial and trivially correct.  There's basically no risk at all.

So let's uplift it to the 38 branch, and also to the 37 branch if there's still time.

Approval Request Comment
[Feature/regressing bug #]: 641685
[User impact if declined]: Nasty plugin UI bug in e10s mode
[Describe test coverage new/current, TreeHerder]: Baked for a week on m-c
[Risks and why]: Basically no risk. Trivial patch that's trivially correct.
[String/UUID change made/needed]: None.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8570690 [details] [diff] [review]
Better fix

I don't mind taking this on Aurora but with the lost week on Beta, not sure this is a viable uplift there - why rush it in for something that's not on by default?  Will leave the approval nomination for Lawrence to make the final call as he is on point for 37 release.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8570690 [details] [diff] [review]
Better fix

Landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/469a0c3d1eb1

Comment 17

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

Comment on attachment 8570690 [details] [diff] [review]
Better fix

I agree with Lukas. Trivial or not, e10s is not supported for 37 and there is not a strong need to take this change. Beta-

Comment 18

[Jeromie Clark]

I added Matt Talistu to this bug a while back to answer any lingering questions on the Flash side.  Clearing out my NeedsInfo.