Last Comment Bug 1213567 - crash in mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard()
: crash in mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard()
Status: RESOLVED FIXED
: crash, regression
Product: Core
Classification: Components
Component: General (show other bugs)
: 42 Branch
: x86 Windows NT
-- critical with 1 vote (vote)
: mozilla44
Assigned To: Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)
:
:
Mentors:
Depends on: 1217565
Blocks: 1209464
  Show dependency treegraph
 
Reported: 2015-10-10 03:26 PDT by [:philipp]
Modified: 2016-01-08 07:41 PST (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed
fixed


Attachments
Patch (8.36 KB, patch)
2015-10-19 07:39 PDT, Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)
jmathies: review+
Details | Diff | Splinter Review
Patch with updated comments (8.79 KB, patch)
2015-10-20 13:39 PDT, Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman)
aklotz: review+
sledru: approval‑mozilla‑aurora+
sledru: approval‑mozilla‑beta+
Details | Diff | Splinter Review
stackoverflow-beta.patch (9.60 KB, patch)
2015-10-22 03:47 PDT, Bob Owen (:bobowen)
no flags Details | Diff | Splinter Review

Description User image [:philipp] 2015-10-10 03:26:32 PDT
This bug was filed from the Socorro interface and is 
report bp-8e24dbb8-6ae0-41ac-9c78-bca312151009.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard() 	widget/windows/TSFTextStore.cpp
1 	xul.dll 	nsWindow::ExternalHandlerProcessMessage(unsigned int, unsigned int&, long&, mozilla::widget::MSGResult&) 	widget/windows/nsWindow.cpp

though this is currently a low level crash on firefox 42 beta on windows (~rank 50), it seems to be newly introduced in this version. the volume on devedition and nightly is minute so it's not possible to pin it down when it might have started looking just on crash stats...
Comment 1 User image Masayuki Nakano [:masayuki] (JST, +0900) 2015-10-10 19:30:09 PDT
Hmm, I don't think that the stack indicates actual bug since it's crashed by stack overflow but the collected stack is just 2 (and their relation is odd). Additionally, nobody comments *when* it occurred. So, we need to look for other hints.
Comment 2 User image Masayuki Nakano [:masayuki] (JST, +0900) 2015-10-10 19:51:59 PDT
I think that this stack tells us what's this bug.
https://crash-stats.mozilla.com/report/index/2dba69b0-b117-4cc7-a1e7-4d10c2151004

