592563 - Mouse/keyboard events sent to plugin while in background tab, in Cocoa event mode

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

In current Minefield nightlies on OS X, events can be sent by the
browser to a plugin in a background tab (one that isn't currently
focused).  This happens only in Cocoa event mode -- not in Carbon
event mode.

This can easily be reproduced using the current version of my
DebugEventsPlugin from bug 441880.

1) Install my DebugEventsPlugin from bug 441880.

2) Run the Console utility -- which displays console output.

3) Start Minefield and open test.html from the DebugEventsPlugin
   distro.  An instance of the plugin will appear that fills most of
   the upper-left part of the browser window's content region.

4) Open a new tab.

5) In the second tab, click somewhere in the area that used to be
   filled by the plugin instance in the first tab.

   In the console you'll see either a single "getFocus" message or a
   "getFocus" message followed by a "loseFocus" message.  In the
   latter case click once again -- you should see a single "getFocus"
   message.

6) Type one or more keys.

   For each key pressed, in the console you should see (at least) one
   NSCocoaEventKeyDown event and one NPCocoaEventTextInput event.  (By
   the way, this is fewer events than you would normally see if the
   plugin was in the current tab and had the keyboard focus.)

If in step 3 you load test-carbononly.html, the console doesn't
display any plugin events in steps 5 or 6.  This shows that the
problem only exists in Cocoa event mode.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

This should probably block the next beta.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

This is a very recent regression.  Here's the regression range:

2010-08-27-03-mozilla-central
2010-08-28-03-mozilla-central

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-08-27+03%3A00%3A00&enddate=2010-08-28+03%3A00%3A00

In this range I suspect one of the patches landed by tnikkel on Fri
Aug 27 16:42:15 2010 -0700.  I'll use hg bisect to find the right one.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

hg bisect tells me:

The first bad revision is:
changeset:   51636:7804b5cf4313
user:        Robert O'Callahan <[e-mail]>
date:        Fri Aug 27 18:15:08 2010 -0500
summary:     Bug 130078. Part 2. Remove widgets from all subframes. r=dbaron

Presumably this isn't the bug's cause -- just what triggered it.

Comment 4

[juan becerra [:juanb]]

I'm wondering if Marcia is seeing this in bug 592332.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

What I report at bug 581608 comment #8 may be related.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

> I'm wondering if Marcia is seeing this in bug 592332.

Sounds likely.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

> What I report at bug 581608 comment #8 may be related.

Now I no longer think so -- turning off retained layers doesn't make
this bug (bug 592563) stop happening.

I'll open a new bug on this (probably unrelated) matter (what I report
at bug 581608 comment #8) sometime tomorrow.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

I've found another, related bug with the same regression range -- bug 592691.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Another related bug -- bug 592453.

Comment 10

[Marcia Knous [:marcia]]

Steven: I am assuming Bug 592332 is the same as this one?

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> Steven: I am assuming Bug 592332 is the same as this one?

Probably.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I've figured out why this bug (bug 592563) and bug 592691 happen.
I've also now got a fix for both of them.

The basic problem is that, on platforms with child widgets (like OS
X), the patches for bug 130078 stopped plugin widgets from being
"hidden" on switching to another tab.  So the native counterparts of
plugin views (ChildView objects) now stay continuously visible from
the time they've been created.  Which makes it possible for a
left-click to focus a plugin even when it's in a different tab.

For reasons I can't fathom, nsIWidget::Show() has never been called on
plugin widgets.  But this didn't used to cause trouble:  Before the
patches for bug 130078 removed widgets from subframes, a plugin widget
always had an nsIWidget ancestor on which nsIWidget::Show() *was*
called on switching tabs.  And these calls made a plugin widget's
ChildView visible or invisible (as appropriate) by making one of its
superviews visible or invisible.

My solution to the problem is to make sure the plugin nsIView object's
'mWindow' field is set appropriately.  Then if the plugin has a
widget, nsIWidget::Show() will get called on it from
nsView::NotifyEffectiveVisibilityChanged().

This change makes a big difference on platforms with child widgets
(like OS X).  It might also help with "windowed" plugins on other
platforms.

(I know the convention is to say that Mozilla only supports
"windowless" plugins on OS X.  And NPAPI plugins on OS X do get all
their events via NPP_HandleEvent() -- as "windowless" plugins do on
other platforms.  But on OS X plugins do always have "windows" --
i.e. widgets.)

Comment 13

[Marcia Knous [:marcia]]

Steven: Can we get a tryserver build to test :)?

Comment 14

[Steven Michaud [:smichaud] (Retired)]

> Steven: Can we get a tryserver build to test :)?

Sure.  But it'll probably take overnight, since I really should build on every platform :-(

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 472014 [details] [diff] [review]
Fix

