Closed Bug 545149 Opened 10 years ago Closed 10 years ago

Fix various issues with winless plugin context menus

Categories

(Core :: Plug-ins, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [fixed-lorentz])

Attachments

(1 file, 4 obsolete files)

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.
Would bug 547434 be this as well?
Duplicate of this bug: 547434
Blocks: LorentzBeta1
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.
Duplicate of this bug: 552051
Jim, just want to make sure that you seen in the bug you duped that the browser hangs and even sometimes crashes.
Assignee: nobody → jmathies
(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.
Also in Bug 552211, if I right click on the ad on top, the context-menu stays and CPU goes to 60%.
Attached patch popup fix (obsolete) — Splinter Review
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.
Attachment #433523 - Flags: review?(bent.mozilla)
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.
Attachment #433523 - Flags: review?(bent.mozilla) → review-
Attached patch patch (obsolete) — Splinter Review
Attachment #433523 - Attachment is obsolete: true
Attached patch patch v.2 (obsolete) — Splinter Review
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.
Attachment #433936 - Attachment is obsolete: true
Attachment #433940 - Flags: review?(bent.mozilla)
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.
(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.
Attached patch updated patch (obsolete) — Splinter Review
Attachment #433940 - Attachment is obsolete: true
Attachment #433940 - Flags: review?(bent.mozilla)
Attached patch updated patchSplinter Review
nixed unused mModuleName.
Attachment #434118 - Attachment is obsolete: true
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()
Attachment #434120 - Flags: review+
Summary: Plugin context menus don't hide properly → Fix various issues with winless plugin context menus
http://hg.mozilla.org/mozilla-central/rev/60f50bdbc544
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
and http://hg.mozilla.org/projects/firefox-lorentz/rev/846dd0f94c99 because the branch still has separate tiers
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
You need to log in before you can comment on or make changes to this bug.