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

RESOLVED FIXED in Firefox 42

Status

()

Core
General
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: aklotz)

Tracking

({crash, regression})

42 Branch
mozilla44
x86
Windows NT
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42+ fixed, firefox43 fixed, firefox44 fixed)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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...
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.
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?
Flags: needinfo?(ehsan)
Flags: needinfo?(benjamin)
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
Flags: needinfo?(benjamin)
Sorry, I don't know much about this code.  Bill owns IPC these days.
Flags: needinfo?(ehsan) → needinfo?(wmccloskey)

Updated

2 years ago
Crash Signature: [@ mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard()] → [@ mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard()] [@ mozilla::widget::TSFStaticSink::EnsureInitActiveTIPKeyboard]

Comment 5

2 years ago
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

2 years ago
Note that the rising signatures I mention in comment #5 have been around for a while at lower volumes but are higher in b5.
Let's start by needinfoing Aaron.
Flags: needinfo?(wmccloskey) → needinfo?(aklotz)
Regression & crash, tracking.
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
tracking-firefox42: --- → +
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.
It looks like a window procedure is being "doubly neutered" even though that isn't supposed to be possible (sounds painful).
Assignee: nobody → aklotz
Flags: needinfo?(aklotz)
OK, it looks like what I'm seeing here is that something called nsWindow::SubclassWindow while that window was neutered...
(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.
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.

Updated

2 years ago
Blocks: 1209464
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
Flags: needinfo?(aklotz)
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.
Flags: needinfo?(aklotz)
Attachment #8675704 - Flags: review?(jmathies)
Jim, sorry for the pressure but it would be great if you could review that quickly.
Flags: needinfo?(jmathies)

Comment 17

2 years ago
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.
Attachment #8675704 - Flags: review?(jmathies) → review+

Updated

2 years ago
Flags: needinfo?(jmathies)
Created attachment 8676471 [details] [diff] [review]
Patch with updated comments

Updated with comments, carrying forward r+.
Attachment #8675704 - Attachment is obsolete: true
Attachment #8676471 - Flags: review+

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/828b2b47a4db
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
Attachment #8676471 - Flags: approval-mozilla-beta?
Attachment #8676471 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/828b2b47a4db
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8676471 [details] [diff] [review]
Patch with updated comments

Important crashes, taking it.
Should be in 42 beta 9.
Attachment #8676471 - Flags: approval-mozilla-beta?
Attachment #8676471 - Flags: approval-mozilla-beta+
Attachment #8676471 - Flags: approval-mozilla-aurora?
Attachment #8676471 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/88b2ba444fd9
status-firefox43: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/aac4b4aaa5f4
status-firefox42: affected → fixed
backed out from beta for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=596415&repo=mozilla-beta
Flags: needinfo?(aklotz)

Comment 26

2 years ago
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.
Flags: needinfo?(sledru)
status-firefox42: fixed → affected
Flags: needinfo?(sledru)
(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.
Flags: needinfo?(aklotz)
https://hg.mozilla.org/releases/mozilla-beta/rev/45ab7cdffbb4

since it had approval per comment #22 and aaron confirmed this is the right approach
status-firefox42: affected → fixed
Depends on: 1217565
Depends on: 1218473
No longer depends on: 1218473
You need to log in before you can comment on or make changes to this bug.