>  0 	xul.dll	nsWindow::ExternalHandlerProcessMessage(unsigned int, unsigned int&, long&, mozilla::widget::MSGResult&)
> 1 	xul.dll	nsWindow::ProcessMessage(unsigned int, unsigned int&, long&, long*)
> 2 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 3 	xul.dll	CallWindowProcCrashProtected
> 4 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 5 	user32.dll	InternalCallWinProc
> 6 	user32.dll	UserCallWinProcCheckWow
> 7 	user32.dll	CallWindowProcAorW
> 8 	user32.dll	CallWindowProcW
> 9 	xul.dll	`anonymous namespace'::ProcessOrDeferMessage(HWND__*, unsigned int, unsigned int, long)
> 10 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 11 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 12 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 13 	user32.dll	InternalCallWinProc
> 14 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 15 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 16 	user32.dll	UserCallWinProcCheckWow
> 17 	user32.dll	CallWindowProcAorW
> 18 	user32.dll	CallWindowProcW
> 19 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 20 	xul.dll	CallWindowProcCrashProtected
> 21 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 22 	user32.dll	InternalCallWinProc
> 23 	user32.dll	UserCallWinProcCheckWow
> 24 	user32.dll	CallWindowProcAorW
> 25 	user32.dll	CallWindowProcW
> 26 	xul.dll	`anonymous namespace'::ProcessOrDeferMessage(HWND__*, unsigned int, unsigned int, long)
> 27 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 28 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 29 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 30 	user32.dll	InternalCallWinProc
> 31 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 32 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 33 	user32.dll	UserCallWinProcCheckWow
> 34 	user32.dll	CallWindowProcAorW
> 35 	user32.dll	CallWindowProcW
> 36 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 37 	xul.dll	CallWindowProcCrashProtected
> 38 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 39 	user32.dll	InternalCallWinProc
> 40 	user32.dll	UserCallWinProcCheckWow
> 41 	user32.dll	CallWindowProcAorW
> 42 	user32.dll	CallWindowProcW
> 43 	xul.dll	`anonymous namespace'::ProcessOrDeferMessage(HWND__*, unsigned int, unsigned int, long)
> 44 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 45 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 46 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 47 	user32.dll	InternalCallWinProc
> 48 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 49 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 50 	user32.dll	UserCallWinProcCheckWow
> 51 	user32.dll	CallWindowProcAorW
> 52 	user32.dll	CallWindowProcW
> 53 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 54 	xul.dll	CallWindowProcCrashProtected
> 55 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 56 	user32.dll	InternalCallWinProc
> 57 	user32.dll	UserCallWinProcCheckWow
> 58 	user32.dll	CallWindowProcAorW
> 59 	user32.dll	CallWindowProcW
> 60 	xul.dll	`anonymous namespace'::ProcessOrDeferMessage(HWND__*, unsigned int, unsigned int, long)
> 61 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 62 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 63 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 64 	user32.dll	InternalCallWinProc
> 65 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 66 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 67 	user32.dll	UserCallWinProcCheckWow
> 68 	user32.dll	CallWindowProcAorW
> 69 	user32.dll	CallWindowProcW
> 70 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 71 	xul.dll	CallWindowProcCrashProtected
> 72 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 73 	user32.dll	InternalCallWinProc
> 74 	user32.dll	UserCallWinProcCheckWow
> 75 	user32.dll	CallWindowProcAorW
> 76 	user32.dll	CallWindowProcW
> 77 	xul.dll	`anonymous namespace'::ProcessOrDeferMessage(HWND__*, unsigned int, unsigned int, long)
> 78 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 79 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 80 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 81 	user32.dll	InternalCallWinProc
> 82 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 83 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 84 	user32.dll	UserCallWinProcCheckWow
> 85 	user32.dll	CallWindowProcAorW
> 86 	user32.dll	CallWindowProcW
> 87 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 88 	xul.dll	CallWindowProcCrashProtected
> 89 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long)
> 1015 	xul.dll	NeuteredWindowProc(HWND__*, unsigned int, unsigned int, long)
> 1016 	user32.dll	InternalCallWinProc
> 1017 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 1018 	xul.dll	`anonymous namespace'::NeuterWindowProcedure(HWND__*)
> 1019 	user32.dll	UserCallWinProcCheckWow
> 1020 	user32.dll	CallWindowProcAorW
> 1021 	user32.dll	CallWindowProcW
> 1022 	xul.dll	nsWindow::WindowProcInternal(HWND__*, unsigned int, unsigned int, long)
> 1023 	xul.dll	CallWindowProcCrashProtected
> 1024 	xul.dll	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long

Looks like that this is related to this change:
http://hg.mozilla.org/mozilla-central/diff/3510c76c789f/widget/src/windows/nsWindow.cpp

Ehsan, Benjamin, do you have some idea for this?
Comment 3 User image Benjamin Smedberg [:bsmedberg] 2015-10-12 09:23:50 PDT
I don't think that commit has much to do with this. This likely has something to do with the IPC window neutering code, and the recent commits to WindowsMessageLoop.cpp:

