1044182 - Plugin focus is broken in e10s on Mac

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Jim's editor windowless testcase

This is spun off bug 586656.

STR:

1) Use a Mac. Use a build of Firefox that has this patch:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/8581ea023552
(bug 586656 comment 54).

2) Open an e10s window

3) Open the attached editor windowless testcase (from Jimm's archive on bug 586656).

4) Once the page is loaded, click on the editable area.

Expected results:

Clicking the editable area should focus it and give a text-editing cursor.

Actual results:

Clicking the editable area doesn't do anything. It doesn't get focused. No cursor.


Some debugging of what is going on there, was posted in bug 586656 comment 50:

(In reply to Benoit Jacob [:bjacob] from comment #50)
> Here is an update on my attempts to debug the focus issues on Mac.
> 
> I've added some logging and compared what happened between e10s and non-e10s.
> 
> Here is the basic difference:
> 
> In non-e10s (where things work), when I click on the plugin area, I get the
> becomeFirstResponder callback called (as defined in nsChildView.mm) on the
> plugin's view. That in turn generates the focus events.
> 
> In e10s (where things do not work), that becomeFirstResponder callback (in
> nsChildView.mm) is never called - neither for the plugin view nor for any
> other view, it seems.
> 
> I'm failing to understand that so I think I'll work on other stuff until
> someone with Mac widget knowledge can help me understand that.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Hm, somehow, here, the testcase doesn't want to load the plugin when served from that bugzilla URL... no idea why. You might have to save it and serve it locally.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8462751 [details]
Jim's editor windowless testcase

I just discovered that this testcase only works over HTTP, not HTTPS.  Over an HTTPS connection (which bugzilla uses to serve its testcases), you always get "plugin is not loaded!!"

How bizarre!

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I'm going to take this, at least for the time being.

I see at least two problems, one of which was already reported to me by bjacob on IRC:

1) In an e10s window, the plugin doesn't get the focus when you click on it.

2) But even before you click on it, the cursor should change to an I-beam when you mouse over it -- but it doesn't.

The first is definitely a Cocoa widgets problem (or possibly an obscure Apple bug), so I'll work on it first.  The second might also be a Cocoa widgets problem, or it could be a DOM event bug.  So I'll get to it only after the first problem.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> 1) In an e10s window, the plugin doesn't get the focus when you click on it.

