1213567 - crash in mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard()

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[[:philipp]]

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

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

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

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

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

[Benjamin Smedberg]

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

[(no longer active)]

Sorry, I don't know much about this code.  Bill owns IPC these days.

Comment 5

[Robert Kaiser]

Hmm, we have a number of crashes rising in 42.0b5, e.g. https://crash-stats.mozilla.com/report/list?signature=nsWindow%3A%3AProcessMessage%28unsigned%20int%2C%20unsigned%20int%26%2C%20long%26%2C%20long%2A%29 https://crash-stats.mozilla.com/report/list?signature=RtlActivateActivationContextUnsafeFast%20%7C%20UserCallWinProcCheckWow https://crash-stats.mozilla.com/report/list?signature=_SEH_prolog4 https://crash-stats.mozilla.com/report/list?signature=InternalFindAtom https://crash-stats.mozilla.com/report/list?signature=nsWindow%3A%3AExternalHandlerProcessMessage%28unsigned%20int%2C%20unsigned%20int%26%2C%20long%26%2C%20mozilla%3A%3Awidget%3A%3AMSGResult%26%29 https://crash-stats.mozilla.com/report/list?signature=PluginWndProcInternal https://crash-stats.mozilla.com/report/list?signature=GetWindowLongW

It sounds like at least multiple, if not all of those are connected to window stuff, may they all go back to this landing of bug 1209464 in that beta?

Comment 6

[Robert Kaiser]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Let's start by needinfoing Aaron.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Regression & crash, tracking.

Comment 9

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

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

[(No longer employed by Mozilla) Aaron Klotz]

It looks like a window procedure is being "doubly neutered" even though that isn't supposed to be possible (sounds painful).

Comment 11

[(No longer employed by Mozilla) Aaron Klotz]

OK, it looks like what I'm seeing here is that something called nsWindow::SubclassWindow while that window was neutered...

Comment 12

[(No longer employed by Mozilla) Aaron Klotz]

(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

[(No longer employed by Mozilla) Aaron Klotz]

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

[Sylvestre Ledru [:Sylvestre]]

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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: 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

[Sylvestre Ledru [:Sylvestre]]

Jim, sorry for the pressure but it would be great if you could review that quickly.

Comment 17

[Jim Mathies [:jimm]]

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

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Patch with updated comments

Updated with comments, carrying forward r+.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/828b2b47a4db

Comment 20

[(No longer employed by Mozilla) Aaron Klotz]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/828b2b47a4db

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8676471 [details] [diff] [review]
Patch with updated comments

Important crashes, taking it.
Should be in 42 beta 9.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88b2ba444fd9

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aac4b4aaa5f4

Comment 25

[Carsten Book [:Tomcat]]

backed out from beta for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=596415&repo=mozilla-beta

Comment 26

[Bob Owen (:bobowen)]

Attachment: 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

[(No longer employed by Mozilla) Aaron Klotz]

(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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45ab7cdffbb4

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