Oops, I just found a dumb mistake in this patch ... unfortunately after I'd already started my tryserver builds.

I'll fix the problem and post another patch in a bit.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (fix dumb mistake)

Tryserver builds will follow sometime tomorrow.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #12)
> My solution to the problem is to make sure the plugin nsIView object's
> 'mWindow' field is set appropriately.  Then if the plugin has a
> widget, nsIWidget::Show() will get called on it from
> nsView::NotifyEffectiveVisibilityChanged().

I'd prefer not to do that. It was a deliberate choice to unhook the plugin widget from the view and manage it from the frame instead. Hooking the widget to the view means that the view will start to manage its geometry, which is not what we want.

nsChildView::ConfigureChildren should be setting the clip region for plugin widgets in background tabs to empty, but nsChildView::ApplyConfiguration doesn't do anything with the region. I think we should probably hide/show the view in there based on whether the clip region is empty, and that would fix this bug.

I also wonder why, even if the plugin's NSView is visible, when we click in it the plugin gets focus. All those mouse events should be handled by Gecko and redirected to the topmost tab now. Does MacOS automatically transfer focus to the clicked NSView?

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> nsChildView::ConfigureChildren should be setting the clip region for
> plugin widgets in background tabs to empty, but
> nsChildView::ApplyConfiguration doesn't do anything with the
> region. I think we should probably hide/show the view in there based
> on whether the clip region is empty, and that would fix this bug.

OK, I'll try that.

> Does MacOS automatically transfer focus to the clicked NSView?

Yes, and that's how things should be.  We need to make sure ChildView
objects in background tabs aren't there to be clicked (are
"invisible").

