[Chromium] Fix crash in WebFrameImpl::loadHistoryItem
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
http://code.google.com/p/chromium/issues/detail?id=147363
Created attachment 163281 [details] Patch
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 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?
(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?
> 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.
Created attachment 164664 [details] Patch 2
(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 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 on attachment 164664 [details] Patch 2 Clearing flags on attachment: 164664 Committed r128972: <http://trac.webkit.org/changeset/128972>
All reviewed patches have been landed. Closing bug.