90574 - Windowless plugins do not receive mouse/keyboard events

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Kevin Tieskoetter]

From Bugzilla Helper:
BuildID:    20010627

If I go to a page with an embedded plugin (this happens with our Viewpoint 
Media Player plugin; I haven't seen it with others yet), and click on the embed 
area, Mozilla generates an assert in nsHTMLExternalObjSH::GetPluginInstance(), 
in the following line:

CallQueryInterface(frame, &objectFrame);
NS_ENSURE_TRUE(objectFrame, NS_ERROR_UNEXPECTED);

Apparently the nsIFrame is expected to have an nsIObjectFrame, but it doesn't.

I have enclosed a version of the plugin that has most of the functionality 
stripped out of it to make it easier to find the problem. 

Reproducible: Always
Steps to Reproduce:
1.Put the enclosed plugin in the plugins directory
2.Go to this URL: http://samples.viewpoint.com/jewelry/Razolith/index.html
3.The page will load up, but you won't see any viewpoint content - that's ok 
because i stripped most everything out of the plugin. Just click on the tan 
rectangle in the middle of the window.
4. Mozilla should bring up an assert

Comment 1

[Kevin Tieskoetter]

Attachment: Sample plugin

Comment 2

[av (gone)]

Could you please double check the url? I get 'Not Found' error.

Comment 3

[Kevin Tieskoetter]

fixed the url

Comment 4

[Peter Lubczynski]

Kevin, I can't get the sample plugin you attached to work in either Nav 4 or
Mozilla. I think Bugzilla ate it. Could you attach again, perhaps zipped?

Also, what is your specific problem or bug? The ASSERT by itself I don't think
is a bug. It may even be out of date. I think it was added with jst XPCDOM landing.

------
Actually, looking at the comment for the method, it looks like this helps
javascript call into the embed tag. I'd be good to know if there is a failure here:
3838 // HTMLObject/EmbedElement helper
3839 
3840 // This resolve hook makes embed.nsIFoo work as if
3841 // QueryInterface(Components.interfaces.nsIFoo) was called on the
3842 // plugin instance, the result of calling QI, assuming it's
3843 // successful, will be defined on the embed element as a nsIFoo
3844 // property.

Comment 5

[av (gone)]

The attached plugin is a zip file. Just rename it after 'Save Link As' to 
something.zip.

Comment 6

[Peter Lubczynski]

Ahh, thanks Andrei. It looks like it's an INLINE frame instead of an OBJECT one.
I recall we do some kind of frame substitution in nsObjectFrame::Reflow() that
may account for this. Strange, as looking around the stack, It seems to be a
NS_MENU_EVENT_START event which triggers the Javascript to go down this path.
Funny thing is that I didn't click, I just hovered over the plugin. Is this even
a problem? How does this block viewpoint?

The markup also looks interesting.
1) You have a |source=| instead of |src=| on the embed tag. I think plugin code
specifically looks for |src=| but I'm not sure and haven't dug into this.
2) Bug 90152 may effect this as it looks like the same sort of like
<object><embed></object>

Comment 7

[Kevin Tieskoetter]

To address a couple of questions:  First, we use "source=" instead of "src=" 
because we don't want Netscape starting the download immediately; we want to 
start it ourselves later on.  Since we don't have the 'src' tag, we rely on the 
MIME type tag to tell Netscape to activate us.

Second, about Bug 91052, I don't think this is the same problem, since we don't 
have any problem with rendering (this sample plugin doesn't render since I took 
all that code out).

Third, as Peter noticed, the assert fires when the cursor hovers over the 
plugin.  The reason this is a blocker is that we never get any mouse click if 
you do click on the plugin.  Maybe the assert has nothing to do with this fact, 
they just seemed tied together.

Comment 8

[Peter Lubczynski]

Kevin,

Could you further expalin, in more detail, how this bug is blocking you,
specially the part about not getting key events. I'm trying to find out the
"root" bug here as an ASSERT is not a bug, but probably will help in tracking
down the bug. Also, repharsing the summary to be more descriptive about problem
in general will help us triage it better.

Also, have you tried removing the ASSERT or trying different markup? First try a
simple testcase, with only an EMBED tag. Our code specifically looks for an SRC
attribute so that could effect it. Try it both ways and if you see anything,
please attach the testcase.

Comment 9

[Kevin Tieskoetter]

Attachment: Sample windowless plugin for reproducing problem

Comment 10

[Kevin Tieskoetter]

OK, I think I was incorrect when I began describing the problem. I don't know 
if the assert has anything to do with the problem, but the crux of the issue is 
that windowless plugins do not receive mouse or keyboard events.  It's my 
understanding that windowless plugins are supposed to receive these events 
through HandleEvent() (if I'm wrong here, please let me know, but this is how 
we do it for NS 4.x).

Attached is the windowless plugin that was provided by David Brittain for bug 
#44322.  If you put a breakpoint in HandleEvent(), it won't get called when you 
click on the plugin.

Project files, a test .htm file, and a compiled dll are included in the .zip 
file attached.

Comment 11

[Peter Lubczynski]

Ahh...well that's a whole lot better summary.

And thinking about the problem, I think I know what's up. On Win32, all events
are sent to the plugin through child windows. Since a windowless plugin does not
have a child window, no events get sent.

