564260 - Plugin hangs, plugin is [@ hang | KiFastSystemCallRet | NtUserSetFocus]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Parent stacks are CallUpdateWindow and CallNPP_SetWindow. I'm pulling minidumps now:

https://crash-stats.mozilla.com/report/index/12ea445d-0aea-402c-8727-f27ef2100505

Comment 1

[Benjamin Smedberg]

The parent stack is
 	xul.dll!mozilla::ipc::RPCChannel::Call(msg=0x07b22a18, reply=0x004ee114)  Line 210	C++
 	xul.dll!mozilla::plugins::PPluginInstanceParent::CallUpdateWindow()  Line 620	C++
 	xul.dll!nsWindow::OnPaint(aDC=0x00000000) 	C++
 	xul.dll!nsWindow::ProcessMessage(msg=0x0000000f, wParam=0x00000000, lParam=0x00000000, aRetValue=0x004ee448)  Line 4026	C++
 	xul.dll!nsWindow::WindowProc(hWnd=0x00000001, msg=0x0000000f, wParam=0x00000000, lParam=0x00000000)  Line 3725	C++
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_CallWindowProcAorW@24() 	
 	user32.dll!_CallWindowProcW@20() 	
 	xul.dll!mozilla::plugins::PluginInstanceParent::PluginWindowHookProc(hWnd=0x000303f2, message=0x0000000f, wParam=0x00000000, lParam=0x00000000)  Line 929	C++
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_CallWindowProcAorW@24() 	
 	user32.dll!_CallWindowProcW@20() 	
 	xul.dll!PluginWndProc(hWnd=0x000303f2, msg=0x017cd771, wParam=0x00000000, lParam=0x00000000)  Line 360	C++
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_DispatchClientMessage@20() 	
 	user32.dll!___fnDWORD@4() 	
 	ntdll.dll!_KiUserCallbackDispatcher@12() 	
 	user32.dll!_NtUserCallHwndLock@8() 	
 	user32.dll!_UpdateWindow@4() 	
 	xul.dll!nsWindow::DispatchStarvedPaints(aWnd=0x000403fc, aMsg=0x00000000)  Line 3118	C++
 	user32.dll!_InternalEnumWindows@24() 	
 	user32.dll!_EnumChildWindows@12() 	
 	xul.dll!nsWindow::DispatchPendingEvents()  Line 3161	C++
>	xul.dll!nsWindow::ProcessMessageForPlugin(aMsg={...}, aResult=0x00000001, aCallDefWndProc=)  Line 3809	C++
 	xul.dll!nsWindow::ProcessMessage(msg=0x00000282, wParam=0x00000001, lParam=0x00000000, aRetValue=0x004eea0c) 	C++
xul.dll!nsWindow::WindowProc(hWnd=0x00000001, msg=0x00000282, wParam=0x00000001, lParam=0x00000000)  Line 3725	C++
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_DispatchClientMessage@20() 	
 	user32.dll!___fnDWORD@4() 	
 	ntdll.dll!_KiUserCallbackDispatcher@12() 	
 	user32.dll!_NtUserMessageCall@28() 	
 	user32.dll!_SendMessageWorker@20() 	
 	user32.dll!_SendMessageW@16() 	
 	user32.dll!_SendOpenStatusNotify@12() 	
 	user32.dll!_ImeSetContextHandler@16() 	
 	user32.dll!_ImeWndProcWorker@20() 	
 	user32.dll!_ImeWndProcW@16() 	
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_DispatchClientMessage@20() 	
 	user32.dll!___fnDWORD@4() 	
 	ntdll.dll!_KiUserCallbackDispatcher@12() 	
 	user32.dll!_NtUserMessageCall@28() 	
 	user32.dll!_SendMessageWorker@20() 	
 	user32.dll!_RealDefWindowProcWorker@20() 	
 	user32.dll!_RealDefWindowProcW@16() 	
 	user32.dll!_DefWindowProcW@16() 	
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_CallWindowProcAorW@24() 	
 	user32.dll!_CallWindowProcW@20() 	
