Bug 52945 – crash @ WebCore::ResourceLoader::didCancel(WebCore::ResourceError const &)

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Page Loading

[raman tenneti]

Logged into my personal yahoo email a/c and opened an email and print it. 

Displays printable version of email in a new window and select printer dialog gets displayed. Click on "Cancel" of that dialog, sometimes it crashes (duplicated it in Chrome build Beta and Dev).


Stack trace:
###########
Thread 0 *CRASHED* ( EXCEPTION_ACCESS_VIOLATION_READ @ 0x000006c4 )

0x0225162f	 [chrome.dll	 - resourceloader.cpp:341]	WebCore::ResourceLoader::didCancel(WebCore::ResourceError const &)
0x02250e03	 [chrome.dll	 - subresourceloader.cpp:231]	WebCore::SubresourceLoader::didCancel(WebCore::ResourceError const &)
0x02251716	 [chrome.dll	 - resourceloader.cpp:364]	WebCore::ResourceLoader::cancel(WebCore::ResourceError const &)
0x022516c1	 [chrome.dll	 - resourceloader.cpp:354]	WebCore::ResourceLoader::cancel()
0x02179605	 [chrome.dll	 - documentloader.cpp:64]	WebCore::cancelAll
0x02179cc6	 [chrome.dll	 - documentloader.cpp:251]	WebCore::DocumentLoader::stopLoading(WebCore::DatabasePolicy)
0x0210316e	 [chrome.dll	 - frameloader.cpp:1707]	WebCore::FrameLoader::stopAllLoaders(WebCore::DatabasePolicy)
0x023fdb04	 [chrome.dll	 - webframeimpl.cpp:962]	WebKit::WebFrameImpl::stopLoading()
0x024112e3	 [chrome.dll	 - chromeclientimpl.cpp:427]	WebKit::ChromeClientImpl::closeWindowSoon()
0x0233860a	 [chrome.dll	 - v8domwindow.cpp:2636]	WebCore::DOMWindowInternal::closeCallback
0x028e4d86	 [chrome.dll	 - builtins.cc:983]	v8::internal::HandleApiCallHelper<0>
0x028e507f	 [chrome.dll	 + 0x00cb507f]	
0x05e4d458			
Thread 1

0x7c90e514	 [ntdll.dll	 + 0x0000e514]	KiFastSystemCallRet
0x7c90df49	 [ntdll.dll	 + 0x0000df49]	NtWaitForMultipleObjects
0x7c80958f	 [kernel32.dll	 + 0x0000958f]	CreateFileMappingA
0x77df8630	 [advapi32.dll	 + 0x00028630]	WmipEventPump
0x7c80b728	 [kernel32.dll	 + 0x0000b728]	BaseThreadStart
Thread 2