Windowless plugins probably need to hook into the DOMEvent stuff that Mac uses
(as it doesn't have child windows either). The code also probably need to be
tuned to work on Windows instead of Mac.

As a hack, try to change the #ifndef code around
nsPluginInstanceOwner::DispatchMouseEvent and DisptachKeyEvent in
nsObjectFrame.cpp. See if your events get through then.

Comment 12

[Kevin Tieskoetter]

Changing the #ifdef in DispatchMouseEvent does improve things a little bit, but 
there are still some peculiarities:  I don't get ButtonDown events, but I get 
ButtonUp events, and I only get a MouseMove event when the cursor enters the 
plugin area, but not as it moves around inside the area (meaning if I move the 
cursor over the plugin, I get one mouse-move event only unless I repeatably 
move the mouse outside the embed area and back in again).

Comment 13

[Peter Lubczynski]

Right, this code was written for the Mac, so it probably needs to be tweeked for
windowless plugins.

Comment 14

[Peter Lubczynski]

Kevin,

Now that the fires have died down a bit, I've taken a closer look at windowless
plugin events:

I think if you repeat the hack you did for DispatchMouseEvent in
nsPluginInstanceOwner::MouseDown, you may get your MouseDown events to reach the
plugin. Let me know if that works for you as probably the same needs to be done
for key and focus events.

I'll try to figure out why MouseMove only fires once. Perhaps there is a flag
that must be set.

Comment 15

[Peter Lubczynski]

Attachment: first try at fixing this bug, patch v.1

Comment 16

[Peter Lubczynski]

Kevin, PLEASE try my patch and let me know if it makes things better.

Comment 17

[Peter Lubczynski]

Attachment: better patch (please ignore previous) v.2

Comment 18

[av (gone)]

Peter, you removed #ifndef XP_WIN. Should not we still ifdef it out for regular 
plugins on Windows and do this for windowless plugins only?

Comment 19

[Peter Lubczynski]

I'm still trying to find out the correct thing to do here, but I think you may
be right. As with the case with keyevents, we crash in Win32.

Comment 20

[Peter Lubczynski]

Well, bug 93056 talks about SetWindow having incorrect coordinates while
scolling for windowless plugins. That may be kind of difficult to fix as I
recall it was hard to fix on the Mac. 

Comment 21

[chris hofmann]

is this still something we might want to fix on the 0.9.2 branch for the
embedding customer in the next couple of weeks?

Comment 22

[Arun Ranganathan]

This is still something we ought to fix, and I'd like to lobby for a topembed+ .
 Kevin Tieskoetter, please jump in and comment here about how/why this is a
blocker for you.

Comment 23

[Peter Lubczynski]

PDT Status: Waiting for Kevin Tieskoetter to review and test attachment 44056 [details] [diff] [review].

Comment 24

[Kevin Tieskoetter]

I've been trying out the fix that Peter provided, but I'm getting a very 
strange problem.  It appears that, after adding the patch, my plugin is no 
longer getting any arguments (all the extra arguments in the object embed). Is 
there any way this patch could have caused this, or am I going completely mad?

In tracing back into the Mozilla code, it seems now that in 
nsPluginInstancePeerImpl::GetAttributes(), when mOwner is queried for 
nsIPluginTagInfo, it returns NULL, so no attributes are attainable.

I tried out the code just before applying the patch, and everything seemed to 
work OK...

On the bright side, my plugin does seem to be getting mouse events now... :-)

Comment 25

[Peter Lubczynski]

Oh...I'm an idiot.....I forgot the nsIPluginTagInfo interface in my
NS_IMPL_ISUPPORTS macro. It should read:

+NS_IMPL_ISUPPORTS10(nsPluginInstanceOwner,
+                    nsIPluginInstanceOwner,
+                    nsIPluginTagInfo,
+                    nsIPluginTagInfo2,
+                    nsIJVMPluginTagInfo,
+                    nsIEventListener,
+                    nsITimerCallback,
+                    nsIDOMMouseListener,
+                    nsIDOMMouseMotionListener,
+                    nsIDOMKeyListener,
+                    nsIDOMFocusListener)

Kevin, if you make that change, does it work for you then? Does that completely
fix this bug for you or are there other issues? 

If you are satisfied, I'll make a patch for approvals and reviess. I still need
to ensure we only do this in the windowless or Mac cases.

Comment 26

[Peter Lubczynski]

Attachment: cleaned up patch with windowless checks v.3

Comment 27

[av (gone)]

I like the last one. r=av

Comment 28

[Kevin Tieskoetter]

Cool - the fix seems to work fine now.  However, it then uncovered another bug 
that I didn't notice before, in that the window coordinates passed to 
NPP_SetWindow() are incorrect.  I logged that as Bug #93500.

Thanks for helping me out on this!  :-)

Comment 29

[Kevin Tieskoetter]

To answer Arun's question, this bug is a blocker for us, since it prevents us 
(or any windowless plugin) from receiving any mouse or keyboard events.

Comment 30

[Peter Lubczynski]

Kevin,

I think your new bug may be a dup of bug 93056. Can you try THIS patch (by David
Brittain) and see if it fixes the coordinate problem:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=44370

If you are satisfied with this patch, I will get a super review, approvals and
check it in.

Comment 31

[Peter Lubczynski]

Attachment: updated patch (removing some accidental garbage in other patch) v.4

Comment 32

[Chris Waterson]

sr=waterson

Comment 33

[Peter Lubczynski]

Fix on trunk, do I need a plus to check in on the branch?

Comment 34

[chris hofmann]

ok, check in on the branch.  thanks

Comment 35

[Peter Lubczynski]

Chofmann, something got screwed up in bugzilla. This bug auto-morphed!
attempting to change it back. (I assume the + was correct)

Comment 36

[Peter Lubczynski]

Checked into the MOZILLA_0_9_2_BRANCH, marking FIXED.

Could someone see about getting a '+' for bug 93056?

Comment 37

[Chris Petersen]

Changing QA to [e-mail].

Comment 38

[shrirang khanzode]

verif that no assertions come up on clicking/typing inside the windowless
plugin. verif on build 1120 trunk win.