>	xul.dll!nsWindow::WindowProc(hWnd=0x00000001, msg=0x00000281, wParam=0x00000000, lParam=0xc000000f)  Line 3732	C++
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_DispatchClientMessage@20() 	
 	user32.dll!___fnDWORD@4() 	
 	ntdll.dll!_KiUserCallbackDispatcher@12() 	
 	user32.dll!_NtUserMessageCall@28() 	
 	user32.dll!_SendMessageWorker@20() 	
 	user32.dll!_SendMessageW@16() 	
 	imm32.dll!_ImmSetActiveContext@12() 	
 	user32.dll!_FocusSetIMCContext@8() 	
 	user32.dll!_ImeSystemHandler@16() 	
 	user32.dll!_ImeWndProcWorker@20() 	
 	user32.dll!_ImeWndProcW@16() 	
 	user32.dll!_InternalCallWinProc@20() 	
 	user32.dll!_UserCallWinProcCheckWow@32() 	
 	user32.dll!_DispatchClientMessage@20() 	
 	user32.dll!___fnDWORD@4() 	
 	ntdll.dll!_KiUserCallbackDispatcher@12() 	
 	user32.dll!_NtUserPeekMessage@20() 	
 	user32.dll!__PeekMessage@24() 	
 	user32.dll!_PeekMessageW@20() 	
 	xul.dll!nsAppShell::ProcessNextNativeEvent(mayWait=0x00000000)  Line 172	C++

So nsWindow::WindowProc gets WM_IME_SETCONTEXT, DefWindowProc ends up dispatching WM_IME_NOTIFY, which ends up... dispatching pending events? Which ends up painting. I don't understand the DispatchPendingEvents bit, but it's apparently related to bug 491848. Anyway, I think we should probably unconditionally ReplyMessage somewhere, but I'm not sure where. jimm?

Comment 2

[juan becerra [:juanb]]

I also ran into something similar by trying to reproduce bug 563936:

http://crash-stats.mozilla.com/report/index/bp-3608318b-4121-4f61-b415-b6e3e2100506

Comment 3

[Benjamin Smedberg]

That's quite different, actually.

Comment 4

[Benjamin Smedberg]

Attachment: ReplyMessage, rev. 1

It appears that all of these are WM_IME_SETCONTEXT fallout.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 443953 [details] [diff] [review]
ReplyMessage, rev. 1

Masayuki, thoughts on this? We get hangs when set context is sent via a SetFocus call in the child process. This patch will always return 0 for the return result to that send, which according to the docs for ImmIsUIMessage, indicates the ime window did not handle the event. Can you see any negative fall out from this?

Comment 6

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

Um, WM_IME_SETCONTEXT is a special message...
http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsIMM32Handler.cpp#647

The lParam for calling next wndproc specifies the IME behavior. So, it's used for IME behavior controlling. So, I'm afraid by this change.

I have another idea that should we update other process's window? If a plug-in needs to update its window, the plug-in should do it itself. I want to know why the force updating system is created. If the bug is only for our window painting, we can ignore the plug-in windows. (I cannot look mxr now...)

Comment 7

[LegNeato]

This is blocking 1.9.2.4. Any update on the review/patch?

Comment 8

[Benjamin Smedberg]

The painting really isn't important in this context. We could be processing any Windows messsage there (a mousemove, for example) and do an RPC call in that context. Any RPC call is going to trigger this hang.

Comment 9

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

I still don't understand the cause well. Isn't the cause we go to the locked child process by the UpdateWindow()?

Comment 10

[Benjamin Smedberg]

We go to the locked child process in RPCChannel::Call. This happens from lots of different messages: pretty much any NPAPI call will call it, including windowless painting, any windowless event directed at a plugin, scripts calling into plugins, etc.

Comment 11

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

Comment on attachment 443953 [details] [diff] [review]
ReplyMessage, rev. 1

Um, I still don't think that this patch is safe. I think that the ideal fix is just calling next wndproc without doing anything when we receive WM_IME_CONTEXT.

I'll test the patch with Flash Player, Silverlight and Adobe Acrobat. Wait a couple of hours.

Comment 12

[Benjamin Smedberg]

"the next wndproc"? That's the status quo, isn't it?

LRESULT res = ::CallWindowProcW(someWindow->GetPrevWindowProc(),
                                hWnd, msg, wParam, lParam);

Comment 13

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

