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

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: benjamin, Assigned: jimm)

Tracking

Trunk
x86
Windows 7
Points:
---

Firefox Tracking Flags

(blocking1.9.2 .4+, status1.9.2 .4-fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

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

Comment 1

9 years ago
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?
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
(Reporter)

Comment 3

9 years ago
That's quite different, actually.

Updated

9 years ago
blocking1.9.2: ? → .4+
(Reporter)

Comment 4

9 years ago
Created attachment 443953 [details] [diff] [review]
ReplyMessage, rev. 1

It appears that all of these are WM_IME_SETCONTEXT fallout.
Attachment #443953 - Flags: review?(jmathies)
(Assignee)

Updated

9 years ago
Attachment #443953 - Flags: review?(masayuki)
(Assignee)

Comment 5

9 years ago
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?
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

9 years ago
This is blocking 1.9.2.4. Any update on the review/patch?
(Reporter)

Comment 8

9 years ago
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.
I still don't understand the cause well. Isn't the cause we go to the locked child process by the UpdateWindow()?
(Reporter)

Comment 10

9 years ago
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 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.
(Reporter)

Comment 12

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

LRESULT res = ::CallWindowProcW(someWindow->GetPrevWindowProc(),
                                hWnd, msg, wParam, lParam);
(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.
(Reporter)

Comment 14

9 years ago
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!
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.
(Reporter)

Comment 16

9 years ago
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.
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).
(Assignee)

Comment 18

9 years ago
(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.
> 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...
(Reporter)

Comment 20

9 years ago
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 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.
Attachment #443953 - Flags: review?(masayuki) → review+
(Assignee)

Updated

9 years ago
Attachment #443953 - Flags: review?(jmathies)
(Assignee)

Comment 22

9 years ago
http://hg.mozilla.org/mozilla-central/rev/d6eb117221a3
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Attachment #443953 - Flags: approval1.9.2.4?

Comment 23

9 years ago
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
Attachment #443953 - Flags: approval1.9.2.4? → approval1.9.2.4+

Updated

9 years ago
Assignee: benjamin → jmathies
Is there a STR for this to be used for bug verification? I don't see anything above.
You need to log in before you can comment on or make changes to this bug.