627649 - [Mac] Some events incorrectly passed to a plugin that occur over the plugin

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[uTest]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b9) Gecko/20100101 Firefox/4.0b9
Build Identifier: Firefox 4.0b9

Go to http;//youtube.com
enter "Geor" in the search box
the drop down box appears with "George Michale Faith"
Tried to click it. Wouldn't click. 

Reproducible: Always

Steps to Reproduce:
1. Enter "Geor" in the search box
2. See "George Michael Faith" in drop Down box
3. Try to click that link
Actual Results:  
it is invisible you click anything below.  Not going to video George Michael Faith.   

Expected Results:  
Should click and go to video linked

http://screencast.com/t/pbR1Q1wtF

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, definitely happening.  Looks like we end up targeting whatever is below the dropdown with the click.

Regression from 3.6.

A regression window would be really useful here.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

OSX only? Can't reproduce on Linux. I'll try on OSX.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Does the problem happen only when there is a video (Flash) loaded in the page before the search?

When I load http://youtube.com, there are just preview images of some
videos, no actual video.

Comment 4

[Boris Zbarsky [:bzbarsky]]

No, I get this even with the images: I click on the list entry and the image's click handler is triggered, replacing itself with a video, etc.

And yes, I'm on OSX.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Can't reproduce on OSX either :/
Is youtube perhaps giving me different content.

Comment 6

[Boris Zbarsky [:bzbarsky]]

It's possible, yes....  :(

Comment 7

[Justin]

I was able to reproduce on OSX this morning

Comment 8

[Justin]

Now that the ad is gone, it is difficult to reproduce. It looks like it occurs only when there is an ad with a playable video that is covered by the suggested search results.

Comment 9

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

Boris, can you you still reproduce? If so, please take the bug and we'll block on it. If you can't reproduce it anymore, then we probably won't block on it.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, I can still reproduce this.

The regression range I see is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=edf41ff32f08&tochange=a42e9b001bc8

My money is on bug 601182; trying to confirm now.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Olli, were you testing a nightly, or a self-build?  If you were using a 64-bit-only self-build (i.e. not 32-bit and not a build with a 32-bit plugin-container), you wouldn't see the issue, since you would have no Flash then.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

I was testing self-build on 32bit osx, and on 32 bit linux.

Comment 13

[Boris Zbarsky [:bzbarsky]]

OK.  For what it's worth, my 32-bit self-build on OSX shows the problem, while the 64-bit does not (no Flash).  So you not seeing it could still be a difference in the content youtube serves....

Comment 14

[Boris Zbarsky [:bzbarsky]]

hg bisect says:

changeset:   57526:1950375b2ec2
user:        Steven Michaud <[e-mail]>
date:        Mon Nov 15 15:12:50 2010 -0600
summary:     Bug 601182 - Spurious focus events sent to NPAPI plugins on OS X in Cocoa event mode. Patch partly by enndeakin. r=josh,smaug a=blocking2.0BetaN+

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Boris:  How, exactly, do you reproduce this bug?

