1107963 - crash in JS::PersistentRooted<JSObject*>::PersistentRooted<JSObject*>(JSContext*, JSObject*)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Liz Henry (:lizzard)]

This bug was filed from the Socorro interface and is 
report bp-c2974926-d138-4a4c-9ba7-091c62141201.
=============================================================

This is a topcrasher for 34.0.5 and 34.0. It first appeared on 2014-04-18. 


crashing thread: 

0 	xul.dll 	JS::PersistentRooted<JSObject*>::PersistentRooted<JSObject*>(JSContext*, JSObject*) 	js/public/RootingAPI.h
1 	xul.dll 	nsJSObjWrapper::nsJSObjWrapper(_NPP*) 	dom/plugins/base/nsJSNPRuntime.cpp
2 	xul.dll 	nsJSObjWrapper::NP_Allocate(_NPP*, NPClass*) 	dom/plugins/base/nsJSNPRuntime.cpp
3 	xul.dll 	mozilla::plugins::parent::_createobject(_NPP*, NPClass*) 	dom/plugins/base/nsNPAPIPlugin.cpp
4 	xul.dll 	nsJSObjWrapper::GetNewOrUsed(_NPP*, JSContext*, JS::Handle<JSObject*>) 	dom/plugins/base/nsJSNPRuntime.cpp
5 	xul.dll 	JSValToNPVariant(_NPP*, JSContext*, JS::Value, _NPVariant*) 	dom/plugins/base/nsJSNPRuntime.cpp
6 	xul.dll 	nsJSObjWrapper::NP_GetProperty(NPObject*, void*, _NPVariant*) 	dom/plugins/base/nsJSNPRuntime.cpp
7 	xul.dll 	mozilla::plugins::parent::_getproperty(_NPP*, NPObject*, void*, _NPVariant*) 	dom/plugins/base/nsNPAPIPlugin.cpp
8 	xul.dll 	mozilla::plugins::PluginScriptableObjectParent::AnswerGetParentProperty(mozilla::plugins::PPluginIdentifierParent*, mozilla::plugins::Variant*, bool*) 	dom/plugins/ipc/PluginScriptableObjectParent.cpp
9 	xul.dll 	mozilla::plugins::PPluginScriptableObjectParent::OnCallReceived(IPC::Message const&, IPC::Message*&) 	obj-firefox/ipc/ipdl/PPluginScriptableObjectParent.cpp
10 	xul.dll 	mozilla::plugins::PPluginModuleParent::OnCallReceived(IPC::Message const&, IPC::Message*&) 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp
11 	xul.dll 	mozilla::ipc::MessageChannel::DispatchInterruptMessage(IPC::Message const&, unsigned int) 	ipc/glue/MessageChannel.cpp
12 	xul.dll 	mozilla::ipc::MessageChannel::InterruptCall(IPC::Message*, IPC::Message*) 	ipc/glue/MessageChannel.cpp
13 	xul.dll 	mozilla::ipc::MessageChannel::Call(IPC::Message*, IPC::Message*) 	ipc/glue/MessageChannel.cpp
14 	xul.dll 	mozilla::plugins::PPluginInstanceParent::CallNPP_Destroy(short*) 	obj-firefox/ipc/ipdl/PPluginInstanceParent.cpp
15 	xul.dll 	mozilla::plugins::PluginInstanceParent::Destroy() 	dom/plugins/ipc/PluginInstanceParent.cpp

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Liz Henry :lizzard from comment #0)
It looks like we are passing null JSContext* when creating the PersistentRooted.  I can't see exactly how this happens but I suspect it's because the plugin is being destroyed at the time.

Note that use of PersistentRooted<JSObject*> in this code got removed in a change that landed in FF36 (changeset e5273d8dcb91 from bug 650161).  I suppose  oneoption is to backport the relevant parts of that change.  I don't see any plugin topcrashers for 36 and 37.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: bug1107963-plugins-crash

Here's a backport to mozilla-beta of the relevant parts of that patch.

Since this is not really my area, johns are you ok with this course of action?

Comment 3

[John Schoenick [:johns]]

Comment on attachment 8534986 [details] [diff] [review]
bug1107963-plugins-crash

bsmedberg should probably make the call on the riskiness of this, but I don't think this is an issue as far as the plugin bits go. The JS-GC bits are voodoo to me, so I can't comment there.

Comment 4

[Benjamin Smedberg]

Comment on attachment 8534986 [details] [diff] [review]
bug1107963-plugins-crash

Plugin changes are inherently risky. If this is a straight backport of something that's already landed, that reduces the risk. I'm going to bounce this back to somebody who hopefully understands it.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 8534986 [details] [diff] [review]
bug1107963-plugins-crash

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

The new code is well tested at this point and it doesn't look like any of the surrounding code has changed -- let's uplift this.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8534986 [details] [diff] [review]
bug1107963-plugins-crash

Approval Request Comment
[Feature/regressing bug #]: Bug 993413
[User impact if declined]: Occasional crashes
[Describe test coverage new/current, TBPL]: This is an uplift of a change that landed on central over a month ago (in changeset e5273d8dcb91)
[Risks and why]: Low
[String/UUID change made/needed]: None

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows zero crashes in Firefox 35 Beta 6 & 8.

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=JS%3A%3APersistentRooted%3CJSObject%2A%3E%3A%3APersistentRooted%3CJSObject%2A%3E%28JSContext%2A%2C+JSObject%2A%29