118789 - [viewpoint.] right mouse click handled by windowless plugin still causes popup context window to pop up

Status: RESOLVED FIXED

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

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    .9.4 branch

When you right-click with the plugin window, we handle the event and return 
true.  This should signal the browser that we have handled the event and that 
it should not.  Instead, the browser handles it anyway.
 

Reproducible: Always
Steps to Reproduce:
1.Install Viewpoint plugin (http://cole.viewpoint.com/~aberger/readme.txt)
2.View above content.
3.Right-click within plugin window.

Actual Results:  Context menu pops up.  It shouldn't.

I have found half the cause to this problem, but not the whole thing.

1) Macro problem:
In ns4xPluginInstance.cpp,  HandleEvent:
      NS_TRY_SAFE_CALL_RETURN(res, CallNPP_HandleEventProc(fCallbacks->event,
                                    &fNPP,
                                    (void*)&npEvent), fLibrary);

This is sticking the results of the function call into the variable named "res".

The definition of the macro is:
#define NS_TRY_SAFE_CALL_RETURN(ret, fun, library) \
PR_BEGIN_MACRO                                     \
  nsresult res;                                    \
  PRBool dontdoit = PR_FALSE;                      \
  nsCOMPtr<nsIPref> prefs(do_GetService(kPrefServiceCID, &res));\
  if(NS_SUCCEEDED(res) && (prefs != nsnull))       \
  {                                                \
    res = prefs->GetBoolPref("plugin.dont_try_safe_calls", &dontdoit);\
    if(NS_FAILED(res))                             \
      dontdoit = FALSE;                            \
  }                                                \
  if(dontdoit)                                     \
    ret = fun;                                     \
  else                                             \
  {                                                \
    try                                            \
    {                                              \
      ret = fun;                                   \
    }                                              \
    catch(...)                                     \
    {                                              \
      nsCOMPtr<nsIPluginHost> host(do_GetService(kCPluginManagerCID, &res));\
      if(NS_SUCCEEDED(res) && (host != nsnull))    \
        host->HandleBadPlugin(library);            \
      ret = (NPError)NS_ERROR_FAILURE;             \
    }                                              \
  }                                                \
PR_END_MACRO

Note how all instances of "ret" in the macro will be replaced with "res".  
Unfortunately, there is also a variable in the macro's {} scope named res.  The 
macro will not funcion properly and you will lose the return code.

This means that this function NEVER for ANY EVENT gets back the return code.  
Changing the variable name in the function is an easy fix (I tried and it now 
gets the return code), but is probably not the best fix.  I'd change the name 
of the variable in the macro to something less likely to clash.

2)
Even when I fix problem 1, the bug still shows- it definitely sends back that 
the event has been handled and returns that up the stack, but I still see the 
context menu.

I think this could be related to the mouse flickering problem that I spoke to 
Peter about, but I'm not sure.  I've made some fixes to our mouse code and I'll 
look at that now.

Comment 1

[shrirang khanzode]

dup of bug 75456 ? or that should be duped with this ?

Comment 2

[Ari Berger]

You are right- sorry, didn't see that one.  This is pretty important for us, as 
the right mouse button is used extensively.

Comment 3

[shrirang khanzode]

*** Bug 75456 has been marked as a duplicate of this bug. ***

Comment 4

[Peter Lubczynski]

This may not be a compete dup of bug 75456 because that is with all Mac plugins
and this is only with windowless. However, it may also depend on bug 103696.

Comment 5

[Peter Lubczynski]

Looking at this in the debugger last night, this totally depends on bug 103696.
We do get the CONTEXTMENU event in the object frame but consuming them doesn't
do what it should. That bug needs to be fixed before this one.

To reproduce:
For a testcase, use the windowless sdk sample:
http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/
First run nmake in "sdk".
Load up test.html and set a breakpoint in nsObjectFrame::HandleEvent when a
CONTEXTMENU comes through. Consume it, but notice the browser's context menu
still shows up.

Comment 6

[Ari Berger]

Do you disagree that the macro still needs to be fixed?

Comment 7

[Peter Lubczynski]

I'm not sure about the macro, but it does look like it needs to be fixed.

I talked with Kevin and it looks like we do indeed need to implement the
nsIDOMContextMenuListener interface in order to consume the event while in the
DOM phase. Frames get lower priority than then DOM when processing events which
is probably why we can't consume it.

The only problem with that is that I lost mouse move and click events when all I
did was inherit and implement the QI. I'll take another look.

However, looking through the docshell, it looks like we special case some html
tags here:
http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1469
I'll see if I can hack something together.