f641c7662bdd 2015-10-04 22:17 -0600 Aaron Klotz - Bug 1209464: Fix missing neutered window region in MessageChannel::WaitForInterruptNotify. Regression from bug 1189709; r=jimm
993384a9dc2e 2015-08-26 15:57 -0600 Aaron Klotz - Bug 1189709 - Reduce scope of MessageChannel window neutering. r=jimm
f290fbb44af6 2015-07-21 01:21 -0600 Aaron Klotz - Bug 1185639 - Allow deferred message processing to happen between consecutive IPC message dispatches. r=jimm
Comment 4 User image :Ehsan Akhgari (needinfo please, extremely long backlog) 2015-10-13 07:06:17 PDT
Sorry, I don't know much about this code.  Bill owns IPC these days.
Comment 6 User image Robert Kaiser 2015-10-13 09:32:06 PDT
Note that the rising signatures I mention in comment #5 have been around for a while at lower volumes but are higher in b5.
Comment 7 User image Bill McCloskey (:billm) 2015-10-13 10:04:56 PDT
Let's start by needinfoing Aaron.
Comment 8 User image Sylvestre Ledru [:sylvestre] 2015-10-13 10:11:11 PDT
Regression & crash, tracking.
Comment 9 User image Masayuki Nakano [:masayuki] (JST, +0900) 2015-10-13 17:43:40 PDT
Some useful comments from https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Awidget%3A%3ATSFStaticSink%3A%3AEnsureInitActiveTIPKeyboard%28%29#tab-comments

* Firefox keeps crashing when I'm clicking on arrows in online video presentations.
* constant shutdowns when on sites or in mail
* I tried opening up an email from Workspace

from https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsWindow%3A%3AExternalHandlerProcessMessage%28unsigned+int%2C+unsigned+int%26%2C+long%26%2C+mozilla%3A%3Awidget%3A%3AMSGResult%26%29#tab-comments

* firefox crashes regularly I don't know why. after I restart it it seems OK for a while then crashes again. It gets into a state where it has a spinning icon that indicates that it is waiting for something to finish, but it never recovers and I have to kill it and restart it. I don't have to restart the computer, just firefox 
* i was searching Ebay for a fuel tank
* every time I hit golf channel to go back to main page it crashes 


It seems that not specific user action triggers this.
Comment 10 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-14 11:38:32 PDT
It looks like a window procedure is being "doubly neutered" even though that isn't supposed to be possible (sounds painful).
Comment 11 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-14 14:01:51 PDT
OK, it looks like what I'm seeing here is that something called nsWindow::SubclassWindow while that window was neutered...
Comment 12 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-14 15:03:29 PDT
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #11)
> OK, it looks like what I'm seeing here is that something called
> nsWindow::SubclassWindow while that window was neutered...

This could happen if a new nsWindow was created while neutering was in place. CreateWindowEx would send initial messages to the window procedure, which the window neutering hook would catch, then subclassing the window. But once CreateWindowEx returns, nsWindow::Create also tries to subclass the window itself, which returns the neutered window procedure as the previous one, creating a cycle.

I am not sure why we're only seeing this now; without further data my best guess is that the additional neutered window region in MessageChannel::Call creates a condition where the nsWindow could be created while neutering is active.

I think that the right thing to do here (and it must be done very carefully, might I add) is to suppress neutering during the creation of a new window.
Comment 13 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-14 17:54:22 PDT
OK, so the main problem is async init + neutered window regions + a11y.

If PluginInstanceParent::RecvAsyncNPP_NewResult indicates that the child-side NPP_New was successful, it creates the plugin widget. It is possible that this happens while neutering is activated during MessageChannel::WaitForIncomingMessage or another MessageChannel function.

For most messages this is OK because the neutered window proc either saves the message for later or sends it to DefWindowProc. In the WM_GETOBJECT case, the neutered window proc tries to call the original wnd proc which contains a cycle as described in comment 12.
Comment 14 User image Sylvestre Ledru [:sylvestre] 2015-10-19 02:14:31 PDT
Aaron, today is the go to build of beta 8 (understand: we are very late in the cycle). Any chance you could fix this issue? Thanks
Comment 15 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-19 07:39:19 PDT
Created attachment 8675704 [details] [diff] [review]
Patch

As pointed out in previous comments, the problem here is that neutering would happen upon window creation, which mean that the subsequent subclassing that occurs in nsWindow::Create ends up subclassing the NeuteredWindowProcedure, not the real one.

