317486 - Some plug-ins may eat WM_SETFOCUS / WM_KILLFOCUS

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Masatoshi Kimura [:emk]]

Steps to reproduce:
1. Load the testcase.
2. Click on the Flash textbox.
3. Alt+Tab to switch other application.
4. Alt+tab again to return Mozilla.
Actual result:
Blur event won't be fired after step 3.
You can't type text after step 4.
Expected result:
Blur event should be fired.
You will have to press tab to type in textbox, but it's out of scope for this bug.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Testcase

Comment 2

[Masatoshi Kimura [:emk]]

Attachment: Patch rv1.0

Comment 3

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

Comment on attachment 203996 [details] [diff] [review]
Patch rv1.0

Let's check what Ere has to say about this...

Comment 4

[Ere Maijala (slow)]

Comment on attachment 203996 [details] [diff] [review]
Patch rv1.0

I think this is worth a shot, but I believe getting mPrevWinProc is in wrong place. You should move it to SubclassAndAssociateWindow. There you can do it unconditionally before the call to SubclassWindow and it'd be a simple assignment of  currentWndProc.

Comment 5

[Masatoshi Kimura [:emk]]

> I think this is worth a shot, but I believe getting mPrevWinProc is in wrong
> place. You should move it to SubclassAndAssociateWindow. There you can do it
> unconditionally before the call to SubclassWindow and it'd be a simple
> assignment of  currentWndProc. 
No, it's too late to get mPrevWinProc in SubclassAndAssociateWindow.
Some plug-ins may already subclass our window in NPP_SetWindow which is called from CallSetWindow (And actually Flash Player did).
Thus we are required to get mPrevWinProc BEFORE CallSetWindow.

Comment 6

[Masatoshi Kimura [:emk]]

Attachment: Adding comment to clarify:-)

Comment 7

[Ere Maijala (slow)]

Comment on attachment 204918 [details] [diff] [review]
Adding comment to clarify:-)

Ok, but there's still one problem. You're assigning mPrevWinProc regardless of aPluginInstance (!= NULL) and only once. This will break at least if the first call is CallSetWindow(NULL), and it will cause a possibility for deadlock when unsubclassing (mPrevWinProc will point to the current WinProc). So what I think you should do is null it out whe unsubclassing. Then call it conditionally when the window messages are received. Ok?

Then you should get rid of the assertion. Just get the current WndProc, compare to PluginWndProc and assign to mPrevWndProc if different. This would make the code more bulletproof.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.2

Resolving review comment.

Comment 9

[Ere Maijala (slow)]

Comment on attachment 204938 [details] [diff] [review]
Patch rv 1.2

+      // Make sure activate/deactivate
+      // even if focus messages are eaten by plug-in

Make it something like this:

// Make sure setfocus and killfocus get through 
// even if they are eaten by the plugin

+      WNDPROC prevWndProc = (WNDPROC)win->GetPrevWindowProc();

No need for the cast (WNDPROC) here, right?

r=emaijala with these fixed.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Patch rv 1.3

Also limited prevWndProc scope.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.