Comment 8

[Peter Lubczynski]

Okay, I got it to work by giving nsIDOMContextMenuListener it's own class. Then
it doesn't seem to interfear with other mouse evnets but it consumes the event.

I'm formulating a patch right now. It should also fix the Mac and dup bug 103696.

Comment 9

[Peter Lubczynski]

*** Bug 103696 has been marked as a duplicate of this bug. ***

Comment 10

[Peter Lubczynski]

Attachment: implementing nsPluginDOMContextMenuListener

Here's a patch that implements a seperate class just for consuming DOM context
menu events.

Please test and review.

Comment 11

[av (gone)]

Comment on attachment 64601 [details] [diff] [review]
implementing nsPluginDOMContextMenuListener 

r=av on the menu listener. Do we also want to fix name conflict in the macro
Ari mentioned in the original report?

Comment 12

[Ari Berger]

I will try out the patch today.
Once again, without fixing the macro, I believe that even when we return 1 from 
NPP_HandleEvent, you get the return value as 0.  Note that we ALWAYS return 1 
from HandleEvent.  Put a breakpoint in your code after that call.  Do you get 1 
or 0 as the return value?

Comment 13

[Ari Berger]

This fix looks good.

Comment 14

[Peter Lubczynski]

Attachment: updated previous patch to include fix for NPP_HandelEvent

This should patch should also fix the return code but I'm not sure this will
even give the plugin the ability to consume events. Many of the events (like
context menu), we just consume by default.

Andrei/Patrick, can I get reviews?

Comment 15

[av (gone)]

Comment on attachment 64817 [details] [diff] [review]
updated previous patch to include fix for NPP_HandelEvent

This is OK, r=av. In the future we should probably use some less usual name for
the |res| variable in the macro itself to ensure it will not conflict with the
calling code. How about |rezultat|?

Comment 16

[Judson Valeski]

we'd like to get this into 0.9.4 by midnight tomorrow; if it's ready.

Comment 17

[Patrick C. Beard]

Comment on attachment 64817 [details] [diff] [review]
updated previous patch to include fix for NPP_HandelEvent

Any reason that mCXMenuListener couldn't be an nsCOMPtr?
sr=beard

Comment 18

[Peter Lubczynski]

Using mCXMenuListener as a nsCOMPtr, the compiler didn't like my Init/Destroy
methods and I wasn't sure what should be done with |new|. Since in doubt, I used
the old-fasion way.

Comment 19

[Judson Valeski]

can this hit 0.9.4 by midnight?

Comment 20

[Peter Lubczynski]

patch in trunk, 0.9.4 landing shortly

Comment 21

[Peter Lubczynski]

fixed0.9.4

Comment 22

[shrirang khanzode]

verified fixed on 0.9.4 (0115 br build). Also fixed on mac/win trunk 0116. 
Browser context menu no longer appears inside plugin area.

Comment 23

[Peter Lubczynski]

Reopening because this is NOT fixed in MFCEmbed. :(

I think this time it IS:
http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTreeOwner.cpp#1469

I'll take a closer look.

Comment 24

[Peter Lubczynski]

Attachment: patch for embedding

Here's a patch, effecting embedding only, that will always consume context menu
events for applet, embed, and object tags. Please review.

One problem is that embedders won't be able to override the context menu for
the object tag in the case that it is used for images or documents. That's
probably not that big a deal considering that in order to do this correctly,
we'd have to fetch the coresponding layout frame for this content to query its
type PLUS embedders' code would need to look for a "data" attribute instead of
just "src".

Comment 25

[av (gone)]

Comment on attachment 65944 [details] [diff] [review]
patch for embedding

r=av

Comment 26

[Michael Dunn]

Please see:
http://bugscape.mcom.com/show_bug.cgi?id=11811

Comment 27

[Alec Flett]

Comment on attachment 65944 [details] [diff] [review]
patch for embedding

sr=alecf

Comment 28

[saari (gone)]

Do we really want to always consume the events over plugins, or only when the
plugin has handled the event itself?

Comment 29

[Peter Lubczynski]

Plugins don't know anything about context menu events so their isn't any return
to check. They usually use left/right down/up events to fire their menus but the
browser doesn't know that.

Jud, is the 0.9.4 branch open? Can you '+'?

Comment 30

[shrirang khanzode]

qa->bmartin since this is MFCembed only.

Comment 31

[Judson Valeski]

please checkin to 0.9.4 and add "fixed0.9.4" to the keyword field once it's in
there; thanks!

Comment 32

[Peter Lubczynski]

checked in on the trunk and fixed0.9.4