0x7c90e514	 [ntdll.dll	 + 0x0000e514]	KiFastSystemCallRet
0x7c90da49	 [ntdll.dll	 + 0x0000da49]	ZwRemoveIoCompletion
0x7c80a7e5	 [kernel32.dll	 + 0x0000a7e5]	GetQueuedCompletionStatus
0x01d0b56b	 [chrome.dll	 - message_pump_win.cc:518]	base::MessagePumpForIO::GetIOItem(unsigned long,base::MessagePumpForIO::IOItem *)
0x01d0b4b7	 [chrome.dll	 - message_pump_win.cc:487]	base::MessagePumpForIO::WaitForIOCompletion(unsigned long,base::MessagePumpForIO::IOHandler *)
0x01d0b45f	 [chrome.dll	 - message_pump_win.cc:465]	base::MessagePumpForIO::DoRunLoop()
0x01d0aefe	 [chrome.dll	 - message_pump_win.cc:51]	base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate *,base::MessagePumpWin::Dispatcher *)
0x01d0ad43	 [chrome.dll	 - message_pump_win.h:80]	base::MessagePumpWin::Run(base::MessagePump::Delegate *)
0x01cf5d13	 [chrome.dll	 - message_loop.cc:258]	MessageLoop::RunInternal()
0x01cf5c91	 [chrome.dll	 - message_loop.cc:230]	MessageLoop::RunHandler()
0x01cf5c3f	 [chrome.dll	 - message_loop.cc:208]	MessageLoop::Run()
0x0270d40d	 [chrome.dll	 - thread.cc:140]	base::Thread::Run(MessageLoop *)
0x0270d4b9	 [chrome.dll	 - thread.cc:164]	base::Thread::ThreadMain()
0x01cfd757	 [chrome.dll	 - platform_thread_win.cc:26]	`anonymous namespace'::ThreadFunc(void *)
0x7c80b728	 [kernel32.dll	 + 0x0000b728]	BaseThreadStart

-------

From [e-mail] noticed the following:

We're crashing in ResourceLoader::didCancel() because we're assuming that ResourceLoader::m_documentLoader is valid. The stack below is from within SubresourceLoader::didCancel(), right before we crash.  We're re-entering the ResourceLoader and finishing it while in the process of cancelling it.

Note that the ResourceLoader is not yet freed (it's RefPtr<> protected). It's just accessing members that it already nulled.


chrome.dll!WebCore::ResourceLoader::releaseResources()  Line 91	C++
chrome.dll!WebCore::ResourceLoader::didFinishLoading(double finishTime=1290207487.7740891)  Line 302 + 0xf bytes	C++
chrome.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime=1290207487.7740891)  Line 188	C++
chrome.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x04e77c20, double finishTime=1290207487.7740891)  Line 435 + 0x18 bytes	C++
chrome.dll!WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader * __formal=0x05e232c0, double finishTime=1290207487.7740891)  Line 191 + 0x2e bytes	C++
chrome.dll!webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(const URLRequestStatus & status={...}, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & security_info="", const base::Time & completion_time={...})  Line 652 + 0x2c bytes	C++
chrome.dll!ResourceDispatcher::OnRequestComplete(int request_id=146, const URLRequestStatus & status={...}, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & security_info="", const base::Time & completion_time={...})  Line 439 + 0x1b bytes	C++
chrome.dll!DispatchToMethod<ResourceDispatcher,void (__thiscall ResourceDispatcher::*)(int,URLRequestStatus const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,base::Time const &),int,URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time>(ResourceDispatcher * obj=0x01d99aa0, void (int, const URLRequestStatus &, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > &, const base::Time &)* method=0x5c00b460, const Tuple4<int,URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time> & arg={...})  Line 573 + 0x23 bytes	C++
chrome.dll!IPC::MessageWithTuple<Tuple4<int,URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time> >::Dispatch<ResourceDispatcher,void (__thiscall ResourceDispatcher::*)(int,URLRequestStatus const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,base::Time const &)>(const IPC::Message * msg=0x060ae928, ResourceDispatcher * obj=0x01d99aa0, void (int, const URLRequestStatus &, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > &, const base::Time &)* func=0x5c00b460)  Line 944 + 0x11 bytes	C++
chrome.dll!ResourceDispatcher::DispatchMessageW(const IPC::Message & message={...})  Line 509 + 0x12 bytes	C++
chrome.dll!ResourceDispatcher::OnMessageReceived(const IPC::Message & message={...})  Line 297	C++
chrome.dll!ChildThread::OnMessageReceived(const IPC::Message & msg={...})  Line 139 + 0x19 bytes	C++
chrome.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const IPC::Message & message={...})  Line 232 + 0x19 bytes	C++
chrome.dll!DispatchToMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),IPC::Message>(IPC::ChannelProxy::Context * obj=0x01de0000, void (const IPC::Message &)* method=0x5ace84b0, const Tuple1<IPC::Message> & arg={...})  Line 554 + 0xf bytes	C++
chrome.dll!RunnableMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),Tuple1<IPC::Message> >::Run()  Line 330 + 0x1e bytes	C++
chrome.dll!MessageLoop::RunTask(Task * task=0x060ae900)  Line 418 + 0xf bytes	C++
chrome.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask & pending_task={...})  Line 430	C++
chrome.dll!MessageLoop::DoWork()  Line 534 + 0xc bytes	C++
chrome.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate * delegate=0x003ff06c)  Line 23 + 0xf bytes	C++
chrome.dll!MessageLoop::RunInternal()  Line 266 + 0x2a bytes	C++
chrome.dll!MessageLoop::RunHandler()  Line 239	C++
chrome.dll!MessageLoop::Run()  Line 217	C++
chrome.dll!IPC::SyncChannel::WaitForReplyWithNestedMessageLoop(IPC::SyncChannel::SyncContext * context=0x01de0000)  Line 476	C++
chrome.dll!IPC::SyncChannel::WaitForReply(IPC::SyncChannel::SyncContext * context=0x01de0000, base::WaitableEvent * pump_messages_event=0x01d90028)  Line 442 + 0x9 bytes	C++
chrome.dll!IPC::SyncChannel::SendWithTimeout(IPC::Message * message=0x04ee8d70, int timeout_ms=-1)  Line 417 + 0x12 bytes	C++
chrome.dll!IPC::SyncChannel::Send(IPC::Message * message=0x04ee8d70)  Line 381 + 0x15 bytes	C++
chrome.dll!ChildThread::Send(IPC::Message * msg=0x04ee8d70)  Line 96 + 0x21 bytes	C++
chrome.dll!RenderThread::Send(IPC::Message * msg=0x04ee8d70)  Line 431 + 0xf bytes	C++
chrome.dll!RenderWidget::Send(IPC::Message * message=0x04ee8d70)  Line 186 + 0x19 bytes	C++
chrome.dll!PrintWebViewHelper::Send(IPC::Message * msg=0x04ee8d70)  Line 254 + 0x1d bytes	C++
chrome.dll!PrintWebViewHelper::GetPrintSettingsFromUser(WebKit::WebFrame * frame=0x04ebd160, int expected_pages_count=2, bool use_browser_overlays=true)  Line 428 + 0xf bytes	C++
chrome.dll!PrintWebViewHelper::Print(WebKit::WebFrame * frame=0x04ebd160, bool script_initiated=true, bool is_preview=false)  Line 145 + 0x14 bytes	C++
chrome.dll!RenderView::Print(WebKit::WebFrame * frame=0x04ebd160, bool script_initiated=true, bool is_preview=false)  Line 5185	C++
chrome.dll!RenderView::printPage(WebKit::WebFrame * frame=0x04ebd160)  Line 1970	C++
chrome.dll!WebKit::ChromeClientImpl::print(WebCore::Frame * frame=0x05c42800)  Line 628 + 0x2a bytes	C++
chrome.dll!WebCore::Chrome::print(WebCore::Frame * frame=0x05c42800)  Line 418 + 0x1c bytes	C++
chrome.dll!WebCore::DOMWindow::print()  Line 901	C++
chrome.dll!WebCore::DOMWindow::finishedLoading()  Line 1587	C++
chrome.dll!WebCore::DocumentLoader::updateLoading()  Line 351	C++
chrome.dll!WebCore::DocumentLoader::removeSubresourceLoader(WebCore::ResourceLoader * loader=0x055d3400)  Line 710	C++
chrome.dll!WebCore::SubresourceLoader::didCancel(const WebCore::ResourceError & error={...})  Line 230	C++
chrome.dll!WebCore::ResourceLoader::cancel(const WebCore::ResourceError & error={...})  Line 378 + 0x1f bytes	C++

[raman tenneti]

Raman Tenneti <[e-mail]>
to	Adam Barth <[e-mail]>
cc	Jim Roskind <[e-mail]>
date	Fri, Jan 21, 2011 at 2:25 PM
subject	Re: crash in webcore ResouceLoader
mailed-by	google.com
hide details 2:25 PM (5 hours ago)
Hi Adam,


If we look at the callstack for the crash, there's usually a correct
point to do the null check.  Oftentimes that's close to the use, but
if something higher on the callstack does't make sense when the
pointer is null, we might want to add the check there instead.

The above is a good idea. Add a check to MainResourceLoader.cpp to check if m_DoocumentLoader is not null, then only call ResourceLoader::didCancel. Also in ResourceLoader:didCancel, return if m_documentLoader is null. If it not null, then access the m_documentLoader and the next steps:

void MainResourceLoader::didCancel(const ResourceError& error)
.....
    if (m_documentLoader)
       ResourceLoader::didCancel(error);
}

void ResourceLoader::didCancel(const ResourceError& error)
{
...
    if (!m_documentLoader)
        return;

    m_documentLoader->cancelPendingSubstituteLoad(this);


I came across the following is the crash while investigating this bug. It is a different stack trace than the one reported by go/crash.

It looks like in the following case we are trying to resume. m_suspended was false and we are asserting it should be true. (I thought I duplicated the bug, but was seeing a different problem).

thanks,
raman

     chrome.dll!WebCore::SuspendableTimer::resume()  Line 72 + 0x24 bytes    C++
     chrome.dll!WebCore::ScriptExecutionContext::resumeActiveDOMObjects()  Line 203 + 0x1c bytes    C++
>    chrome.dll!WebCore::PageGroupLoadDeferrer::~PageGroupLoadDeferrer()  Line 74    C++
     chrome.dll!WebCore::PageGroupLoadDeferrer::`scalar deleting destructor'()  + 0x16 bytes    C++
     chrome.dll!WebKit::WebView::didExitModalLoop()  Line 257 + 0x25 bytes    C++
     chrome.dll!RenderThread::Send(IPC::Message * msg=0x06c49640)  Line 424    C++
     chrome.dll!RenderWidget::Send(IPC::Message * message=0x06c49640)  Line 191 + 0x19 bytes    C++
     chrome.dll!PrintWebViewHelper::Send(IPC::Message * msg=0x06c49640)  Line 283 + 0x1d bytes    C++
     chrome.dll!PrintWebViewHelper::GetPrintSettingsFromUser(WebKit::WebFrame * frame=0x0628b840, int expected_pages_count=1, bool use_browser_overlays=true)  Line 480 + 0xf bytes    C++
     chrome.dll!PrintWebViewHelper::Print(WebKit::WebFrame * frame=0x0628b840, WebKit::WebNode * node=0x00000000, bool script_initiated=true, bool is_preview=false)  Line 167 + 0x15 bytes    C++
     chrome.dll!PrintWebViewHelper::PrintFrame(WebKit::WebFrame * frame=0x0628b840, bool script_initiated=true, bool is_preview=false)  Line 102    C++
     chrome.dll!RenderView::Print(WebKit::WebFrame * frame=0x0628b840, bool script_initiated=true, bool is_preview=false)  Line 5296    C++
     chrome.dll!RenderView::printPage(WebKit::WebFrame * frame=0x0628b840)  Line 2161    C++
     chrome.dll!WebKit::ChromeClientImpl::print(WebCore::Frame * frame=0x06506200)  Line 623 + 0x2a bytes    C++
     chrome.dll!WebCore::Chrome::print(WebCore::Frame * frame=0x06506200)  Line 415 + 0x1c bytes    C++
     chrome.dll!WebCore::DOMWindow::print()  Line 905    C++
     chrome.dll!WebCore::DOMWindow::finishedLoading()  Line 1587    C++
     chrome.dll!WebCore::DocumentLoader::updateLoading()  Line 370    C++
     chrome.dll!WebCore::DocumentLoader::removeSubresourceLoader(WebCore::ResourceLoader * loader=0x06551000)  Line 729    C++
     chrome.dll!WebCore::SubresourceLoader::didFinishLoading(double finishTime=1295645962.2659061)  Line 188    C++
     chrome.dll!WebCore::ResourceLoader::didFinishLoading(WebCore::ResourceHandle * __formal=0x06c73ad0, double finishTime=1295645962.2659061)  Line 439 + 0x18 bytes    C++
     chrome.dll!WebCore::ResourceHandleInternal::didFinishLoading(WebKit::WebURLLoader * __formal=0x07058008, double finishTime=1295645962.2659061)  Line 191 + 0x2e bytes    C++
     chrome.dll!webkit_glue::WebURLLoaderImpl::Context::OnCompletedRequest(const net::URLRequestStatus & status={...}, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & security_info="", const base::Time & completion_time={...})  Line 657 + 0x2c bytes    C++
     chrome.dll!ResourceDispatcher::OnRequestComplete(int request_id=50, const net::URLRequestStatus & status={...}, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & security_info="", const base::Time & completion_time={...})  Line 457 + 0x1b bytes    C++
     chrome.dll!DispatchToMethod<ResourceDispatcher,void (__thiscall ResourceDispatcher::*)(int,net::URLRequestStatus const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,base::Time const &),int,net::URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time>(ResourceDispatcher * obj=0x004e7730, void (int, const net::URLRequestStatus &, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > &, const base::Time &)* method=0x58f7d120, const Tuple4<int,net::URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time> & arg={...})  Line 570 + 0x23 bytes    C++
     chrome.dll!IPC::MessageWithTuple<Tuple4<int,net::URLRequestStatus,std::basic_string<char,std::char_traits<char>,std::allocator<char> >,base::Time> >::Dispatch<ResourceDispatcher,ResourceDispatcher,void (__thiscall ResourceDispatcher::*)(int,net::URLRequestStatus const &,std::basic_string<char,std::char_traits<char>,std::allocator<char> > const &,base::Time const &)>(const IPC::Message * msg=0x0703f5a8, ResourceDispatcher * obj=0x004e7730, ResourceDispatcher * sender=0x004e7730, void (int, const net::URLRequestStatus &, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > &, const base::Time &)* func=0x58f7d120)  Line 928 + 0x11 bytes    C++
     chrome.dll!ResourceDispatcher::DispatchMessageW(const IPC::Message & message={...})  Line 530 + 0x16 bytes    C++
     chrome.dll!ResourceDispatcher::OnMessageReceived(const IPC::Message & message={...})  Line 298    C++
     chrome.dll!ChildThread::OnMessageReceived(const IPC::Message & msg={...})  Line 144 + 0x2d bytes    C++
     chrome.dll!IPC::ChannelProxy::Context::OnDispatchMessage(const IPC::Message & message={...})  Line 255 + 0x19 bytes    C++
     chrome.dll!DispatchToMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),IPC::Message>(IPC::ChannelProxy::Context * obj=0x0052e000, void (const IPC::Message &)* method=0x57b699a0, const Tuple1<IPC::Message> & arg={...})  Line 551 + 0xf bytes    C++
     chrome.dll!RunnableMethod<IPC::ChannelProxy::Context,void (__thiscall IPC::ChannelProxy::Context::*)(IPC::Message const &),Tuple1<IPC::Message> >::Run()  Line 331 + 0x1e bytes    C++
     chrome.dll!MessageLoop::RunTask(Task * task=0x0703f580)  Line 356 + 0xf bytes    C++
     chrome.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask & pending_task={...})  Line 368    C++
     chrome.dll!MessageLoop::DoWork()  Line 558 + 0xc bytes    C++
     chrome.dll!base::MessagePumpDefault::Run(base::MessagePump::Delegate * delegate=0x0037ed40)  Line 23 + 0xf bytes    C++
     chrome.dll!MessageLoop::RunInternal()  Line 331 + 0x2a bytes    C++
     chrome.dll!MessageLoop::RunHandler()  Line 305    C++
     chrome.dll!MessageLoop::Run()  Line 235    C++
     chrome.dll!RendererMain(const MainFunctionParams & parameters={...})  Line 298    C++
     chrome.dll!`anonymous namespace'::RunNamedProcessTypeMain(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & process_type="renderer", const MainFunctionParams & main_function_params={...})  Line 593 + 0x12 bytes    C++
     chrome.dll!ChromeMain(HINSTANCE__ * instance=0x01060000, sandbox::SandboxInterfaceInfo * sandbox_info=0x0037f6d8, wchar_t * command_line_unused=0x003e1d3e)  Line 919 + 0x10 bytes    C++
     chrome.exe!MainDllLoader::Launch(HINSTANCE__ * instance=0x01060000, sandbox::SandboxInterfaceInfo * sbox_info=0x0037f6d8)  Line 280 + 0x1d bytes    C++
     chrome.exe!wWinMain(HINSTANCE__ * instance=0x01060000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ * __formal=0x00000000, HINSTANCE__ * __formal=0x00000000)  Line 46 + 0x10 bytes    C++
     chrome.exe!__tmainCRTStartup()  Line 263 + 0x2c bytes    C
     chrome.exe!wWinMainCRTStartup()  Line 182    C
     kernel32.dll!@BaseThreadInitThunk@12()  + 0x12 bytes    
     ntdll.dll!___RtlUserThreadStart@8()  + 0x27 bytes    
     ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes    
- Show quoted text -

[raman tenneti]

From	Jim Roskind <[e-mail]>
date	Fri, Jan 21, 2011 at 1:50 PM

Interesting.... so if it is all single threaded, and scheduling is the issue... then the thread must (deep down) start to service messages again (effectively yielding to other pending tasks).  Is this part of the webkit model?  Are tasks allowed to suspend in such a way?

Having argued strongly on Chrome *against* nested message loops (places where threads can pause/yield), I'm sad to hear that Webkit would expose such a "feature."

Where would a breakpoint need to be placed in webkit to catch such a re-entrant dispatcher?  Putting a breakpoint there would prove your point (that re-entrancy was facilitated).  It would also make it clear where the null check has to be to ensure that it is after any/all plausible re-entrancy has completed.

Jim

- Show quoted text -

From	Adam Barth <[e-mail]>
date	Fri, Jan 21, 2011 at 2:52 PM

WebKit does run nested message loops because of things like
synchronous XMLHttpRequest.  You'd see them on the stack though.  Much
of WebKit needs to be re-entrant because we call out to JavaScript,
which can call back into WebCore.

Adam

    chrome.dll!WebKit::WebView::didExitModalLoop()  Line 257 + 0x25
bytes    C++

That frame is indicative of a nested message loop.  We had a modal
loop with a bunch of junk on the stack.

Adam

[raman tenneti]

Created attachment 79822 [details]
defensive checks for m_documentLoader being NULL

in FrameLoader.cpp, we check for m_documentLoader before accessing it. Because the code is reentrant, releaseResources could have been called and m_documentLoader is set to 0.

This is a hard bug to duplicate. I was able to duplicate it with the chrome build by cancelling the Print dialog. Wasn't able to reproduce it after adding these checks.

thanks,
raman

[WebKit Review Bot]

Attachment 79822 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 79822 [details]
defensive checks for m_documentLoader being NULL

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

> Source/WebCore/ChangeLog:9
> +        Covered by Chromium browser_tests.

It's much better if we can test changes using only things in webkit.org.  That way regressions don't surprise use down the road.  We've talked about trying to make a layout test for this change.  It's probably worthwhile to explain what you tried here and what trouble you ran into.

> Source/WebCore/loader/ResourceLoader.cpp:84
>      ASSERT(!m_reachedTerminalState);
> +    if (m_reachedTerminalState)
> +        return;

Generally we don't have both an assert and code to handle the opposite of the assert.  If this case occurs, that means the assert is wrong and should be removed.  However, we might try to look at "svn blame" to understand why this assert was added.  It's a clue telling us that something else might be wrong.

[Alexey Proskuryakov]

Comment on attachment 79822 [details]
defensive checks for m_documentLoader being NULL

r- for lack of tests and for tabs.

This looks very much like some bug I investigated recently, so I'd like to think about this in depth. Generally, early returns on null checks should be added very cautiously, because it's hard to tell which class invariants get violated by not running the remainder of the function, and you end up investigating much more mysterious bugs a few months later.

Thanks for posting a lot of detail about your investigation, it's really helpful.

[Alexey Proskuryakov]

I found the bug I've been looking into - it's bug 51357. Do you agree that this is a duplicate?

[raman tenneti]

bug 51357 seems to be similar to this bug. The stack trace in this bug may be slightly different, but the root cause may be the same. I agree it is better to fix the root cause of the problem which will fix different manifestations. Will mark this as a duplicate of https://bugs.webkit.org/show_bug.cgi?id=51357

[raman tenneti]

*** This bug has been marked as a duplicate of bug 51357 ***

[raman tenneti]

(fyi)
The chrome built with the attached patch didn't crash when I opened the following link, whereas Google Chrome 8.0.552.237 crashes.

https://bugs.webkit.org/attachment.cgi?id=77044

thanks,
raman