Comment 19

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (follow Roc's suggestion)

Thanks, Roc, for your suggestion!

It works perfectly, and also fixes bug 592453 (which my previous patch
didn't).  So now we have a fix for this bug (bug 592563), bug 592691
and bug 592453.

A tryserver build will follow in (I think) a few hours.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 472185 [details] [diff] [review]
Fix rev2 (follow Roc's suggestion)

I've already found a small problem in my own patch.

This

+    for (PRUint32 i = 0; i < aConfiguration.mClipRegion.Length(); ++i) {
+      if (!aConfiguration.mClipRegion[i].IsEmpty())
+        childIsZeroClipped = PR_FALSE;
+    }

should be changed to this

+    for (PRUint32 i = 0; i < aConfiguration.mClipRegion.Length(); ++i) {
+      if (!aConfiguration.mClipRegion[i].IsEmpty()) {
+        childIsZeroClipped = PR_FALSE;
+        break;
+      }
+    }

I'll fix this in the next revision, or on landing.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build of my patch from comment #19:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bcf8cfdf1685/tryserver-macosx/firefox-4.0b6pre.en-US.mac.dmg

Comment 22

[Timothy Nikkel (:tnikkel)]

Thanks for taking this Steven, and sorry that you had to deal with this breakage at all. I had little hope of figuring this out without a mac.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    PRBool childIsZeroClipped = PR_TRUE;
+    for (PRUint32 i = 0; i < aConfiguration.mClipRegion.Length(); ++i) {
+      if (!aConfiguration.mClipRegion[i].IsEmpty())
+        childIsZeroClipped = PR_FALSE;
+    }
+    child->Show(!childIsZeroClipped);

This can just be child->Show(!aConfiguration.mClipRegion.IsEmpty());

+  nsChildView* child = static_cast<nsChildView*>(aConfiguration.mChild);
+  if (child->IsPluginView()) {

Just assert child->IsPluginView(). ConfigureChildren/ApplyConfiguration is only used on plugins.

I actually wonder whether we ever need to show the plugin NSView. If we left it permanently hidden, what would break? (I know it sounds scary; I'm not proposing we do that in this bug.)

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And yes, thanks for tracking this down.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

> Thanks for taking this Steven, and sorry that you had to deal with
> this breakage at all. I had little hope of figuring this out without
> a mac.

> And yes, thanks for tracking this down.

No problem.  It's not surprising there's unexpected breakage after
making such a deep change as getting rid of all subframe widgets.

> I actually wonder whether we ever need to show the plugin NSView. If
> we left it permanently hidden, what would break?

One consequence would be that there would never be any calls to the
plugin widget's drawRect: method, so all calls (from the OS) to redraw
the plugin would actually be sent to the plugin widget's superview.
(This is in fact already happening, for other reasons (see bug 581608
comment #8), and I've seen some (so far minor) breakage from it.)

Other things would probably break, too ... though once again it's
difficult to predict exactly what.

It might be better to get rid of plugin widgets altogether.

Please, please don't make either of these changes in FF 4 :-)

Comment 26

[Steven Michaud [:smichaud] (Retired)]

On further thought, making the plugin NSView/ChildView always
invisible would probably be equivalent to getting rid of plugin
widgets -- I suspect the OS would send *no* events to the plugin
ChildView (all would go to its superview).

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (add Roc's changes)

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #25)
> Please, please don't make either of these changes in FF 4 :-)

Yeah, we don't want to do that for FF4.

We should do it, though. It would let us simplify quite a lot of code. In particular it would mean that on all platforms we only need to paint into one widget per toplevel browser window, a widget that covers the whole window.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> In particular it would mean that on all platforms we only need to
> paint into one widget per toplevel browser window, a widget that
> covers the whole window.

I agree this is a worthy goal -- it'll make it impossible for the OS
to focus the "wrong" ChildView.

I'm also glad the subframe widgets have disappeared -- that makes it
much less likely (in principle) that the OS will focus the "wrong"
ChildView.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I checked this in (anticipating josh's review) but backed it out because it seemed to cause failures in test_cocoa_focus.html:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283746712.1283747587.6577.gz

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That test seemed to pass on OSX, but fail on OSX-64.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backing out the patch fixed the regression.

Comment 33

[Steven Michaud [:smichaud] (Retired)]

> I checked this in (anticipating josh's review) but backed it out
> because it seemed to cause failures in test_cocoa_focus.html:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283746712.1283747587.6577.gz

One of those horrible "test timed out" failures :-(

I'll investigate further -- starting by trying to reproduce this
failure locally.

But there may be something wrong with the testbed, or with the test
itself.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (work around test failure)

I *am* able to reproduce this failure locally, and the timeout *is*
due to a bug in the test harness (it hangs when a plugin's npruntime
method returns 'false' -- at least on 64-bit OS X).

But underlying the timeout there is also a geniune test failure -- on
64-bit OS X, with my current patch, JavaScript [pluginobject].focus()
never focuses a plugin.

I've found that I can stop this happening by reversing my patch for
bug 504450.  Which means that (for some reason) calling
[pluginobject].focus() in 64-bit OS X builds always (or usually) tries
to focus a 'hidden' plugin.

I've no idea why this happens, or why it doesn't happen in 32-bit OS X
builds.  And frankly I don't have the time to find out.  But I suspect
that, now that subframe widgets are gone, reversing my patch for bug
504450 is much less likely to cause trouble (by causing the "wrong"
NSView/ChildView to be focused).  Though if we're going to take my new
patch we should land it as soon as possible -- to expose it to as much
user-level testing as possible.

New tryserver builds (32-bit and 64-bit) should follow in a few hours.

I'll also run all the automated tests locally, on both 32-bit and
64-bit builds.  I'll post my results once I have them.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

> I've no idea why this happens, or why it doesn't happen in 32-bit OS
> X builds.

Actually, this may also happen in 32-bit builds --
test_cocoa_focus.html's tests only run on 64-bit builds.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 472484 [details] [diff] [review]
Fix rev4 (work around test failure)

Looks good to me if it passes tests. Thanks!

Comment 37

[Steven Michaud [:smichaud] (Retired)]

> New tryserver builds (32-bit and 64-bit) should follow in a few hours.
>
> I'll also run all the automated tests locally, on both 32-bit and
> 64-bit builds.  I'll post my results once I have them.

I'm still working on the local tests.  But the tryserver tests all
passed (the first time I've seen that in quite a while!).

Here are the 32-bit and 64-bit opt builds:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-81cbc8f3c688/tryserver-macosx/firefox-4.0b6pre.en-US.mac.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-81cbc8f3c688/tryserver-macosx64/firefox-4.0b6pre.en-US.mac64.dmg

Comment 38

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 472484 [details] [diff] [review]
Fix rev4 (work around test failure)

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/82ac2ec4836d

Comment 39

[Marcia Knous [:marcia]]

So far I have not been able to reproduce the issues I was seeing using the tryserver build in Comment 37 on 10.5. I will try 10.6 as well. Thank you Steven for your fine effort on this bug.

Comment 40

[Marcia Knous [:marcia]]

Have been using  Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b6pre) Gecko/20100908 Firefox/4.0b6pre regularly and have not seen the problem reappear. I will verify once I have tested on x64 as well.

Comment 42

[Mike Beltzner [:beltzner, not reading bugmail]]

This needs to land on mozilla-central GECKO20b5_20100831_RELBRANCH

Comment 43

[Johnny Stenback (:jst)]

Backout landed on GECKO20b5_20100831_RELBRANCH

http://hg.mozilla.org/mozilla-central/rev/4a732aec89a5

Comment 44

[juan becerra [:juanb]]

I've tested around this for the issues described in comment #19. I was able to reproduce bug 592691 (very easy to reproduce) and bug 592453 in beta 5, and I haven't been able to reproduce the problem on beta 6 (build 2).