(I haven't tried yet.  I just want to save myself some trouble.)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Steven, I just use the steps in comment 0 and click any of the entries near the bottom of the dropdown (the ones that are over the ad).

Comment 17

[Boris Zbarsky [:bzbarsky]]

And I can confirm that when I do that I get a FocusPlugin call for the plug-in that's below the dropdown, with this stack:

#0  nsFocusManager::FocusPlugin (this=0x5128880, aContent=0x1ec8cc60) at /Users/bzbarsky/mozilla/vanilla/mozilla/dom/base/nsFocusManager.cpp:1003
#1  0x004388ae in nsObjectFrame::HandleEvent (this=0x6785da8, aPresContext=0x6766000, anEvent=0xbfffd668, anEventStatus=0xbfffd1cc) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/generic/nsObjectFrame.cpp:2227
#2  0x003745c1 in nsPresShellEventCB::HandleEvent (this=0xbfffd290, aVisitor=@0xbfffd1c0) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:1466
#3  0x007a73c4 in nsEventTargetChainItem::HandleEventTargetChain (this=0x5a58a40, aVisitor=@0xbfffd1c0, aFlags=6, aCallback=0xbfffd290, aMayHaveNewListenerManagers=0, aPusher=0xbfffd1d8) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/events/src/nsEventDispatcher.cpp:386
#4  0x007a7eef in nsEventDispatcher::Dispatch (aTarget=0x1ec8cc60, aPresContext=0x6766000, aEvent=0xbfffd668, aDOMEvent=0x0, aEventStatus=0xbfffd428, aCallback=0xbfffd290, aTargets=0x0) at /Users/bzbarsky/mozilla/vanilla/mozilla/content/events/src/nsEventDispatcher.cpp:628
#5  0x0035eba1 in PresShell::HandleEventInternal (this=0x23057e60, aEvent=0xbfffd668, aView=0x1ec8d6d0, aStatus=0xbfffd428) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:6996
#6  0x0036f63b in PresShell::HandleEvent (this=0x23057e60, aView=0x1ec8d6d0, aEvent=0xbfffd668, aDontRetargetEvents=0, aEventStatus=0xbfffd428) at /Users/bzbarsky/mozilla/vanilla/mozilla/layout/base/nsPresShell.cpp:6735
#7  0x0098aad4 in nsViewManager::HandleEvent (this=0x1fbbeb20, aView=0x1ec8fd50, aEvent=0xbfffd668) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsViewManager.cpp:1095
#8  0x0098efe8 in nsViewManager::DispatchEvent (this=0x1fbbeb20, aEvent=0xbfffd668, aView=0x1ec8fd50, aStatus=0xbfffd57c) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsViewManager.cpp:1073
#9  0x0098811c in HandleEvent (aEvent=0xbfffd668) at /Users/bzbarsky/mozilla/vanilla/mozilla/view/src/nsView.cpp:161
#10 0x01429be4 in nsChildView::DispatchEvent (this=0x1ec8fdc0, event=0xbfffd668, aStatus=@0xbfffd69c) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:1797
#11 0x01428b29 in -[ChildView sendFocusEvent:] (self=0x1ec90da0, _cmd=0x1d30ed2, eventType=163) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:2549
#12 0x0142a3f2 in -[ChildView updateCocoaPluginFocusStatus:] (self=0x1ec90da0, _cmd=0x1d333d9, hasFocus=1 '\001') at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:5749
#13 0x0142dd3d in -[ChildView becomeFirstResponder] (self=0x1ec90da0, _cmd=0x991cdddc) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsChildView.mm:5761
#14 0x98a2fa8c in -[NSWindow makeFirstResponder:] ()
#15 0x98b4cb29 in -[NSWindow sendEvent:] ()
#16 0x014195e8 in -[ToolbarWindow sendEvent:] (self=0x51d92d0, _cmd=0x991c8a58, anEvent=0x1fbd9000) at /Users/bzbarsky/mozilla/vanilla/mozilla/widget/src/cocoa/nsCocoaWindow.mm:2439
#17 0x98a65817 in -[NSApplication sendEvent:] ()
#18 0x989f92a7 in -[NSApplication run] ()

In becomeFirstResponder mIsPluginView is true.  So for some reason the cocoa widgetry decides to send a focus event to the plug-in widget(?), which transfers focus manager focus to the plug-in, etc.  I bet that causes us to mistarget the click event....

Steven, could you or Josh take a look at this?  I don't understand the cocoa widgetry well enough to deal here, I suspect.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

Thanks, Boris, for the stack.

I just tried a 32-bit self-build on OS X 10.5.8 and was unable to reproduce -- either with Flash or with my DebugEventsPlugin from bug 441880.

Later I'll try in other configurations.

I'll definitely take a look at this -- probably tomorrow.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK. If you want to do some remote debugging, have me try logging patches, whatever, I'm happy to help out!

Comment 20

[Boris Zbarsky [:bzbarsky]]

Did some more digging.  The menu goes away when clicked, in the failing case, due to script that's handling an NS_BLUR_CONTENT event, with the currentTaret an HTMLInputElement.  This happens _after_ the FocusPlugin call (in fact that call is what causes the blur to happen).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase, probably not minimal

Comment 22

[Boris Zbarsky [:bzbarsky]]

Steven, can you reproduce the issue with the attached testcase?  For me it shows the problem every other click or so...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase with clearer directions

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> Steven, can you reproduce the issue with the attached testcase?

Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.

I can't on OS X 10.5.8, or on OS X 10.6.6 in 64-bit mode (the
default).