This patch wraps CreateWindow* so that we avoid neutering during the window creation messages.
Comment 16 User image Sylvestre Ledru [:sylvestre] 2015-10-20 10:08:45 PDT
Jim, sorry for the pressure but it would be great if you could review that quickly.
Comment 17 User image Jim Mathies [:jimm] 2015-10-20 12:54:16 PDT
Comment on attachment 8675704 [details] [diff] [review]
Patch

Review of attachment 8675704 [details] [diff] [review]:
-----------------------------------------------------------------

afaict 64bit has the same definitions, so looks like you're ok there too.

::: ipc/glue/WindowsMessageLoop.cpp
@@ +436,5 @@
>    gDeferredMessages->AppendElement(deferred);
>    return res;
>  }
>  
> +WindowsDllInterceptor sUser32Interceptor;

Please add a good comment block explaining what this is for and why we set it.
Comment 18 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-20 13:39:08 PDT
Created attachment 8676471 [details] [diff] [review]
Patch with updated comments

Updated with comments, carrying forward r+.
Comment 20 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-20 13:43:04 PDT
Comment on attachment 8676471 [details] [diff] [review]
Patch with updated comments

Approval Request Comment
[Feature/regressing bug #]: out-of-process plugins + asyncInit + a11y
[User impact if declined]: Stack overflow crashes when all three features are enabled
[Describe test coverage new/current, TreeHerder]: Plugin test suite
[Risks and why]: Low, the fundamental concept behind this patch is just flipping a boolean flag under certain conditions.
[String/UUID change made/needed]: None
Comment 21 User image Carsten Book [:Tomcat] 2015-10-21 07:30:02 PDT
https://hg.mozilla.org/mozilla-central/rev/828b2b47a4db
Comment 22 User image Sylvestre Ledru [:sylvestre] 2015-10-21 09:06:49 PDT
Comment on attachment 8676471 [details] [diff] [review]
Patch with updated comments

Important crashes, taking it.
Should be in 42 beta 9.
Comment 24 User image Carsten Book [:Tomcat] 2015-10-22 00:56:11 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/aac4b4aaa5f4
Comment 25 User image Carsten Book [:Tomcat] 2015-10-22 01:07:16 PDT
backed out from beta for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=596415&repo=mozilla-beta
Comment 26 User image Bob Owen (:bobowen) 2015-10-22 03:47:35 PDT
Created attachment 8677372 [details] [diff] [review]
stackoverflow-beta.patch

Here's my updated patch for Beta.

This changes the MOZ_RAII to a MOZ_STACK_CLASS, as MOZ_RAII hasn't hit Beta yet.

It also changes the assertion in widget/windows/nsWindow.cpp, to allow for the neutering suppression.
(By the way, the current debug version of Beta already crashes on this assertion currently, when I try and run flash.)
This part will need landing separately on Nightly and Aurora as well.

Sylvestre - not sure what you want to do as I know you wanted this in Beta 9.
Comment 27 User image Aaron Klotz [:aklotz] (a11y work receiving priority right now, please send interceptor reviews to dmajor or handyman) 2015-10-22 07:37:26 PDT
(In reply to Bob Owen (:bobowen) from comment #26)
> Created attachment 8677372 [details] [diff] [review]
> stackoverflow-beta.patch
> 
> Here's my updated patch for Beta.
> 
> This changes the MOZ_RAII to a MOZ_STACK_CLASS, as MOZ_RAII hasn't hit Beta
> yet.
> 
> It also changes the assertion in widget/windows/nsWindow.cpp, to allow for
> the neutering suppression.
> (By the way, the current debug version of Beta already crashes on this
> assertion currently, when I try and run flash.)
> This part will need landing separately on Nightly and Aurora as well.
> 
> Sylvestre - not sure what you want to do as I know you wanted this in Beta 9.

Yeah, Bob's revision to the patch is what is needed for this to land on Beta.
Comment 28 User image Carsten Book [:Tomcat] 2015-10-22 08:02:53 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/45ab7cdffbb4

since it had approval per comment #22 and aaron confirmed this is the right approach

Note You need to log in before you can comment on or make changes to this bug.