(In reply to comment #12)
> "the next wndproc"? That's the status quo, isn't it?

Yes. The important thing is we should do nothing before that.

Comment 14

[Benjamin Smedberg]

So how do we solve the bug? It appears that processing WM_IME_SETCONTEXT can lead to executing arbitrary code, which can cause a deadlock. We can either
 
* unblock the caller (ReplyMessage)
* stop executing arbitrary code?
** maybe stop calling nsWindow::DispatchPendingEvents from nsWindow::ProcessMessageForPlugin, but I don't know why that loop was added in the first place, which makes me more nervous than this patch!

Comment 15

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

Hey, it seems that we discuss about wrong message. ProcessMessageForPlugin() doesn't call DispatchPendingEvents() at WM_IME_SETCONTEXT. Looks like that it's called at WM_IME_NOTIFY. I think that you should add the message to IPCWindowProcHandler() instead of WM_IME_SETCONTEXT. WM_IME_NOTIFY is just a notification, so, unlocking should be safe.

Comment 16

[Benjamin Smedberg]

But... the sequence of events is:

plugin-process: plugin calls SetFocus
plugin-process: SetFocus calls SendMessage(WM_IME_SETCONTEXT)
browser-process: receives WM_IME_SETCONTEXT, dispatches normally
browser-process: DefWindowProc processes WM_IME_SETCONTEXT, ends up in
browser-process ImeSetContextHandler calls SendMessage(WM_IME_NOTIFY)
which ends up in ProcessMessageForPlugin

If we call ::ReplyMessage in ProcessMessageForPlugin, we'll be replying to WM_IME_NOTIFY, but *not* WM_IME_SETCONTEXT, so we won't actually be unblocking the plugin. I think.

Comment 17

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

Ah, right. But isn't WM_IME_SETCONTEXT called by system too? Does it unlock the SetFocus()? If so, I'll mark r+. The patch works fine on Win7 but we should test on XP after landing (I cannot build the patched build on XP soon).

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #15)
> Hey, it seems that we discuss about wrong message. ProcessMessageForPlugin()
> doesn't call DispatchPendingEvents() at WM_IME_SETCONTEXT. Looks like that it's
> called at WM_IME_NOTIFY. I think that you should add the message to
> IPCWindowProcHandler() instead of WM_IME_SETCONTEXT. WM_IME_NOTIFY is just a
> notification, so, unlocking should be safe.

We wouldn't be replying to WM_IME_NOTIFY, since according to the docs, ReplyMessage only replies to a thread that is blocked waiting on the current thread. From MSDN: 

"If the message was not sent through SendMessage or if the message was sent by the same thread, ReplyMessage has no effect."

So even if we reply in ProcessMessageForPlugin, the result val we pass would go to plugin-process's SendMessage(WM_IME_SETCONTEXT).

Also note, in ProcessMessageForPlugin, WM_IME_NOTIFY has already been processed, we get wrapped up in DispatchPendingEvents() to fix some other ime related bug, which should probably have a better fix.

I don't think freeing the child waiting on WM_IME_SETCONTEXT is going to cause any problems. Whatever we do with that message in parent, it'll get processed. If it heads to the plugin, it'll get dropped by IPC on the receiving side anyway.

Comment 19

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

> The patch works fine on Win7 but we should
> test on XP after landing (I cannot build the patched build on XP soon).

Oops, but the issue should be happened only when the plug-in calls SetFocus(), so, I couldn't test this actually. I want to know the actual sites which can reproduce this bug...

Comment 20

[Benjamin Smedberg]

I don't think we ever came up with STR for this bug, it's entirely based on ex-post-facto debugging of minidump pairs.

Comment 21

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

Comment on attachment 443953 [details] [diff] [review]
ReplyMessage, rev. 1

O.K. Thank you for many comments.

> +    case WM_IME_SETCONTEXT;

Change ';' to ':', with that change r=masayuki.

I think that even if there is a regression, it's rare case, probably. And also even if it happens, IME users see a strange composition string rendering. But it's not critical for a11y. And the users will report bugs with STR. Then, we can check the behavior easily.

I'll post the possibility of the regression to Japanese community.

Comment 22

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/d6eb117221a3

Comment 23

[LegNeato]

Comment on attachment 443953 [details] [diff] [review]
ReplyMessage, rev. 1

a=LegNeato for 1.9.2.4. Please land on both mozilla-1.9.2 default and GECKO1924_20100413_RELBRANCH

Comment 24

[Jim Mathies [:jimm]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4cc2d2bb15ec
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0aff61215cdb

Comment 25

[Al Billings [:abillings - ex-MoCo]]

Is there a STR for this to be used for bug verification? I don't see anything above.