I tested with today's Minefield nightly and with my 32-bit self build
(made with code pulled this morning).

I'll try to make sense of this later, when I start digging into this
bug myself.

My 64-bit tests were with today's nightly -- which *can* load 32-bit
plugins (and of course only runs 64-bit on 10.6.X).

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> > Steven, can you reproduce the issue with the attached testcase?
> 
> Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.
> 
> I can't on OS X 10.5.8, or on OS X 10.6.6 in 64-bit mode (the
> default).

Ah, this could be the reason why I can't reproduce.
I have only 10.5.x

Comment 26

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #24)

>> Steven, can you reproduce the issue with the attached testcase?
>
> Yes, but only on OS X 10.6.X (10.6.6) in 32-bit mode.

Actually this is wrong -- I was too hasty.  Using Boris's testcase
from comment #23, I can reproduce this bug on OS X 10.5.8 ... on the
second try.  And in 64-bit mode on 10.6.6, I get other wierdness (a
new blank tab opens up), which turns out to be related.

Testing with a variant of Boris's testcase that loads my
DebugEventsPlugin from bug 441880, my results are much clearer:  Those
tests show that this bug exists "everywhere" on OS X, and that
variants of it existed on the trunk (in Cocoa event mode) before my
patch for bug 601182 landed, and even on the branches (in Carbon event
mode).

The basic problem is that we incorrectly pass some events to the
plugin that occur in a drop-down list above the plugin -- which can
include mouse up and down events and focus events.  This doesn't occur
with every kind of drop-down list.  But it does always occur with the
kind of drop-down list contained in Boris's testcase.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

This is really a Plugins bug ... or possibly a Cocoa widgets bug.

But I hesitate to reclassify it because doing that may cause it to lose its blocking flag.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> Testing with a variant of Boris's testcase that loads my
> DebugEventsPlugin from bug 441880, my results are much clearer:
> Those tests show that this bug exists "everywhere" on OS X, and that
> variants of it existed on the trunk (in Cocoa event mode) before my
> patch for bug 601182 landed, and even on the branches (in Carbon
> event mode).

This bug doesn't exist in Safari, Chrome or Opera.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

> But I hesitate to reclassify it because doing that may cause it to lose its
> blocking flag.

I did reclassify it, and the blocker flag is still there.

Comment 30

[Boris Zbarsky [:bzbarsky]]

The fact that the event delivery changes focus and hence causes the misbehavior observed on this testcase is still a regression from bug 601182, no?

Comment 31

[Steven Michaud [:smichaud] (Retired)]

I haven't yet thoroughly investigated this bug.  But I'm pretty sure
that the behavior changes caused by my patch for bug 601182 (and there
were some) will turn out to be minor ... though they might have made
this bug more visible.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

By the way, to really see what's going on here you do need to test with my DebugEventsPlugin from bug 441880.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Oh, the _plugin_ parts of this were not much changed by the patch in question.  But it caused a change in the focus behavior on web pages, which caused a change in what focus events fire when, which caused a change in what scripts run when, which is what broke the youtube search box.  At least as far as I can tell.

Comment 34

[Steven Michaud [:smichaud] (Retired)]

