1201239 - 64-bit crash in mozilla::plugins::PluginAsyncSurrogate::NotifyAsyncInitFailed()

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-77603c0b-889d-4a1b-88b9-6281e2150902.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::plugins::PluginAsyncSurrogate::NotifyAsyncInitFailed() 	dom/plugins/ipc/PluginAsyncSurrogate.cpp
1 	xul.dll 	mozilla::plugins::PluginModuleParent::OnInitFailure() 	dom/plugins/ipc/PluginModuleParent.cpp
2 	xul.dll 	mozilla::plugins::PluginModuleChromeParent::OnProcessLaunched(bool) 	dom/plugins/ipc/PluginModuleParent.cpp
3 	xul.dll 	mozilla::plugins::PluginProcessParent::RunLaunchCompleteTask() 	dom/plugins/ipc/PluginProcessParent.cpp
4 	xul.dll 	mozilla::plugins::PluginProcessParent::WaitUntilConnected(int) 	dom/plugins/ipc/PluginProcessParent.cpp
5 	xul.dll 	mozilla::plugins::PluginAsyncSurrogate::WaitForInit() 	dom/plugins/ipc/PluginAsyncSurrogate.cpp
6 	xul.dll 	mozilla::plugins::PluginAsyncSurrogate::ScriptableHasProperty(NPObject*, void*) 	dom/plugins/ipc/PluginAsyncSurrogate.cpp
7 	xul.dll 	NPObjWrapper_Resolve 	dom/plugins/base/nsJSNPRuntime.cpp
8 	xul.dll 	js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) 	js/src/vm/NativeObject.h
9 	xul.dll 	GetPropertyOperation 	js/src/vm/Interpreter.cpp
10 	xul.dll 	Interpret 	js/src/vm/Interpreter.cpp
11 	xul.dll 	js::RunScript(JSContext*, js::RunState&) 	js/src/vm/Interpreter.cpp
12 	xul.dll 	js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) 	js/src/vm/Interpreter.cpp
13 	xul.dll 	js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) 	js/src/vm/Interpreter.cpp
14 	xul.dll 	JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) 	js/src/jsapi.cpp
15 	xul.dll 	mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) 	obj-firefox/dom/bindings/FunctionBinding.cpp
16 	xul.dll 	mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) 	obj-firefox/dist/include/mozilla/dom/FunctionBinding.h
17 	xul.dll 	nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) 	dom/base/nsGlobalWindow.cpp
18 	xul.dll 	nsGlobalWindow::RunTimeout(nsTimeout*) 	dom/base/nsGlobalWindow.cpp
19 	xul.dll 	nsGlobalWindow::TimerCallback(nsITimer*, void*) 	dom/base/nsGlobalWindow.cpp
20 	xul.dll 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp
21 	xul.dll 	nsTimerEvent::Run() 	xpcom/threads/nsTimerImpl.cpp
22 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
23 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
24 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
25 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
26 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
27 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
28 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
29 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
30 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
31 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
32 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
33 	firefox.exe 	NS_internal_main(int, char**) 	browser/app/nsBrowserApp.cpp
34 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
35 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt/crtw32/startup/crt0.c:255
36 	kernel32.dll 	BaseThreadInitThunk 	
37 	ntdll.dll 	RtlUserThreadStart 	
38 	kernel32.dll 	BasepReportFault 	
39 	kernel32.dll 	BasepReportFault

this crash signature seems to surge in firefox 41.0b6, presumably related to the code-change that landed with bug 1198302

Comment 1

[[:philipp]]

need-infoing aaron klotz about this (as per conversation on irc with dmajor).

Comment 2

[Benjamin Smedberg]

[Tracking Requested - why for this release]: regression/topcrash

Comment 3

[(Away)]

|owner| is null in nsPluginInstanceOwner::NotifyHostAsyncInitFailed().

The spike on b6 is entirely on win64 builds.

Comment 4

[Robert Kaiser]

I see 3 patches in b6 that touched plugin-related stuff: Bug 1177367, bug 1198302, and bug 1185532. The second of those is in PluginAsyncSurrogate::NotifyAsyncInitFailed, which is where we crash here, and the latter is specifically 64bit-related.

Comment 5

[(Away)]

Bug 1198302 was releases/mozilla-beta changeset cc07ed95653e and I don't think that it could have caused a null |owner| when there wasn't one before.

Bug 1185532 was releases/mozilla-beta changeset adad025c19a8 which seems much more likely, given the 64-bit correlation. CCing Bob and since he's on PTO, flagging bsmedberg too.

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Add owner check

I can't speak for why we're seeing this only on 64-bit, but seeing that it's happening, let's add a proper null check.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 8656643 [details] [diff] [review]
Add owner check

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

The handler we're skipping here tells content to stop loading the plugin. I guess if owner is null the sandbox broke plugin init and presumably content already knows?

Comment 8

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Jim Mathies [:jimm] from comment #7)
> Comment on attachment 8656643 [details] [diff] [review]
> Add owner check
> 
> Review of attachment 8656643 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The handler we're skipping here tells content to stop loading the plugin. I
> guess if owner is null the sandbox broke plugin init and presumably content
> already knows?

Yeah if the owner is null then content must have already torn its side down.

Comment 9

[(No longer employed by Mozilla) Aaron Klotz]

Comment on attachment 8656643 [details] [diff] [review]
Add owner check

Approval Request Comment
[Feature/regressing bug #]: sandboxing + async plugin init
[User impact if declined]: crashes if plugin can't load
[Describe test coverage new/current, TreeHerder]: locally
[Risks and why]: None. Simple patch to add null pointer check
[String/UUID change made/needed]: None

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656643 [details] [diff] [review]
Add owner check

Patch seems simple enough and it addresses a top crash on Beta41. Let's uplift before Beta7 goes out to see if it helps.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked as it's a top crash.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a808cd51b754

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a808cd51b754

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a4a86b5a85fd