Bug 96352 – [Chromium] Fix crash in WebFrameImpl::loadHistoryItem

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

[Kent Tamura]

[Chromium] Fix crash in WebFrameImpl::loadHistoryItem

[Kent Tamura]

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

[Kent Tamura]

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

[Kent Tamura]

Created attachment 163281 [details]
Patch

[Adam Barth]

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

[Adam Barth]

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?

[Kent Tamura]

(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?

[Adam Barth]

> 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.

[Kent Tamura]

Created attachment 164664 [details]
Patch 2

[Kent Tamura]

(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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 164664 [details]
Patch 2

Clearing flags on attachment: 164664

Committed r128972: <http://trac.webkit.org/changeset/128972>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.