I'm still mostly preoccupied with bug 626016 and bug 629593.  But
Windows tryserver builds are so slow, and I'm having such horrendous
trouble getting usable Windows builds of my own, that I sometimes have
gaps of time in which I can work on this bug.

I *hope* I can fix this bug by making changes to Mac-specific plugin
code (and possibly also to Cocoa widgets code).  But if that doesn't
suffice, I may need your help figuring out what else is wrong here :-)

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I was right -- this bug is mostly Mac-specific.

The problem is that, as Boris' testcase shows, a DOM element can be
displayed over a plugin that *isn't* (technically) a popup (something
managed by the nsXULPopupManager).

Popups are displayed in their own NSWindow, and can overlay a plugin
without causing trouble.  But when the element from Boris' example (an
absolutely positioned table) overlays a plugin, clicking anywhere
above the plugin (even those parts obscured by the table) causes the
plugin's ChildView (a Cocoa object that's a subclass of NSView) to be
focused.  Or at least that's the case in current code.

My patch changes this behavior, and thereby (in Boris' testcase) stops
the plugin from receiving a focus event and mouse-down event when
clicking on "george lopez".

It also makes a cross-platform change to
nsPluginInstanceOwner::MouseUp() (in nsObjectFrame.cpp) to stop a
mouse-up event from being sent to the plugin (which won't have been
focused by the previous mouse-down event).  This part of my patch
should be correct on all platforms, but at this late date (so soon
before the FF4 release) we might want to play safe and ifdef it to
XP_MACOSX.

A tryserver build will follow in a few hours.

Comment 36

[Boris Zbarsky [:bzbarsky]]

The stopPropagation there looks wrong.  It'll confuse whatever mouse listeners the page has, because they will never see the event.  Why is that needed?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Also, the ElementFromPointHelper call seems to be passing device pixels to a function that I think expects CSS pixels.

And frame destruction should null out the content pointer on the widget, right?

Comment 38

[Steven Michaud [:smichaud] (Retired)]

> Also, the ElementFromPointHelper call seems to be passing device
> pixels to a function that I think expects CSS pixels.

nsIDocument::ElementFromPointHelper does in fact expect device pixels.
Which is a little odd, but I expect probably is explained by the fact
that it's often called from user-level JavaScript code.

> And frame destruction should null out the content pointer on the
> widget, right?

Yes.  My patch does this when the plugin widget is destroyed (in
[ChildView widgetDestroyed], on a call to nsIWidget::Destroy() from
nsObjectFrame.cpp).

> The stopPropagation there looks wrong.  It'll confuse whatever mouse
> listeners the page has, because they will never see the event.  Why
> is that needed?

I'm not completely sure it's needed, but swallowing the event sounds
right to me:  The DOM is in fact delivering the event to the correct
destination (to the plugin element, rather than to the table element
that's now disappeared).  But it's incorrect to deliver that event to
the plugin (which might be confused by it).

If we didn't swallow it, it might be delivered somewhere else, which I
don't think would be correct.

Comment 39

[Boris Zbarsky [:bzbarsky]]

> nsIDocument::ElementFromPointHelper does in fact expect device pixels.

What makes you say that?  It certainly looks to me like it expects CSS pixels.  Near the beginning of that function:

2825   nscoord x = nsPresContext::CSSPixelsToAppUnits(aX);
2826   nscoord y = nsPresContext::CSSPixelsToAppUnits(aY);

> that it's often called from user-level JavaScript code.

User-level JS code never has access to device pixels.

> on a call to nsIWidget::Destroy() from nsObjectFrame.cpp).

Ah, ok.  I thought that was being triggered from ~ChildView.  OK, this works.

> The DOM is in fact delivering the event to the correct destination

Yes.

> But it's incorrect to deliver that event to the plugin

Then we should block _that_, not block event delivery to everything in the DOM altogether.

> which I don't think would be correct.

It would be correct.  The mouseup is in fact happening.  It's happening over the <embed> or whatever.  That needs to fire a mouseup event.  If we have particular broken consumers who can't handle that, we should fix those consumers or keep them from seeing the event, not break the world.

Comment 40

[Steven Michaud [:smichaud] (Retired)]

>> nsIDocument::ElementFromPointHelper does in fact expect device
>> pixels.
>
> What makes you say that?  It certainly looks to me like it expects
> CSS pixels.  Near the beginning of that function:
>
> 2825   nscoord x = nsPresContext::CSSPixelsToAppUnits(aX);
> 2826   nscoord y = nsPresContext::CSSPixelsToAppUnits(aY);

Oops, you're right (I think).  New patch coming up.

I was confused by the fact that (in your testcase) device pixels are
the same size as CSS pixels.  Also, I usually only think about app
units and dev pixels :-(

>> But it's incorrect to deliver that event to the plugin
>
> Then we should block _that_, not block event delivery to everything
> in the DOM altogether.

You know a lot more about this stuff than I do, so I'm willing to
trust your judgement.

Should I get rid of the calls to PreventDefault() and
StopPropagation(), or just the call to StopPropagation()?

Comment 41

[Boris Zbarsky [:bzbarsky]]

It's probably fine to leave the preventDefault() in if that's enough to fix the issue here.  I don't expect that to break websites.  The stopPropagation() almost certainly would.

If we do run into problems with it, we can consider doing something in the plugin code, but that would probably be more risky.

Comment 42

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (follow Boris' suggestions)

Comment 43

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

This seems like a pretty bad layering violation, to have widget code that mucks with the DOM.

It seems to me a more logical structure would be for nsObjectFrame::HandleEvent to be responsible for focusing the plugin. If the user clicks on content over the plugin, nsObjectFrame::HandleEvent will not be called.

Comment 44

[Steven Michaud [:smichaud] (Retired)]

> This seems like a pretty bad layering violation, to have widget code
> that mucks with the DOM.

It's not mucking with the DOM (in the sense of changing it) -- it's
querying it.  We already have an nsQueryContentEvent that does
something similar.  Would you be more comfortable if this kind of
query was implemented using an nsQueryContentEvent (or something like
it)?

> It seems to me a more logical structure would be for
> nsObjectFrame::HandleEvent to be responsible for focusing the
> plugin. If the user clicks on content over the plugin,
> nsObjectFrame::HandleEvent will not be called.

Maybe once we get rid of plugin widgets (and have only one widget for
everything).  But for now focusing a plugin's NSView/ChildView will
have to focus the plugin itself.  And there will be times when we have
to stop a ChildView from gaining the keyboard focus in order to stop a
plugin from being focused.

Comment 45

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

At what stage of event processing is becomeFirstResponder called?

Comment 46

[Steven Michaud [:smichaud] (Retired)]

> At what stage of event processing is becomeFirstResponder called?

Very early -- before the NSLeftMouseDown event has even been sent to
[ChildView mouseDown:] (and of course before any NS_MOUSE_BUTTON_DOWN
event has been sent to Gecko).

Comment 47

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

I see. That makes things difficult.

What about the case where an ancestor of the plugin element installs a capturing event listener for "mouse down" and calls preventDefault in its event handler? I think that means the plugin should not take focus, but your patch (in fact, this whole approach) won't stop that.

Comment 48

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

Attachment: testcase

On Windows clicking on the input or the plugin won't give them focus. But I'm guessing with your patch, on Mac, the plugin would still get focus.

(There is a related bug on Windows, which is that calling event.preventDefault() but not event.stopPropagation() still lets the plugin take focus, because our plugin focus code is listening for DOM events, not the frame event handler.)

So, I wonder if there is any approach to controlling plugin focus on Mac that would let us fix this case as well.

Comment 49

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

E.g. what if we always return false from becomeFirstResponder for plugin views, so that plugin views are never the first responder? Does that actually break plugins? Gecko should continue delivering keyboard and other events to the plugin if we think it's focused.

If we do need to make the plugin first-responder, can we return false from becomeFirstResponder and then later make the plugin view first-responder when our normal mouse event code decides to focus the plugin?

Comment 50

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #48)

> So, I wonder if there is any approach to controlling plugin focus on
> Mac that would let us fix this case as well.

I will try to find out.

(In reply to comment #49)

> E.g. what if we always return false from becomeFirstResponder for
> plugin views, so that plugin views are never the first responder?
> Does that actually break plugins?

This would change a basic assumption of Cocoa widgets code, and would
break any number of things in nsChildView.mm (and possibly elsewhere).

> If we do need to make the plugin first-responder, can we return
> false from becomeFirstResponder and then later make the plugin view
> first-responder when our normal mouse event code decides to focus
> the plugin?

This might work.  I'll look into it.

By the way, my life is a bit complicated this week because I have to
deal with a bad bug in the JEP.  But I should have *some* time to work
on this bug.

Comment 51

[Josh Aas]

I'll try to help with this tomorrow but it's getting pretty late for Firefox 4, any patch here is going to be risky, and this isn't a hard blocker. Unless we can come up with a solution we're feeling pretty confident about by Monday we'll probably have to punt to the first update to Firefox 4.

Comment 52

[Josh Aas]

I think this needs to hard block. It is 100% reproducible, it occurs on the YouTube front page. It's a terrible user experience, at best a user's action will result in nothing and at worst it invokes an unintended action.

Comment 53

[Josh Aas]

I think we should try to take Steven's existing patch. I think roc is right about the remaining issues, and that we don't want to do this in the long run, but for right now I think Steven's current patch has the lowest risk for the highest reward.

If we do this, we should remember to file a bug about replacing this with a better fix after Firefox 4.

Comment 54

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

If we take this as a temporary hack, who is going to fix it for real and when?

Comment 55

[Steven Michaud [:smichaud] (Retired)]

>> If we do need to make the plugin first-responder, can we return
>> false from becomeFirstResponder and then later make the plugin view
>> first-responder when our normal mouse event code decides to focus
>> the plugin?
>
> This might work.  I'll look into it.

I'd like to pursue this when I have the time (post FF4).

Comment 56

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #55)

Of course the problem will go away when/if we get rid of plugin
widgets.  But doing that will be a lot of work (at least on the Mac).

Comment 57

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

In the current patch, it looks like we never clear mPluginContent. Seems like that could lead to use of a dangling pointer.

A safer way to do this might be to send a new kind of event, which includes the mouse position and the nsIWidget for the plugin. The event would return true if the mouse event at that position hits the nsObjectFrame whose widget is the given widget.

Comment 58

[Steven Michaud [:smichaud] (Retired)]

> In the current patch, it looks like we never clear mPluginContent.

Boris thought the same, but it's not true.  See comment #38.

Comment 59

[Steven Michaud [:smichaud] (Retired)]

> A safer way to do this might be to send a new kind of event, which
> includes the mouse position and the nsIWidget for the plugin. The
> event would return true if the mouse event at that position hits the
> nsObjectFrame whose widget is the given widget.

I should have time to do this tomorrow ... if you still think it's
necessary.

Comment 60

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

Ah OK. That's relying on mDestroyWidget always being true on Mac. If you can do the refactoring I suggested in comment #57 tomorrow, I think you should. Thanks!

Comment 61

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (follow Roc's suggestion)

Comment 62

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

+  nsIntPoint eventLoc =
+    aEvent->refPoint + aEvent->widget->WidgetToScreenOffset();
+  nsRect docFrameRect = docFrame->GetScreenRectInAppUnits();
+  eventLoc.x -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.x);
+  eventLoc.y -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.y);

You're mixing device pixels and CSS pixels here. I was actually hoping you'd go through PresShell::HandleEvent and use the mouse coordinate processing that happens there.

Comment 63

[Steven Michaud [:smichaud] (Retired)]

> I was actually hoping you'd go through PresShell::HandleEvent and
> use the mouse coordinate processing that happens there.

I'll look into it, but it'll probably take all day again.  These are
parts of the tree I'm not very familiar with.

Comment 64

[Steven Michaud [:smichaud] (Retired)]

> I'll look into it, but it'll probably take all day again.  These are
> parts of the tree I'm not very familiar with.

Actually, it might take me as long as a week.

I just looked at nsPresShell::HandleEvent(), and I can't figure out
where to intervene, or find any other event I might use as a model (as
I did with NS_QUERY_CHARACTER_AT_POINT for my rev2 patch).  So, if you
don't mind, I'm going to keep using the strategy of my rev2 patch.

>+  nsIntPoint eventLoc =
>+    aEvent->refPoint + aEvent->widget->WidgetToScreenOffset();
>+  nsRect docFrameRect = docFrame->GetScreenRectInAppUnits();
>+  eventLoc.x -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.x);
>+  eventLoc.y -= nsPresContext::AppUnitsToIntCSSPixels(docFrameRect.y);
>
> You're mixing device pixels and CSS pixels here.

As Boris pointed out in comment #39,
nsIDocument::ElementFromPointHelper expects CSS pixels.  I'll try to
rewrite my patch to use something else.

Comment 65

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (fix various problems)

> As Boris pointed out in comment #39,
> nsIDocument::ElementFromPointHelper expects CSS pixels.  I'll try to
> rewrite my patch to use something else.

This is much better.

Note that I also fixed a couple of other problems:  I replaced
GetScreenRectInAppUnits() with GetScreenRect() (which returns CSS
pixels).  And I made sure targetFrame is null-checked before use.

Comment 66

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 515945 [details] [diff] [review]
Fix rev3 (fix various problems)

Oops, stupid mistake.  Another patch coming up very shortly.

Comment 67

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (fix another problem)

Comment 68

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

Comment on attachment 515947 [details] [diff] [review]
Fix rev4 (fix another problem)

Because you're not using the HandleEvent path, this patch will fail in at least the following circumstances:
-- If the plugin is in an IFRAME, and the parent document contains another IFRAME over the plugin's IFRAME, ElementFromPoint will report that the mouse is over the plugin when it's actually not. We'll focus the plugin when we shouldn't. (This is not a regression for this patch though.)
-- If mouse capturing is happening, events might be directed to the plugin while the plugin thinks the events are not over the plugin. This shouldn't cause any problems though since it's only mouse-down events we care about and we shouldn't be holding capture while the button is up.

So I think this is worth taking as-is.

Comment 69

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 515947 [details] [diff] [review]
Fix rev4 (fix another problem)

Landed on the trunk:
http://hg.mozilla.org/mozilla-central/rev/75082063f8e6

Comment 70

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

(In reply to comment #55)
> >> If we do need to make the plugin first-responder, can we return
> >> false from becomeFirstResponder and then later make the plugin view
> >> first-responder when our normal mouse event code decides to focus
> >> the plugin?
> >
> > This might work.  I'll look into it.
> 
> I'd like to pursue this when I have the time (post FF4).

Do you have time yet? :-)

Comment 71

[Steven Michaud [:smichaud] (Retired)]

> Do you have time yet? :-)

I probably won't have time for at least as month -- what with work
I've got to do on the JEP (concerning a security bug and Lion
compatibility), and with further truly urgent FF4 bugs that are likely
to turn up without warning (like bug 629030 on the 1.9.2 branch).

It doesn't make sense to work on this until we're comfortably "post
FF4".  But I truly am interested in working on it, and I suspect
that once I get started, it might take me about a week.

Comment 72

[Steven Michaud [:smichaud] (Retired)]

> Do you have time yet? :-)

I do now ... or at least I seem to for the moment.

I've opened bug 658550 to follow up on your suggestion.