The proximate cause is that, in an e10s window, clicking on a plugin results in the OS sending the mouseDown event to the wrong ChildView (not the plugin's ChildView but its parent ChildView).

Now I've got to reverse-engineer the OS to find out why.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Since I've got so many other things on my plate (especially Mac sandboxing), I'll only be able to work on this intermittently.  I suspect that means it'll take up to several weeks to fix (or to work around if it's an Apple bug).

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I just discovered that, in an e10s window, no ChildView gets created for the plugin!

That's the reason for the problem I reported in comment #4.  So we're shooting ourselves in the foot -- it's not some obscure OS bug.

I don't know what to do about this.  However we cut it, there will be a lot of work involved.  So I'm putting this aside for the next few weeks.  In the meantime, someone else can pick this up if they want.  We shouldn't need any more reverse-engineering of the OS.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Actually we *are* creating a ChildView object for the plugin.  We're just not adding it to the NSView hierarchy, here:

https://hg.mozilla.org/mozilla-central/annotate/f61a27b00e05/widget/cocoa/nsChildView.mm#l602

I'll leave this for others to figure out.  Unless I come back in a few weeks and find this hasn't happened yet.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Ah, thanks for investigating this! Is there someone currently owning this code, whom we could ask to look at this? Anyway, I'll be away for vacation from now until August 10, I'll try to look at this if noone does before but this looks like code that requires some familiarity with Cocoa to understand.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

It's pretty clear that, at the line I cited in comment #7, mParentView is null -- which is unexpected, and doesn't happen in a non-e10s window.  I suspect the reason is that nsChildView::Create() is being called from cross-platform code with aParent == NULL.  If so, the bug here is (probably) in cross-platform code (not in widget code).

I'll try to find time in the next few days to establish this (or disprove it).  That will leave this bug in better shape for you to work on it when you get back from vacation.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Many thanks! much appreciated.

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Your findings already seem enough to allow me to continue working on this bug when I come back, so, re-self-assigning. Any further help is still welcome!

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Trace of plugin creation in e10s window

> I'll try to find time in the next few days to establish this (or disprove it).

It was actually quite easy.  And I was right ... sort of.

aParent isn't NULL.  Instead it's a PuppetWidget, for which GetNativeData(NS_NATIVE_WIDGET) returns NULL.

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Attachment: Trace of plugin creation in normal window

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debug logging patch I used to get those two traces

I used RTTI plus my interpose library (from http://people.mozilla.org/~stmichaud/ReverseEngineering/InterposeLibraryTemplate/) to get those two traces.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Another thing I just realized after looking at my e10s window trace:

The plugin's native ChildView is being created in the plugin-container process!

That won't do!!!  It needs to be created in and used by the main process.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

The content process's plugin support needs some pretty serious redesign -- at least on the Mac.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Steven Michaud from comment #15)
> That won't do!!!  It needs to be created in and used by the main process.

What do we do on other platforms with windowless plugins? I imagine that with e10s the details of the content view isn't remoted on other platforms and that content view are just an opaque component that we forward events and receive painted buffers for. It would be a real shame to have to synchronize a ChildView into the chrome process for plugins to work in the child process. This would be like regressing back to windowed plugins.

Now that we dropped support for QuickDraw how hard would it be for our purely windowless plugins to no longer require a ChildView? If we're going to do major surgery I would prefer to go in that direction.

If we keep a ChildView in the parent process then not only is it going to be a pain with things like APZC and sycnonizing drawing but when/if we add sub iframe content process then we're going to have to remote the ChildView across another IPC layer.

Is it feasible to support plugins without a Childview in the main process?

Comment 18

[Markus Stange [:mstange]]

I agree completely with Benoit. OOP Plugins shouldn't need to care at all whether there's an NSView for them anywhere. I'd really appreciate if someone could put in the work needed to get rid of plugin ChildViews. I don't have much experience with plugins so I'd hoped somebody else would do it at some point, but if nobody steps up I think I can spend some time on the problem.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Steven is on PTO this week. I think we should wait until next week and set up a quick meeting with relevant people to discuss what should be done and who should do it. Since at this point it's not clear at all that I should be the one trying to fix this, let me un-assign myself for now.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

I agree that getting rid of plugin-specific ChildView objects is a good idea in the long run.  But it will be a lot of work, and we should *not* commit ourselves to doing this, on a short timeline, to fix an entirely unrelated problem.

I've just gotten back from vacation, and am currently very busy.  But I'll try to find time in the next two or three weeks to make e10s plugin support work consistently with current code -- i.e. to have the ChildView object always created in the main process, plus whatever other changes are needed to e10s code to make this work properly.

Note that I'm not very familiar with e10s (content process) code, and am currently very busy with other things.  So I won't cry if someone else gets to this work before me.

Comment 21

[Chris Peterson [:cpeterson]]

Moving to M3 milestone. These bugs affect e10s dogfooding but Brad and Gavin did not think they were M2 blockers.

Comment 23

[David Parks [:handyman]]

Attachment: nsPluginInstanceOwner::ConvertPoint that works in content process

ConvertPoint implementation, also available from my github repo (macplugins-e10s-dp).  Most of the bookkeeping is obvious but most of the trickiness is due to needing a way to map from local plugin coordinates to screen coordinates in the content proc.  I'd have preferred to store the needed data in the plugin objects but I wasn't able to get that to play (I had been using the PluginHost to push to all active plugins simultaneously instead of the current on-demand strategy).

Comment 24

[David Parks [:handyman]]

Attachment: WIP: nsPluginInstanceOwner::ConvertPoint that works in content process

Update to the patch -- this repairs OOP non-e10s plugin behavior.  The issue is that, in the OOP case, non-e10s behavior relied on a widget (that was the size of the plugin) to have been created for the plugin... which I guess we no longer needed to do in the windowless case for obvious reasons.  So the engine was using the entire window to do its calculations, which meant plugin coordinates were mistakenly window coordinates.

I'm not really certain of that -- this is really orthogonal to the ConvertPoint stuff that I worked on and I'm not clear on the non-e10s changes (mainly, did we discard the plugin's NSView or just make it framebuffer-less?).  Still, I've redone the math for this case to use the mPluginFrame but it might be better to leave the code as before if it could be made to work.  Or... maybe its not a big deal either way.

Side note: This also needed to have screen-coordinates flipped like the e10s case in my previous email.  Something else may have gotten mixed up.

Comment 26

[David Parks [:handyman]]

Attachment: WIP: Fix non-e10s coordinates and test_convertpoint

This patch should be applied on top of attachment 8513203 [details] [diff] [review].  It fixes the non-e10s calculations as well as test_convertpoint, which currently only passes on tbpl because it only passes on low-DPI (ie non-retina) devices.

Comment 27

[David Parks [:handyman]]

Attachment: Cleanup comments and ifdefs

This patch cleans up the prior two patches for checkin (they still need to be applied first).  This should finish the convertPoint section of the bug.

NI'ing Josh so that he knows that this is here.  Josh, let me know if this doesn't handle your test case.

Comment 28

[Josh Aas]

Looks good to me, seems to work well. I committed it to my branch.

Comment 29

[David Parks [:handyman]]

Attachment: WIP: Call PlacePanel with correct coordinates

Josh: This should fix the PlacePanel call.  I don't actually know how to repro the issue so can you check it to see if it works?  I'm still looking into the other comment.

Comment 30

[David Parks [:handyman]]

Josh:

From what I can tell, the current implementation of nsPluginInstanceOwner::ShowNativeContextMenu should be safe (you can delete the comment).  It looks to be that the only way that the popup function could possibly get called in the wrong process is if not using OOP plugins (dom.ipc.plugins.enabled.*).  

There are two non-OOP plugin cases:

* non-e10s: Not surprisingly, the call happens in the main process.  From your test plugin, I see that the coordinates are miscalculated.  This works in our previous plugin implementation.

* e10s: The whole application crashes before the plugin appears.  Same as before.

Since non-OOP plugins don't work in e10s and I don't see a reason we should care, we may just be able to deprecate the settings.  Either way, we are using the function in the right processes.  If we want to fix this then I've got some math homework.  (If we care at all, I'd create a new bug -- it'd be very low priority).

Comment 31

[Josh Aas]

(In reply to David Parks [:handyman] from comment #30)

Did a bunch of hacking and debugging tonight, I have a more clear picture of what's going on now.

The important thing about this patch is that it gets rid of the child widget for all OS X plugins in all configurations. You're right that the code path in question here is only executed for in-process plugins, but we have to keep those working for now and this code path is not going to work without a widget.

If you look at commits from tonight on my branch, you'll see that I've entirely removed it, which eliminates the XXXJOSH comment as well. We need to be using more or less the same strategy and code that IPC plugins do for context menus for in-process plugins.

Comment 32

[David Parks [:handyman]]

Attachment: Fix ConvertPointPuppet's window coords calculations

I saw this while comparing e10s and non-e10s in Josh's latest patch in bug 1092630.  There was a minor bug in these equations, mostly due to poor-quality variable names.  This corrects that, too.

There is a remaining issue with window-move and e10s (it screws up the calculations) that looks relatively simple.

Comment 33

[Jim Mathies [:jimm]]

Looks like this landed with bug 1092630.