545149 - Fix various issues with winless plugin context menus

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Follow up bug for issues with context menu focus once the modal dialogs patch lands. (bug 538918) There's an issue with the system's context menu where if the parent window loses focus, the context doesn't hide. This doesn't appear to be related to deferred messaging as all messages are delivered through the deferred run, spin loop code in windowsmessageloop.cpp after the mouse down event.

Comment 1

[Tyler Downer [He/Him]]

Would bug 547434 be this as well?

Comment 3

[Benjamin Smedberg]

STR:
* Go to any video on hulu.com
* right-click the video to show the context menu
* click the Firefox window title bar

Expected: the context menu should dismiss
Actual: the context menu still shows

Tested with 3.7a2, I can try again with trunk nightlies if there have been relevant fixes landed since then.

Comment 5

[u88484]

Jim, just want to make sure that you seen in the bug you duped that the browser hangs and even sometimes crashes.

Comment 6

[Jim Mathies [:jimm]]

(In reply to comment #5)
> Jim, just want to make sure that you seen in the bug you duped that the browser
> hangs and even sometimes crashes.

Hmm, I was reading emil's comments. I guess we can reopen that. There is a bug open someplace on crashes on video sites with context menus show, although for the life of me I can't find it now.

Comment 7

[Ria Klaassen (not reading all bugmail)]

Also in Bug 552211, if I right click on the ad on top, the context-menu stays and CPU goes to 60%.

Comment 8

[Jim Mathies [:jimm]]

Attachment: popup fix

Hook TrackPopupMenu for flash and silverlight and hand in a surrogate hwnd so they don't fail / screw up focus. 

I've removed the old timer proc code because it's pretty much dead code. The changes we made to rpc channel after modal dialogs landed fixed the problem this code was trying to address.

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 433523 [details] [diff] [review]
popup fix

Discussed this with bent late last week, we're going to try and use the dll interceptor class we have in toolkit vs. google's iat patcher.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.2

changes: prevent adding additional hooks per instance. The interceptor is funny, it stays initialized and attached to a particular module for it's lifetime, even if you call init multiple times with different module names. So I've added a test so that it only initializes once and the hook is only set once in the first instance. TrackPopupHookProc is static and we condition our menu code on gWinlessPopupSurrogateHWND being set, so leaving the hook in place should be ok.

Comment 12

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 433940 [details] [diff] [review]
patch v.2

>+WindowsDllInterceptor sNtDllIntercept;

Kinda odd name... We're not hooking ntdll thankfully ;). Make it static too?

>@@ -598,16 +603,17 @@ PluginInstanceChild::AnswerNPP_SetWindow
> ...
>+          CreateWinlessPopupSurrogate();

Should you only do this if mQuirks has QUIRK_WINLESS_TRACKPOPUP_HOOK?

>+typedef BOOL (WINAPI *User32TrackPopupMenu) (HMENU hMenu,
>+                                             UINT uFlags,
>+                                             int x,
>+                                             int y,
>+                                             int nReserved,
>+                                             HWND hWnd,
>+                                             CONST RECT *prcRect);
>+HWND gWinlessPopupSurrogateHWND = NULL;
>+static User32TrackPopupMenu User32TrackPopupMenuStub = NULL;

I would rather these get moved to the top of the file where other typedefs and statics usually live. And I'd make gWinlessPopupSurrogateHWND be actually static and change the "g" to "s" and add an "s" to the stub too to keep everything consistent.

>+  bool haveClass = GetClassNameW(hWnd, szClass,
>+      sizeof(szClass)/sizeof(PRUnichar));

NS_ARRAY_LENGTH(szClass) instead of the sizeof dance.

>+  if (!haveClass || (haveClass &&

The second |haveClass| is unnecessary here.

>+  if (focusHwnd && IsWindow(focusHwnd)) {

I think IsWindow(0) will return false, maybe the first null check is unnecessary.

>+  if (!isRetCmdCall) {
>+      if (res != 0) {
>+          SendMessage(hWnd, WM_COMMAND, MAKEWPARAM(res, 0), 0);
>+          return 1;
>+      }
>+      return 0;
>+  }
>+
>+  return res;

I think you can simplify this all to:

  if (!isRetCmdCall && res) {
    SendMessage(...)
  }
  return res;

>+    NS_NAMED_LITERAL_CSTRING(dll, "user32.dll");
>+    NS_NAMED_LITERAL_CSTRING(func, "TrackPopupMenu");

Why the NAMED_LITERALs? You only ever use them once and then use get()...

>+bool
>+PluginInstanceChild::CreateWinlessPopupSurrogate()

You don't seem to ever use the return value here. Use it or lose it ;)

>+    mWinlessPopupSurrogateHWND =
>+        ::CreateWindowEx(0,
>+                         L"Static", NULL,
>+                         WS_CHILD, 0, 0,
>+                         0, 0, hwnd, 0, GetModuleHandle(NULL), 0);

Do you think we should do the WS_EX_NOPARENTNOTIFY here? We won't deadlock because the parent should be in an RPC loop, but still...

Nit: Maybe rewrap this, you've got one arg on a line, then two, then three, and finally six! Also, lose the |::|?

>+    // TrackPopupMenu will fail if the parent window is not associated with
>+    // our ui thread. So we hook TrackPopupMenu so we can hand in a surrogate
>+    // parent created in the child process.
>+    if (mQuirks & QUIRK_WINLESS_TRACKPOPUP_HOOK && // XXX turn on by default?

Nit: Just for my sanity how about putting |mQuirks & QUIRK_WINLESS_TRACKPOPUP_HOOK| in parens?

>+      gWinlessPopupSurrogateHWND = mWinlessPopupSurrogateHWND;

Is it possible for gWinlessPopupSurrogateHWND to have a value here (reentry)? Can we assert that it is null?

>+        QUIRK_WINLESS_TRACKPOPUP_HOOK = 2, // Win32

Can you add (or move from elsewhere) a comment here saying what's going on? Seems like a good place to document the whole problem.

>+    nsCString ModulePath() { return mModuleName; }

We usually do |void ModulePath(nsCString& aPath) { aPath.Assign(mModuleName); }| to avoid temporary objects.

Comment 13

[Jim Mathies [:jimm]]

(In reply to comment #12)
> (From update of attachment 433940 [details] [diff] [review])
> >+WindowsDllInterceptor sNtDllIntercept;
> 
> Kinda odd name... We're not hooking ntdll thankfully ;). Make it static too?

oops, copy paste error.

> 
> >@@ -598,16 +603,17 @@ PluginInstanceChild::AnswerNPP_SetWindow
> > ...
> >+          CreateWinlessPopupSurrogate();
> 
> Should you only do this if mQuirks has QUIRK_WINLESS_TRACKPOPUP_HOOK?

added.

> 
> >+typedef BOOL (WINAPI *User32TrackPopupMenu) (HMENU hMenu,
> >+                                             UINT uFlags,
> >+                                             int x,
> >+                                             int y,
> >+                                             int nReserved,
> >+                                             HWND hWnd,
> >+                                             CONST RECT *prcRect);
> >+HWND gWinlessPopupSurrogateHWND = NULL;
> >+static User32TrackPopupMenu User32TrackPopupMenuStub = NULL;
> 
> I would rather these get moved to the top of the file where other typedefs and
> statics usually live. And I'd make gWinlessPopupSurrogateHWND be actually
> static and change the "g" to "s" and add an "s" to the stub too to keep
> everything consistent.
> 
> >+  bool haveClass = GetClassNameW(hWnd, szClass,
> >+      sizeof(szClass)/sizeof(PRUnichar));
> 
> NS_ARRAY_LENGTH(szClass) instead of the sizeof dance.
> 
> >+  if (!haveClass || (haveClass &&
> 
> The second |haveClass| is unnecessary here.
> 
> >+  if (focusHwnd && IsWindow(focusHwnd)) {
> 
> I think IsWindow(0) will return false, maybe the first null check is
> unnecessary.
> 
> >+  if (!isRetCmdCall) {
> >+      if (res != 0) {
> >+          SendMessage(hWnd, WM_COMMAND, MAKEWPARAM(res, 0), 0);
> >+          return 1;
> >+      }
> >+      return 0;
> >+  }
> >+
> >+  return res;
> 
> I think you can simplify this all to:
> 
>   if (!isRetCmdCall && res) {
>     SendMessage(...)
>   }
>   return res;
> 

updated.

> >+    NS_NAMED_LITERAL_CSTRING(dll, "user32.dll");
> >+    NS_NAMED_LITERAL_CSTRING(func, "TrackPopupMenu");
> 
> Why the NAMED_LITERALs? You only ever use them once and then use get()...

Personal preference I guess, NS_LITERAL_CSTRING("xyz").get() are always so long they force a wrap. I switched them though.

> 
> >+bool
> >+PluginInstanceChild::CreateWinlessPopupSurrogate()
> 
> You don't seem to ever use the return value here. Use it or lose it ;)

updated.

> 
> >+    mWinlessPopupSurrogateHWND =
> >+        ::CreateWindowEx(0,
> >+                         L"Static", NULL,
> >+                         WS_CHILD, 0, 0,
> >+                         0, 0, hwnd, 0, GetModuleHandle(NULL), 0);
> 
> Do you think we should do the WS_EX_NOPARENTNOTIFY here? We won't deadlock
> because the parent should be in an RPC loop, but still...

Nice catch, will add.
  
> 
> Nit: Maybe rewrap this, you've got one arg on a line, then two, then three, and
> finally six! Also, lose the |::|?
> 
> >+    // TrackPopupMenu will fail if the parent window is not associated with
> >+    // our ui thread. So we hook TrackPopupMenu so we can hand in a surrogate
> >+    // parent created in the child process.
> >+    if (mQuirks & QUIRK_WINLESS_TRACKPOPUP_HOOK && // XXX turn on by default?
> 
> Nit: Just for my sanity how about putting |mQuirks &
> QUIRK_WINLESS_TRACKPOPUP_HOOK| in parens?

updated.

> 
> >+      gWinlessPopupSurrogateHWND = mWinlessPopupSurrogateHWND;
> 
> Is it possible for gWinlessPopupSurrogateHWND to have a value here (reentry)?
> Can we assert that it is null?

If the handle event blocked for some reason without calling track popup menu, then yes. It would be harmless, we just need to be sure it's set if they call tpm on the event. Hopefully plugins aren't doing anything wacky like calling the api on some internal event outside of this, but we'd assert in that case in the hook so we'd know about it.
 
> 
> >+        QUIRK_WINLESS_TRACKPOPUP_HOOK = 2, // Win32
> 
> Can you add (or move from elsewhere) a comment here saying what's going on?
> Seems like a good place to document the whole problem.
> 

sure.


> >+    nsCString ModulePath() { return mModuleName; }
> 
> We usually do |void ModulePath(nsCString& aPath) { aPath.Assign(mModuleName);
> }| to avoid temporary objects.

I took this out, remnants of the previous patch.

Comment 14

[Jim Mathies [:jimm]]

Attachment: updated patch

Comment 15

[Jim Mathies [:jimm]]

Attachment: updated patch

nixed unused mModuleName.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 434120 [details] [diff] [review]
updated patch

Looks good, r=me if you lose these:

NS_LITERAL_CSTRING("user32.dll").get()
NS_LITERAL_CSTRING("TrackPopupMenu").get()

Comment 17

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/60f50bdbc544

Comment 18

[Benjamin Smedberg]

http://hg.mozilla.org/projects/firefox-lorentz/rev/b50d7575d506

Comment 19

[Benjamin Smedberg]

and http://hg.mozilla.org/projects/firefox-lorentz/rev/846dd0f94c99 because the branch still has separate tiers

Comment 20

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

Blanket approval for Lorentz merge to mozilla-1.9.2
a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed

Comment 21

[Benjamin Smedberg]

Merged into 1.9.2 at http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430