The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Plug-ins
--
major
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

({regression, relnote})

Trunk
All
Mac OS X
regression, relnote
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [4b5])

Attachments

(1 attachment, 4 obsolete attachments)

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.
(Assignee)

Comment 1

7 years ago
This should probably block the next beta.
blocking2.0: --- → ?
(Assignee)

Comment 2

7 years ago
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.
(Assignee)

Comment 3

7 years ago
hg bisect tells me:

The first bad revision is:
changeset:   51636:7804b5cf4313
user:        Robert O'Callahan <roc@ocallahan.org>
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.
I'm wondering if Marcia is seeing this in bug 592332.
(Assignee)

Comment 5

7 years ago
What I report at bug 581608 comment #8 may be related.
(Assignee)

Comment 6

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

Sounds likely.
(Assignee)

Updated

7 years ago
Keywords: regression
(Assignee)

Comment 7

7 years ago
> 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.

Updated

7 years ago
blocking2.0: ? → betaN+
(Assignee)

Comment 8

7 years ago
I've found another, related bug with the same regression range -- bug 592691.
(Assignee)

Comment 9

7 years ago
Another related bug -- bug 592453.

Updated

7 years ago
Whiteboard: [4b5]
Steven: I am assuming Bug 592332 is the same as this one?
> Steven: I am assuming Bug 592332 is the same as this one?

Probably.
(Assignee)

Updated

7 years ago
Assignee: nobody → smichaud
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Created attachment 472014 [details] [diff] [review]
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.)
Attachment #472014 - Flags: review?(roc)
Steven: Can we get a tryserver build to test :)?
> 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 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.
Attachment #472014 - Flags: review?(roc)
Created attachment 472084 [details] [diff] [review]
Fix rev1 (fix dumb mistake)

Tryserver builds will follow sometime tomorrow.
Attachment #472014 - Attachment is obsolete: true
Attachment #472084 - Flags: review?(roc)
(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?
> 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").
Created attachment 472185 [details] [diff] [review]
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.
Attachment #472084 - Attachment is obsolete: true
Attachment #472185 - Flags: review?(roc)
Attachment #472084 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Attachment #472185 - Flags: review?(joshmoz)
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.
Here's a tryserver build of my patch from comment #19:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-bcf8cfdf1685/tryserver-macosx/firefox-4.0b6pre.en-US.mac.dmg
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.
+    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.)
And yes, thanks for tracking this down.
> 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 :-)
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).
Created attachment 472270 [details] [diff] [review]
Fix rev3 (add Roc's changes)
Attachment #472185 - Attachment is obsolete: true
Attachment #472270 - Flags: review?(roc)
Attachment #472185 - Flags: review?(roc)
Attachment #472185 - Flags: review?(joshmoz)
(Assignee)

Updated

7 years ago
Attachment #472270 - Flags: review?(joshmoz)
Attachment #472270 - Flags: review?(roc) → review+
blocking2.0: betaN+ → beta6+
(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.
> 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.
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
That test seemed to pass on OSX, but fail on OSX-64.
Backing out the patch fixed the regression.
> 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.
Created attachment 472484 [details] [diff] [review]
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.
Attachment #472270 - Attachment is obsolete: true
Attachment #472484 - Flags: review?(roc)
Attachment #472270 - Flags: review?(joshmoz)
(Assignee)

Updated

7 years ago
Attachment #472484 - Flags: review?(joshmoz)
> 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.
(Assignee)

Updated

7 years ago
Attachment #472484 - Attachment description: Fix rev4 (work around 64-bit test failure) → Fix rev4 (work around test failure)

Updated

7 years ago
OS: Mac OS X → Windows 7
(Assignee)

Updated

7 years ago
OS: Windows 7 → Mac OS X
Comment on attachment 472484 [details] [diff] [review]
Fix rev4 (work around test failure)

Looks good to me if it passes tests. Thanks!
Attachment #472484 - Flags: review?(roc) → review+
> 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/smichaud@pobox.com-81cbc8f3c688/tryserver-macosx/firefox-4.0b6pre.en-US.mac.dmg
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/smichaud@pobox.com-81cbc8f3c688/tryserver-macosx64/firefox-4.0b6pre.en-US.mac64.dmg

Updated

7 years ago
Keywords: relnote

Updated

7 years ago
Attachment #472484 - Flags: review?(joshmoz) → review+
Comment on attachment 472484 [details] [diff] [review]
Fix rev4 (work around test failure)

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

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 591805
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.
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.
Duplicate of this bug: 592332
This needs to land on mozilla-central GECKO20b5_20100831_RELBRANCH
Backout landed on GECKO20b5_20100831_RELBRANCH

http://hg.mozilla.org/mozilla-central/rev/4a732aec89a5
Blocks: 592453
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).
Duplicate of this bug: 595699
You need to log in before you can comment on or make changes to this bug.