Bug 61810 – FrameLoader::addExtraFieldsToRequest can crash when called from or after FrameLoader::detachFromParent

• Component: Page Loading
• Status: RESOLVED FIXED

[Antoine Labour]

See this crash on Chrome OS, when involving a pepper plugin that navigates away when destroyed. It crashes with an invalid DocumentWriter, because the FrameLoader's DocumentLoader was set to NULL.

0x75df3949	 [chrome	 - third_party/WebKit/Source/WebCore/loader/DocumentWriter.cpp:251]	WebCore::DocumentWriter::deprecatedFrameEncoding
0x75df90dc	 [chrome	 - third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:2730]	WebCore::FrameLoader::addExtraFieldsToRequest
0x75e0bd2b	 [chrome	 - third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1345]	WebCore::FrameLoader::loadURL
0x75e0d25f	 [chrome	 - third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:1318]	WebCore::FrameLoader::loadFrameRequest
0x7570c9b5	 [chrome	 - third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:397]	WebKit::WebPluginContainerImpl::loadFrameRequest
0x762e6460	 [chrome	 - webkit/plugins/ppapi/ppapi_plugin_instance.cc:1203]	webkit::ppapi::PluginInstance::Navigate
0x762f755d	 [chrome	 - webkit/plugins/ppapi/ppb_flash_impl.cc:62]	webkit::ppapi::::Navigate
0x765deeb1	 [chrome	 - ppapi/proxy/ppb_flash_proxy.cc:260]	pp::proxy::PPB_Flash_Proxy::OnMsgNavigate
0x765e020b	 [chrome	 - ./base/tuple.h:751]	pp::proxy::PPB_Flash_Proxy::OnMessageReceived
0x765fd162	 [chrome	 - ppapi/proxy/host_dispatcher.cc:174]	pp::proxy::HostDispatcher::OnMessageReceived
0x756df6a6	 [chrome	 - ipc/ipc_channel_proxy.cc:254]	IPC::ChannelProxy::Context::OnDispatchMessage
0x756e60a6	 [chrome	 - ipc/ipc_sync_channel.cc:110]	IPC::SyncChannel::ReceivedSyncMsgQueue::DispatchMessages
0x756e7102	 [chrome	 - ipc/ipc_sync_channel.cc:276]	IPC::SyncChannel::SendWithTimeout
0x756e37dc	 [chrome	 - ipc/ipc_sync_channel.cc:399]	IPC::SyncChannel::Send
0x765ca18a	 [chrome	 - ppapi/proxy/proxy_channel.cc:80]	pp::proxy::ProxyChannel::Send
0x765fd210	 [chrome	 - ppapi/proxy/host_dispatcher.cc:138]	pp::proxy::HostDispatcher::Send
0x765fb53d	 [chrome	 - ppapi/proxy/ppp_instance_proxy.cc:43]	pp::proxy::::DidDestroy
0x762e7ff3	 [chrome	 - webkit/plugins/ppapi/ppapi_plugin_instance.cc:435]	webkit::ppapi::PluginInstance::Delete
0x76a37b37	 [chrome	 - webkit/plugins/ppapi/ppapi_webplugin_impl.cc:89]	webkit::ppapi::WebPluginImpl::destroy
0x7570a6ce	 [chrome	 - third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:467]	WebKit::WebPluginContainerImpl::~WebPluginContainerImpl
0x7570a77d	 [chrome	 - third_party/WebKit/Source/WebKit/chromium/src/WebPluginContainerImpl.cpp:468]	WebKit::WebPluginContainerImpl::~WebPluginContainerImpl
0x760cb3be	 [chrome	 - third_party/WebKit/Source/JavaScriptCore/wtf/RefCounted.h:141]	WebCore::RenderWidget::resumeWidgetHierarchyUpdates
0x75c93697	 [chrome	 - third_party/WebKit/Source/WebCore/dom/Element.cpp:1022]	WebCore::Element::detach
0x75c60c2a	 [chrome	 - third_party/WebKit/Source/WebCore/dom/ContainerNode.cpp:742]	WebCore::ContainerNode::detach
0x75c78e7f	 [chrome	 - third_party/WebKit/Source/WebCore/dom/Document.cpp:1758]	WebCore::Document::detach
0x75e81fcf	 [chrome	 - third_party/WebKit/Source/WebCore/page/Frame.cpp:271]	WebCore::Frame::setView
0x75e0065f	 [chrome	 - third_party/WebKit/Source/WebCore/loader/FrameLoader.cpp:2653]	WebCore::FrameLoader::detachFromParent
0x7571e63f	 [chrome	 - third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:949]	WebKit::WebViewImpl::close
0x766e1723	 [chrome	 - content/renderer/render_widget.cc:829]	RenderWidget::Close
0x766ce6ee	 [chrome	 - content/renderer/render_view.cc:3927]	RenderView::Close
0x766e191f	 [chrome	 - ./base/tuple.h:541]	RunnableMethod<RenderWidget, void (RenderWidget::*)(), Tuple0>::Run
0x74e6e082	 [chrome	 - base/message_loop.cc:371]	MessageLoop::DoWork
0x74e701c7	 [chrome	 - base/message_pump_default.cc:23]	base::MessagePumpDefault::Run
0x74e69f6d	 [chrome	 - base/message_loop.cc:346]	MessageLoop::RunHandler
0x74e6a1e9	 [chrome	 - base/message_loop.cc:243]	MessageLoop::Run
0x766ed0dc	 [chrome	 - content/renderer/renderer_main.cc:233]	RendererMain
0x744910cc	 [chrome	 - chrome/app/chrome_main.cc:446]	RunZygote
0x744917ef	 [chrome	 - chrome/app/chrome_main.cc:492]	ChromeMain
0x74492414	 [chrome	 - chrome/app/chrome_exe_main_gtk.cc:46]	main
0x73298a95	 [libc-2.10.1.so	 + 0x00016a95]	
0x74490aa0	 [chrome	 + 0x00219aa0]	
0x744923bf	 [chrome	 + 0x0021b3bf]	
0x74266fff	 [ld-2.10.1.so	 + 0x0000efff]

