| Summary: | FrameLoader::addExtraFieldsToRequest can crash when called from or after FrameLoader::detachFromParent | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Labour <piman> | ||||||
| Component: | Page Loading | Assignee: | Adam Barth <abarth> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, annulen, ap, brettw, eric, fishd, japhet, jberlin, webkit | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Antoine Labour
2011-05-31 15:01:47 PDT
Thanks for filing! Reproduced similar crash on QtWebkit (2.2 branch) I see how to fix it, but I don't see how to write a test. Created attachment 100722 [details]
Patch
I'm sorry I don't understand this bug well enough to test it. :( 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? > 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 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. (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? 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. 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. 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. Unfortunately, I don't have access to a system with PPAPI Flash, nor do we have support for PPAPI in any WebKit testing harness. Who has access to PPAPI flash? 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.) (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. Yes, anything can happen while we're waiting for a reply to NPP_Destroy. (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 (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.) Comment on attachment 100722 [details]
Patch
Ok. I'll give that a try. Thanks!
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) {
Created attachment 106467 [details]
Patch
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. Comment on attachment 106467 [details]
Patch
OK.
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. Comment on attachment 106467 [details] Patch Clearing flags on attachment: 106467 Committed r94721: <http://trac.webkit.org/changeset/94721> All reviewed patches have been landed. Closing bug. |