Bug 119044 – [Win] Crash after plugin is unloaded.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

I'm frequently getting a crash (access violation reading) in the function _NPN_DeallocateObject, line 159, in WebCore/bridge/npruntime.cpp.

I suspect the reason for this crash is that after a plugin has been unloaded, garbage collection is performed, which is accessing plugin objects which has already been freed when the plugin was unloaded.

Here's the stacktrace of the crash:

 	WebKit.dll!_NPN_DeallocateObject(NPObject * obj=0x085144e8)  Line 159 + 0x5 bytes	C++
 	WebKit.dll!_NPN_ReleaseObject(NPObject * obj=0x085144e8)  Line 150 + 0x9 bytes	C++
 	WebKit.dll!JSC::Bindings::CInstance::~CInstance()  Line 90 + 0xc bytes	C++
 	WebKit.dll!JSC::Bindings::CInstance::`scalar deleting destructor'()  + 0x16 bytes	C++
 	WebKit.dll!WTF::RefCounted<JSC::Bindings::Instance>::deref()  Line 197 + 0x3b bytes	C++
 	WebKit.dll!WTF::derefIfNotNull<JSC::Bindings::Instance>(JSC::Bindings::Instance * ptr=0x12bf5410)  Line 53	C++
 	WebKit.dll!WTF::RefPtr<JSC::Bindings::Instance>::~RefPtr<JSC::Bindings::Instance>()  Line 62 + 0x19 bytes	C++
 	WebKit.dll!JSC::Bindings::RuntimeObject::~RuntimeObject()  + 0x19 bytes	C++
 	WebKit.dll!JSC::Bindings::RuntimeObject::destroy(JSC::JSCell * cell=0x0675af48)  Line 55	C++
 	JavaScriptCore.dll!JSC::MarkedBlock::callDestructor(JSC::JSCell * cell=0x0675af48)  Line 66 + 0x12 bytes	C++
 	JavaScriptCore.dll!JSC::MarkedBlock::specializedSweep<3,1,2>()  Line 90	C++
 	JavaScriptCore.dll!JSC::MarkedBlock::sweepHelper<2>(JSC::MarkedBlock::SweepMode sweepMode=SweepToFreeList)  Line 140 + 0x12 bytes	C++
 	JavaScriptCore.dll!JSC::MarkedBlock::sweep(JSC::MarkedBlock::SweepMode sweepMode=SweepToFreeList)  Line 119 + 0x10 bytes	C++
 	JavaScriptCore.dll!JSC::MarkedAllocator::tryAllocateHelper(unsigned int bytes=16)  Line 35	C++
 	JavaScriptCore.dll!JSC::MarkedAllocator::tryAllocate(unsigned int bytes=16)  Line 66 + 0xc bytes	C++
 	JavaScriptCore.dll!JSC::MarkedAllocator::allocateSlowCase(unsigned int bytes=16)  Line 82 + 0xc bytes	C++
 	WebKit.dll!JSC::MarkedAllocator::allocate(unsigned int bytes=16)  Line 82 + 0xc bytes	C++
 	WebKit.dll!JSC::MarkedSpace::allocateWithNormalDestructor(unsigned int bytes=16)  Line 216	C++
 	WebKit.dll!JSC::Heap::allocateWithNormalDestructor(unsigned int bytes=16)  Line 375	C++
 	WebKit.dll!JSC::allocateCell<WebCore::JSNodeList>(JSC::Heap & heap={...}, unsigned int size=16)  Line 94 + 0xc bytes	C++
 	WebKit.dll!JSC::allocateCell<WebCore::JSNodeList>(JSC::Heap & heap={...})  Line 104 + 0xb bytes	C++
 	WebKit.dll!WebCore::JSNodeList::create(JSC::Structure * structure=0x08bdb568, WebCore::JSDOMGlobalObject * globalObject=0x045fd038, WTF::PassRefPtr<WebCore::NodeList> impl={...})  Line 37 + 0x11 bytes	C++
 	WebKit.dll!WebCore::createWrapper<WebCore::JSNodeList,WebCore::NodeList>(JSC::ExecState * exec=0x04f404d8, WebCore::JSDOMGlobalObject * globalObject=0x045fd038, WebCore::NodeList * node=0x1305cd38)  Line 187 + 0x26 bytes	C++
 	WebKit.dll!WebCore::createNewWrapper<WebCore::JSNodeList,WebCore::NodeList>(JSC::ExecState * exec=0x04f404d8, WebCore::JSDOMGlobalObject * globalObject=0x045fd038, WebCore::NodeList * domObject=0x1305cd38)  Line 213 + 0x11 bytes	C++
 	WebKit.dll!WebCore::toJS(JSC::ExecState * exec=0x04f404d8, WebCore::JSDOMGlobalObject * globalObject=0x045fd038, WebCore::NodeList * impl=0x1305cd38)  Line 275 + 0x15 bytes	C++
 	WebKit.dll!WebCore::jsElementPrototypeFunctionGetElementsByClassName(JSC::ExecState * exec=0x04f404d8)  Line 2174 + 0x30 bytes	C++
 	056c2cef()	
 	JavaScriptCore.dll!cti_op_get_by_id_proto_list()  Line 1829 + 0x20 bytes	C++
 	JavaScriptCore.dll!JSC::call(JSC::ExecState * exec=0x04f40180, JSC::JSValue functionObject={...}, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 40 + 0x3c bytes	C++
 	JavaScriptCore.dll!JSC::functionProtoFuncApply(JSC::ExecState * exec=0x04f40180)  Line 154 + 0x51 bytes	C++
 	044a00ef()	
 	JavaScriptCore.dll!cti_op_get_by_id_proto_list_full()  Line 1841 + 0x1c bytes	C++
 	JavaScriptCore.dll!JSC::Interpreter::executeCall(JSC::ExecState * callFrame=0x045f85a0, JSC::JSObject * function=0x0879e558, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 1052 + 0x27 bytes	C++
 	JavaScriptCore.dll!JSC::call(JSC::ExecState * exec=0x045f85a0, JSC::JSValue functionObject={...}, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 40 + 0x3c bytes	C++
 	WebKit.dll!WebCore::JSMainThreadExecState::call(JSC::ExecState * exec=0x045f85a0, JSC::JSValue functionObject={...}, JSC::CallType callType=CallTypeJS, const JSC::CallData & callData={...}, JSC::JSValue thisValue={...}, const JSC::ArgList & args={...})  Line 56 + 0x29 bytes	C++
 	WebKit.dll!WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext * scriptExecutionContext=0x0c1fdb40, WebCore::Event * event=0x0bcce198)  Line 130 + 0x64 bytes	C++
 	WebKit.dll!WebCore::EventTarget::fireEventListeners(WebCore::Event * event=0x0bcce198, WebCore::EventTargetData * d=0x149b4048, WTF::Vector<WebCore::RegisteredEventListener,1,WTF::CrashOnOverflow> & entry={...})  Line 258 + 0x22 bytes	C++
 	WebKit.dll!WebCore::EventTarget::fireEventListeners(WebCore::Event * event=0x0bcce198)  Line 204 + 0x14 bytes	C++
 	WebKit.dll!WebCore::Node::handleLocalEvents(WebCore::Event * event=0x0bcce198)  Line 2378	C++
 	WebKit.dll!WebCore::EventContext::handleLocalEvents(WebCore::Event * event=0x0bcce198)  Line 58 + 0x24 bytes	C++
 	WebKit.dll!WebCore::EventDispatcher::dispatchEventAtTarget()  Line 168 + 0x32 bytes	C++
 	WebKit.dll!WebCore::EventDispatcher::dispatch()  Line 125 + 0x8 bytes	C++
 	WebKit.dll!WebCore::EventDispatchMediator::dispatchEvent(WebCore::EventDispatcher * dispatcher=0x0026f1bc)  Line 55	C++
 	WebKit.dll!WebCore::EventDispatcher::dispatchEvent(WebCore::Node * node=0x1500c878, WTF::PassRefPtr<WebCore::EventDispatchMediator> mediator={...})  Line 56 + 0x2a bytes	C++
 	WebKit.dll!WebCore::Node::dispatchEvent(WTF::PassRefPtr<WebCore::Event> event={...})  Line 2398 + 0x21 bytes	C++
 	WebKit.dll!WebCore::DOMWindow::dispatchLoadEvent()  Line 1711	C++
 	WebKit.dll!WebCore::Document::dispatchWindowLoadEvent()  Line 3668	C++
 	WebKit.dll!WebCore::Document::implicitClose()  Line 2421	C++
 	WebKit.dll!WebCore::FrameLoader::checkCallImplicitClose()  Line 827	C++
 	WebKit.dll!WebCore::FrameLoader::checkCompleted()  Line 771	C++
 	WebKit.dll!WebCore::FrameLoader::completed()  Line 1077	C++
 	WebKit.dll!WebCore::FrameLoader::checkCompleted()  Line 774	C++
 	WebKit.dll!WebCore::FrameLoader::completed()  Line 1077	C++
 	WebKit.dll!WebCore::FrameLoader::checkCompleted()  Line 774	C++
 	WebKit.dll!WebCore::FrameLoader::loadDone()  Line 716	C++
 	WebKit.dll!WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource * resource=0x184fbfa8)  Line 773	C++
 	WebKit.dll!WebCore::SubresourceLoader::releaseResources()  Line 327	C++
 	WebKit.dll!WebCore::ResourceLoader::didFinishLoading(double finishTime=0.00000000000000000)  Line 346 + 0xf bytes	C++
 	WebKit.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime=0.00000000000000000)  Line 285	C++
 	WebKit.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x084e9410, double finishTime=0.00000000000000000)  Line 500 + 0x18 bytes	C++
 	WebKit.dll!WebCore::ResourceHandleManager::downloadTimerCallback(WebCore::Timer<WebCore::ResourceHandleManager> * timer=0x03fd3de0)  Line 436 + 0x35 bytes	C++
 	WebKit.dll!WebCore::Timer<WebCore::ResourceHandleManager>::fired()  Line 113 + 0x23 bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 129 + 0xf bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 106	C++
 	WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd=0x000e152a, unsigned int message=49794, unsigned int wParam=0, long lParam=0)  Line 110 + 0x8 bytes	C++

Created attachment 207388 [details]
Patch

[Anders Carlsson]

Comment on attachment 207388 [details]
Patch

Plug-in objects should already be invalidated after a plug-in is destroyed. I don't think adding a zero-delay timer solves that particular problem.

Created attachment 212051 [details]
Patch

Updated the patch; perform garbage collection before the plugin is unloaded to make sure GC is done before the plugin is unloaded, instead of delaying the unloading of the plugin.

Without the patch, I'm getting this crash several times a day with normal browsing, but with this patch (or the previous), there has been no crashes.

[Anders Carlsson]

Comment on attachment 212051 [details]
Patch

As I said before: Plug-in objects should already be invalidated after a plug-in is destroyed.

I think you should figure out why this isn't happening.

Created attachment 213071 [details]
Patch

I have investigated why the plugin object is not invalidated, and found that it has been scheduled for garbage collection (state == dead == 1) before the runtime root is invalidated. The runtime root will then skip invalidating the runtime object (now a zombie), and when this object is garbage collect later, after the plugin has been unloaded, we get the crash.

I'm not sure that this is the right patch, but I think it illustrates the problem at hand.

[Darin Adler]

Comment on attachment 213071 [details]
Patch

This is the wrong fix. We need to solve this in some other way. There’s no guarantee that garbage collection will delete any given object, since we use imprecise garbage collection anyway, so we have to deal with this in a way that does not depend on JavaScript object deletion timing.

(In reply to comment #8)
> (From update of attachment 213071 [details])
> This is the wrong fix. We need to solve this in some other way. There’s no guarantee that garbage collection will delete any given object, since we use imprecise garbage collection anyway, so we have to deal with this in a way that does not depend on JavaScript object deletion timing.

Thanks for reviewing :)

I will try to come up with a different fix, maybe using a weak pointer of some sort?

[Anders Carlsson]

I still don't understand why this is an issue. In the PluginView destructor, we call:

    m_parentFrame->script().cleanupScriptObjectsForPlugin(this);

which invalidates all the objects created by that plug-in.This calls RootObject::invalidate() which invalidates all runtime objects using RuntimeObject::invalidate() which nulls out the instance.

I think you should focus your effort on determining if/why that's not working correctly.

(In reply to comment #10)

Thanks for looking into this :)

> I still don't understand why this is an issue. In the PluginView destructor, we call:
> 
>     m_parentFrame->script().cleanupScriptObjectsForPlugin(this);
> 
> which invalidates all the objects created by that plug-in.This calls RootObject::invalidate() which invalidates all runtime objects using RuntimeObject::invalidate() which nulls out the instance.
> 
> I think you should focus your effort on determining if/why that's not working correctly.

I believe some runtime objects are not invalidated, because they have become "zombies". See line 112 in runtime_root.cpp (pasted below), where the invalidate call is skipped if the weak reference is null.
The weak reference is null because the object is in the WeakImpl::Dead state.
If one of these "zombie" objects are garbage collected after the plugin is unloaded, we crash.


103	void RootObject::invalidate()
104	{
105	    if (!m_isValid)
106	        return;
107	
108	    {
109	        HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator end = m_runtimeObjects.end();
110	        for (HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator it = m_runtimeObjects.begin(); it != end; ++it) {
111	            RuntimeObject* runtimeObject = it->value.get();
112	            if (!runtimeObject) // Skip zombies.
113	                continue;
114	            runtimeObject->invalidate();
115	        }
116	
117	        m_runtimeObjects.clear();
118	    }
119

Created attachment 235075 [details]
Patch

[Darin Adler]

Mark, does this change look right to you?

Created attachment 235126 [details]
Patch

(In reply to comment #14)
> Created an attachment (id=235126) [details]
> Patch

Just fixed some incorrect class name references in the changelog.

[Mark Hahnenberg]

Comment on attachment 235126 [details]
Patch

While I'm not familiar with this particular issue, this fix is incorrect. We intentionally do not finalize WeakImpls eagerly. This would be a big regression in GC pause times. 

It is generally not OK to depend upon finalization of WeakImpls happening within any finite amount of time. The correct solution is probably something along the lines of "don't delete anything that the finalizer depends on until the finalizer runs".

(In reply to comment #16)
> (From update of attachment 235126 [details])
> While I'm not familiar with this particular issue, this fix is incorrect. We intentionally do not finalize WeakImpls eagerly. This would be a big regression in GC pause times. 
> 
> It is generally not OK to depend upon finalization of WeakImpls happening within any finite amount of time. The correct solution is probably something along the lines of "don't delete anything that the finalizer depends on until the finalizer runs".

Thanks for reviewing :)

Would it be an acceptable solution to add another virtual method to WeakHandleOwner, say WeakHandleOwner::reaped()?
This method would then be called from WeakBlock::reap(), and WebCore::RootObject would override it, and invalidate the runtime object in its implementation.

Alternatively, another solution not involving JSC, would be to use a weak pointer to the function pointer table (NPClass), so we don't run the risc of accessing a deleted function pointer table.

What do you think?

[Mark Hahnenberg]

> Would it be an acceptable solution to add another virtual method to WeakHandleOwner, say WeakHandleOwner::reaped()?
> This method would then be called from WeakBlock::reap(), and WebCore::RootObject would override it, and invalidate the runtime object in its implementation.

A virtual reap would probably also be too expensive, and it doesn't really make sense. Reaping is only to notify the WeakImpl that it is now dead (and should return nil when asked for its value). It's not a callback to the WeakHandleOwner. That's what finalize is for. Every other client of the WeakImpl API in WebKit/WebCore works without eager finalization. It's just a matter of keeping alive anything the finalizer will need until it runs.

> 
> Alternatively, another solution not involving JSC, would be to use a weak pointer to the function pointer table (NPClass), so we don't run the risc of accessing a deleted function pointer table.

I think this is more of the style of solution we'd like, although I'm not sure of the details. Do you have a way to reproduce this issue? Is it only reproducible on Windows?

[Geoffrey Garen]

Yes, we either need to:

- Not unload the plug-in while we still have references to it;

OR

- NULL out our references to the plug-in when we unload it.

(In reply to comment #18)
> > 
> > Alternatively, another solution not involving JSC, would be to use a weak pointer to the function pointer table (NPClass), so we don't run the risc of accessing a deleted function pointer table.
> 
> I think this is more of the style of solution we'd like, although I'm not sure of the details. Do you have a way to reproduce this issue? Is it only reproducible on Windows?

Yes, I believe it's Windows only.
It can usually be reproduced (after some time) by going back and forth between two or more pages which contains the same plugin.
The plugin will then constantly be loaded and unloaded (as long as no other page contains the plugin).

[Geoffrey Garen]

Another option, which we used to exercise, is simply never to unload a plug-in.

(In reply to comment #21)
> Another option, which we used to exercise, is simply never to unload a plug-in.

Hmm, that's a good point, could also be a performance improvement, I guess :)

[Anders Carlsson]

(In reply to comment #21)
> Another option, which we used to exercise, is simply never to unload a plug-in.

IIRC, The reason we don't do this on Windows is that there are plug-ins that crash if you exit without unloading them (Shockwave for example).

Created attachment 235229 [details]
Patch

(In reply to comment #24)
> Created an attachment (id=235229) [details]
> Patch

Trying again :)

[Build Bot]

Comment on attachment 235229 [details]
Patch

Attachment 235229 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4706709209612288

New failing tests:
media/track/track-long-word-container-sizing.html
media/W3C/video/src/src_reflects_attribute_not_source_elements.html

[Build Bot]

Created attachment 235239 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 235229 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235229&action=review

r=me, but please consider landing my version instead

> Source/WebCore/bridge/runtime_root.cpp:118
>          HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator end = m_runtimeObjects.end();
>          for (HashMap<RuntimeObject*, JSC::Weak<RuntimeObject>>::iterator it = m_runtimeObjects.begin(); it != end; ++it) {
> -            RuntimeObject* runtimeObject = it->value.get();
> -            if (!runtimeObject) // Skip zombies.
> +            // Use the hash key to get the runtime object, since we want to invalidate all runtime objects.
> +            // If we use the weak pointer from the hash value, it might be null, and it will not be invalidated.
> +            // This should be safe since finalized runtime objects are removed from the hash table in the RootObject::finalize() method.
> +            RuntimeObject* runtimeObject = it->key;
> +            if (!runtimeObject)
>                  continue;
>              runtimeObject->invalidate();
>          }

The null check isn’t needed. A HashMap with pointers for keys can’t hold a null, so there is no need to check for it. Here’s the best way to write this:

    // Get the objects from the keys; the values might be nulled.
    // Safe because finalized runtime objects are removed from m_runtimeObjects by RootObject::finalize.
    for (RuntimeObject* runtimeObject : m_runtimeObjects.keys())
        runtimeObject->invalidate();

Created attachment 235293 [details]
Patch

[Build Bot]

Comment on attachment 235293 [details]
Patch

Attachment 235293 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6276248904925184

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 235295 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

(In reply to comment #28)
> (From update of attachment 235229 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235229&action=review
> 
> r=me, but please consider landing my version instead
> 

Thanks for reviewing :) Updated patch.

I was a little bit worried about the mac-wk2 test failures, but other patches seem  to have the same failures, so I don't think it's caused by this patch.

[WebKit Commit Bot]

Comment on attachment 235293 [details]
Patch

Clearing flags on attachment: 235293

Committed r171371: <http://trac.webkit.org/changeset/171371>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.