It's a re-entrancy issue, but I'm pretty sure it's a valid use case for a plugin to navigate the page away when destroyed.
It worked before http://trac.webkit.org/changeset/78342 because the DocumentWriter had the lifetime of the FrameLoader.

See also http://code.google.com/p/chromium-os/issues/detail?id=15943

[Adam Barth]

Thanks for filing!

[Konstantin Tokarev]

Reproduced similar crash on QtWebkit (2.2 branch)

[Adam Barth]

I see how to fix it, but I don't see how to write a test.

[Adam Barth]

Created attachment 100722 [details]
Patch

[Adam Barth]

I'm sorry I don't understand this bug well enough to test it.  :(

[Alexey Proskuryakov]

A test could be more valuable than a fix :(

We have another common crasher that could be related, and I'm unwilling to patch it with a null check. Is there something I can do to help investigate this? You said that you had multiple stack traces, could you post these?

[Adam Barth]

> A test could be more valuable than a fix :(

100% agree.

> We have another common crasher that could be related, and I'm unwilling to patch it with a null check. Is there something I can do to help investigate this? You said that you had multiple stack traces, could you post these?

http://code.google.com/p/chromium/issues/detail?id=88721

Chrome Version       : 14.0.815.0 and 13.0.782.41
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
URLs (if applicable) : http://entertainment.oneindia.in/music/international/2010/rihanna-snare-jesse-williams-030610.html

What steps will reproduce the problem?
1. Loaded the above page.
2. Clicked on the wallpapers http://wallpapers.oneindia.in/v/Hollywood/ or clicking on the Rhianna's image

was able to crash multiple times (but not consistently).

The following are the stack traces from 13.0.782.41 (beta)

http://crash/reportdetail?reportid=13eba3f07f4795ec

Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x00000058 )

0x0239fa1a	 [chrome.dll	 - documentwriter.cpp:251	WebCore::DocumentWriter::deprecatedFrameEncoding()
0x0237cabd	 [chrome.dll	 - frameloader.cpp:2772	WebCore::FrameLoader::addExtraFieldsToRequest(WebCore::ResourceRequest &,WebCore::FrameLoadType,bool,bool)
0x0237e058	 [chrome.dll	 - frameloader.cpp:3339	WebCore::FrameLoader::loadDifferentDocumentItem(WebCore::HistoryItem *,WebCore::FrameLoadType)
0x023af642	 [chrome.dll	 - historycontroller.cpp:715	WebCore::HistoryController::recursiveGoToItem(WebCore::HistoryItem *,WebCore::HistoryItem *,WebCore::FrameLoadType)
0x023aec03	 [chrome.dll	 - historycontroller.cpp:271	WebCore::HistoryController::goToItem(WebCore::HistoryItem *,WebCore::FrameLoadType)
0x0231efac	 [chrome.dll	 - page.cpp:348	WebCore::Page::goToItem(WebCore::HistoryItem *,WebCore::FrameLoadType)
0x025dc6e0	 [chrome.dll	 - webframeimpl.cpp:947	WebKit::WebFrameImpl::loadHistoryItem(WebKit::WebHistoryItem const &)
0x01ca27f7	 [chrome.dll	 - render_view.cc:770	RenderView::OnNavigate(ViewMsg_Navigate_Params const &)
0x01cac7a2	 [chrome.dll	 - ipc_message_utils.h:963	IPC::MessageWithTuple<Tuple1<ViewMsg_Navigate_Params> >::Dispatch<RenderView,RenderView,void ( RenderView::*)(ViewMsg_Navigate_Params const &)>(IPC::Message const *,RenderView *,RenderView *,void ( RenderView::*)(ViewMsg_Navigate_Params const &))
0x01ca1f47	 [chrome.dll	 - render_view.cc:597	RenderView::OnMessageReceived(IPC::Message const &)
0x01c8a6e3	 [chrome.dll	 - message_router.cc:46	MessageRouter::RouteMessage(IPC::Message const &)
0x01c8a6bd	 [chrome.dll	 - message_router.cc:38	MessageRouter::OnMessageReceived(IPC::Message const &)
0x01c8462d	 [chrome.dll	 - child_thread.cc:175	ChildThread::OnMessageReceived(IPC::Message const &)
0x01f2ed7c	 [chrome.dll	 - task.h:338	RunnableMethod<DOMStorageMessageFilter,void ( DOMStorageMessageFilter::*)(DOMStorageMsg_Event_Params const &),Tuple1<DOMStorageMsg_Event_Params> >::Run()
0x01c44f95	 [chrome.dll	 - message_loop.cc:102	`anonymous namespace'::TaskClosureAdapter::Run()
0x01c45b35	 [chrome.dll	 - message_loop.cc:482	MessageLoop::RunTask(MessageLoop::PendingTask const &)
0x01c45ba1	 [chrome.dll	 - message_loop.cc:500	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x01c45f1c	 [chrome.dll	 - message_loop.cc:691	MessageLoop::DoWork()
0x01c50584	 [chrome.dll	 - message_pump_default.cc:50	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x01c45a6d	 [chrome.dll	 - message_loop.cc:449	MessageLoop::RunInternal()
0x01c459f2	 [chrome.dll	 - message_loop.cc:422	MessageLoop::RunHandler()
0x01c458e6	 [chrome.dll	 - message_loop.cc:346	MessageLoop::Run()
0x01c9e087	 [chrome.dll	 - renderer_main.cc:235	RendererMain(MainFunctionParams const &)
0x01c348ec	 [chrome.dll	 - chrome_main.cc:823	ChromeMain
0x004022b8	 [chrome.exe	 - client_util.cc:254	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x0040595a	 [chrome.exe	 - chrome_exe_main_win.cc:46	wWinMain
0x00454f9f	 [chrome.exe	 - crt0.c:263	__tmainCRTStartup
0x7c816fe6	 [kernel32.dll	 + 0x00016fe6]	BaseProcessStart


0:000> dv /V
@eax @eax            this = 0x00000058
0:000> dt this
Local var @ 0 Type WebCore::DocumentWriter*
   +0x000 m_frame          : ???? 
   +0x004 m_receivedData   : ??
   +0x008 m_mimeType       : WTF::String
   +0x00c m_encodingWasChosenByUser : ??
   +0x010 m_encoding       : WTF::String
   +0x014 m_decoder        : WTF::RefPtr<WebCore::TextResourceDecoder>
Memory read error 00000064


We had around 40 crashes in Beta (13.0.782.41)

http://chromecrash/samples?q=v%3DChrome-13.0.782.41%2Fptype%3Drenderer%2Fmagicsignature%3DWebCore%3A%3ADocumentWriter%3A%3AdeprecatedFrameEncoding()%2Fmagicsignature%3DWebCore%3A%3ADocumentWriter%3A%3AdeprecatedFrameEncoding()

http://crash/reportdetail?reportid=5b457456ba050768
http://crash/reportdetail?reportid=bd576087505c60f5

[Adam Barth]

Note: There's a pop under involved in that repro, so maybe it's some sort of race between history navigation and closing / opening the popup?  I don't understand how we can be at that line of code without a DocumentLoader.

[Jessie Berlin]

(In reply to comment #0)
> See this crash on Chrome OS, when involving a pepper plugin that navigates away when destroyed. It crashes with an invalid DocumentWriter, because the FrameLoader's DocumentLoader was set to NULL.

Is it only with pepper plugins? Or do NPAPI plugins cause this as well?

[Antoine Labour]

I'm pretty sure a NPAPI plugin could do exactly the same, calling NPN_GetURL with a target of "_self" from NPP_Destroy, which could cause this. I haven't verified though.

[Alexey Proskuryakov]

In general, we ignore any calls from a plug-in that has been issued NPP_Destroy.

If Pepper plug-ins allow that, then there should probably be code to cancel requests created during their destruction. This null crash is the smallest of problems that can arise from orphaned network requests.

[Antoine Labour]

I'm pretty sure some Flash sites rely on being able to redirect you to another page and/or ping the host when the player is destroyed.

[Adam Barth]

Unfortunately, I don't have access to a system with PPAPI Flash, nor do we have support for PPAPI in any WebKit testing harness.

[Eric Seidel (no email)]

Who has access to PPAPI flash?

[Darin Fisher (:fishd, Google)]

I agree with Antoine that we should be able to construct a test case using TestNetscapePlugin and a LayoutTest.  (Comment #2 indicates that it can be replicated without PPAPI too.)

[Darin Fisher (:fishd, Google)]

(In reply to comment #11)
> In general, we ignore any calls from a plug-in that has been issued NPP_Destroy.
> 
> If Pepper plug-ins allow that, then there should probably be code to cancel requests created during their destruction. This null crash is the smallest of problems that can arise from orphaned network requests.

Are you sure we disallow loads initiated *during* NPP_Destroy?  I agree that we should disallow loads initiated *after* NPP_Destroy.

Looking at PluginView::load(), it certainly has a null-check for FrameLoader::documentLoader().  It looks like WebPluginContainerImpl::loadFrameRequest() should have a similar check.

[Alexey Proskuryakov]

Yes, anything can happen while we're waiting for a reply to NPP_Destroy.

[Konstantin Tokarev]

(In reply to comment #15)
> (Comment #2 indicates that it can be replicated without PPAPI too.)

Right, my case has nothing to do with PPAPI. I built QtWebKit-2.2 branch (in its state in the beginning of June) using Qt-4.7.3 Embedded (QWS). I was getting reproducible crashes while trying to open non-existing URL http://torrents.ru

[Darin Fisher (:fishd, Google)]

(In reply to comment #17)
> Yes, anything can happen while we're waiting for a reply to NPP_Destroy.

So, I really think the right fix here is to disallow the URL load when the DocumentLoader is gone just as PluginView.cpp does.  A test case probably has TestNetscapePlugin attempt to call NPN_GetURL after NPP_Destroy.  The test case probably needs to use some mechanism other than NPN_ThreadAsyncCall to schedule the call to NPN_GetURL.  (I can imagine the browser dropping NPN_ThreadAsyncCall callbacks on the floor after NPP_Destroy.)

[Adam Barth]

Comment on attachment 100722 [details]
Patch

Ok.  I'll give that a try.  Thanks!

[Darin Fisher (:fishd, Google)]

likely patch:

Index: WebPluginContainerImpl.cpp
===================================================================
--- WebPluginContainerImpl.cpp  (revision 91186)
+++ WebPluginContainerImpl.cpp  (working copy)
@@ -368,7 +368,7 @@
 void WebPluginContainerImpl::loadFrameRequest(const WebURLRequest& request, const WebString& target, bool notifyNeeded,
 void* notifyData)
 {
     Frame* frame = m_element->document()->frame();
-    if (!frame)
+    if (!frame || !frame->loader()->documentLoader())
         return;  // FIXME: send a notification in this case?

     if (notifyNeeded) {

[Adam Barth]

Created attachment 106467 [details]
Patch

[Eric Seidel (no email)]

I discussed this at length with Adam.  My understanding of this bug is thus:

This is a Flash-only PPAPI bug.  Flash is provided a private PPAPI call 'Navigate' implemented in
proxy/ppb_flash_proxy.cc:int32_t Navigate(PP_Resource request_id,
which uses a synchronous message call back to the rendering process:
proxy/ppapi_messages.h:IPC_SYNC_MESSAGE_ROUTED3_1(PpapiHostMsg_PPBFlash_Navigate,

As far as I can tell, there is no attempt in the code to ignore messages after telling the plugin to destroy (as ap mentioned may be the case for NPAPI plugins in comment #12)

So the renderer gets this Navigate message, makes no attempt to check whether the plugin is being destroyed and then calls loadFrameRequest, which crashes lacking a documentLoader.

Adam's approach of ignoring the call to loadFrameRequest at the WebKit API layer seems reasonable.

The authors of the PPAPI flash API might wish to consider also ignoring these messages earlier as well.

In either case it's impossible to test this in WebKit, as it would require a PPAPI plugin, as well as a plugin with access to the special Flash API as well.

The patch seems reasonable given that understanding.

[Eric Seidel (no email)]

Comment on attachment 106467 [details]
Patch

OK.

[Eric Seidel (no email)]

I should note, GetURL (which is the NPAPI equivalent) is async.  So even if it were possible to call it during Destroy (which ap suggests it may not be in most implementations), it would not be possible to reproduce this bug using GetURL.

[Adam Barth]

Comment on attachment 106467 [details]
Patch

Clearing flags on attachment: 106467

Committed r94721: <http://trac.webkit.org/changeset/94721>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.