Bug 96352

Summary: [Chromium] Fix crash in WebFrameImpl::loadHistoryItem
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Page LoadingAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, japhet, mihai, webkit.review.bot
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch 2 none

Description Kent Tamura 2012-09-10 22:06:03 PDT
[Chromium] Fix crash in WebFrameImpl::loadHistoryItem
Comment 1 Kent Tamura 2012-09-10 22:08:28 PDT
It seems we access both of 'this' and 'otherItem' at HistoryItem.cpp::555. So I'm not sure which is NULL.

0xb51f7b00	 [chrome]	 - third_party/WebKit/Source/WebCore/history/HistoryItem.cpp:555]	WebCore::HistoryItem::shouldDoSameDocumentNavigationTo
0xb4ba6875	 [chrome]	 - third_party/WebKit/Source/WebKit/chromium/src/WebFrameImpl.cpp:1019]	WebKit::WebFrameImpl::loadHistoryItem
0xb615f962	 [chrome]	 - content/renderer/render_view_impl.cc:1059]	RenderViewImpl::OnNavigate
0xb6169853	 [chrome]	 - ./base/tuple.h:546]	RenderViewImpl::OnMessageReceived
0xb499886f	 [chrome]	 - content/common/message_router.cc:47]	MessageRouter::RouteMessage
0xb49987c3	 [chrome]	 - content/common/message_router.cc:39]	MessageRouter::OnMessageReceived
0xb4923539	 [chrome]	 - content/common/child_thread.cc:275]	ChildThread::OnMessageReceived
0xb42e6f39	 [chrome]	 - ipc/ipc_channel_proxy.cc:257]	IPC::ChannelProxy::Context::OnDispatchMessage
0xb42e69cc	 [chrome]	 - ./base/bind_internal.h:190]	base::internal::Invoker<2, base::internal::BindState<base::internal::RunnableAdapter<void (IPC::ChannelProxy::Context::*)(const IPC::Message&)>, void(IPC::ChannelProxy::Context*, const IPC::Message&), void(IPC::ChannelProxy::Context*, IPC::Message)>, void(IPC::ChannelProxy::Context*, const IPC::Message&)>::Run
0xb4281af8	 [chrome]	 - ./base/callback.h:388]	MessageLoop::RunTask
0xb428238b	 [chrome]	 - base/message_loop.cc:472]	MessageLoop::DeferOrRunPendingTask
0xb428291e	 [chrome]	 - base/message_loop.cc:648]	MessageLoop::DoWork
0xb4285eff	 [chrome]	 - base/message_pump_default.cc:28]	base::MessagePumpDefault::Run
0xb427f002	 [chrome]	 - base/message_loop.cc:419]	MessageLoop::RunInternal
0xb42944e4	 [chrome]	 - base/run_loop.cc:45]	base::RunLoop::Run
0xb427ed69	 [chrome]	 - base/message_loop.cc:299]	MessageLoop::Run
0xb617f458	 [chrome]	 - content/renderer/renderer_main.cc:224]	RendererMain
0xb42077fc	 [chrome]	 - content/app/content_main_runner.cc:331]	content::RunZygote
0xb4207c9c	 [chrome]	 - content/app/content_main_runner.cc:634]	content::ContentMainRunnerImpl::Run
0xb42063f7	 [chrome]	 - content/app/content_main.cc:35]	content::ContentMain
0xb3a01eba	 [chrome]	 - chrome/app/chrome_main.cc:32]	ChromeMain
0xb3a01e5d	 [chrome]	 - chrome/app/chrome_exe_main_gtk.cc:18]	main
Comment 3 Kent Tamura 2012-09-10 22:33:05 PDT
Created attachment 163281 [details]
Patch
Comment 4 Adam Barth 2012-09-18 21:50:12 PDT
Comment on attachment 163281 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:12
> +        We don't have reproducible steps and we're not sure which of hisotryItem

hisotryItem -> historyItem
Comment 5 Adam Barth 2012-09-18 21:54:28 PDT
Comment on attachment 163281 [details]
Patch

I worry that this patch is papering over the real problem.  It sounds like this crash was a regression at some point (or at least it started occurring somewhat recently).  Without a repro, it can be hard to narrow the regression range, but have we tried?
Comment 6 Kent Tamura 2012-09-18 22:28:16 PDT
(In reply to comment #5)
> (From update of attachment 163281 [details])
> I worry that this patch is papering over the real problem.  It sounds like this crash was a regression at some point (or at least it started occurring somewhat recently).  Without a repro, it can be hard to narrow the regression range, but have we tried?

We don't have a repro and this crash started at Google Chrome 10 AFAIK.  I don't think investigating all of WebKit/Chromium changes between Google Chrome 9 and 10 is doable.

BTW, Is it possible that m_frame->loader()->history()->currentItem() returns 0?
Comment 7 Adam Barth 2012-09-18 22:37:38 PDT
> BTW, Is it possible that m_frame->loader()->history()->currentItem() returns 0?

I'm not sure who understands HistoryController the best.  I factored it out of FrameLoader a while back, but I don't really understand it completely.
Comment 8 Kent Tamura 2012-09-18 22:55:46 PDT
Created attachment 164664 [details]
Patch 2
Comment 9 Kent Tamura 2012-09-18 22:57:21 PDT
(In reply to comment #7)
> > BTW, Is it possible that m_frame->loader()->history()->currentItem() returns 0?
> 
> I'm not sure who understands HistoryController the best.  I factored it out of FrameLoader a while back, but I don't really understand it completely.

It seems it can be null.  HistoryController has a lot of null check for m_currentItem.
I updated the patch so that it checkes only currentItem.
Comment 10 Adam Barth 2012-09-18 22:59:34 PDT
Comment on attachment 164664 [details]
Patch 2

Ok. This patch seems better than the previous one.  It's aways worrying when we ASSERT one thing and then handle the case where the ASSERT fails.  Obviously, I'd much prefer if we had a regression test for this issue.  Perhaps we can investigate when currentItem() returns 0 and build a test case out of that?  I'm going to mark this patch r+, but please consider working on a test case.
Comment 11 WebKit Review Bot 2012-09-19 00:20:18 PDT
Comment on attachment 164664 [details]
Patch 2

Clearing flags on attachment: 164664

Committed r128972: <http://trac.webkit.org/changeset/128972>
Comment 12 WebKit Review Bot 2012-09-19 00:20:21 PDT
All reviewed patches have been landed.  Closing bug.