96352 2012-09-10 22:06:03 -0700 [Chromium] Fix crash in WebFrameImpl::loadHistoryItem 2012-09-19 00:20:21 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P1 Normal --- 1 tkent tkent abarth japhet mihaip webkit.review.bot oldest_to_newest 716841 0 tkent 2012-09-10 22:06:03 -0700 [Chromium] Fix crash in WebFrameImpl::loadHistoryItem 716842 1 tkent 2012-09-10 22:08:28 -0700 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 716856 2 tkent 2012-09-10 22:32:25 -0700 http://code.google.com/p/chromium/issues/detail?id=147363 716857 3 163281 tkent 2012-09-10 22:33:05 -0700 Created attachment 163281 Patch 723256 4 163281 abarth 2012-09-18 21:50:12 -0700 Comment on attachment 163281 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 723259 5 163281 abarth 2012-09-18 21:54:28 -0700 Comment on attachment 163281 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? 723272 6 tkent 2012-09-18 22:28:16 -0700 (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? 723275 7 abarth 2012-09-18 22:37:38 -0700 > 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. 723291 8 164664 tkent 2012-09-18 22:55:46 -0700 Created attachment 164664 Patch 2 723294 9 tkent 2012-09-18 22:57:21 -0700 (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. 723297 10 164664 abarth 2012-09-18 22:59:34 -0700 Comment on attachment 164664 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. 723338 11 164664 webkit.review.bot 2012-09-19 00:20:18 -0700 Comment on attachment 164664 Patch 2 Clearing flags on attachment: 164664 Committed r128972: <http://trac.webkit.org/changeset/128972> 723339 12 webkit.review.bot 2012-09-19 00:20:21 -0700 All reviewed patches have been landed. Closing bug. 163281 2012-09-10 22:33:05 -0700 2012-09-18 22:55:42 -0700 Patch bug-96352-20120911143223.patch text/plain 2209 tkent U3VidmVyc2lvbiBSZXZpc2lvbjogMTI4MTQ0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IGQyNmRkODAxMWE1ZGQxMzE2ZWE2ODBkZGNkZTY2MGJkNjlmZmI2NGUuLjY5MjYwZGUxZTNiNmNh ZWNmMWY1YjE1NDgxOGQ3OGI1Mjk4MTgzNTQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMjIgQEAKKzIwMTItMDktMTAgIEtlbnQgVGFtdXJhICA8dGtlbnRAY2hyb21pdW0u b3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gRml4IGNyYXNoIGluIFdlYkZyYW1lSW1wbDo6bG9h ZEhpc3RvcnlJdGVtCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD05NjM1MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlIGhhdmUgbWFueSBjcmFzaCByZXBvcnRzIHdpdGggdGhlIGZvbGxvd2luZyBzdGFjazoK KyAgICAgICAgICAtIEhpc3RvcnlJdGVtOjpzaG91bGREb1NhbWVEb2N1bWVudE5hdmlnYXRpb25U by4KKyAgICAgICAgICAtIFdlYkZyYW1lSW1wbDo6bG9hZEhpc3RvcnlJdGVtCisgICAgICAgICAg Li4uCisgICAgICAgIFdlIGRvbid0IGhhdmUgcmVwcm9kdWNpYmxlIHN0ZXBzIGFuZCB3ZSdyZSBu b3Qgc3VyZSB3aGljaCBvZiBoaXNvdHJ5SXRlbQorICAgICAgICBhbmQgY3VycmVudEl0ZW0gaXMg bnVsbC4gQW55d2F5IHRoaXMgY2hhbmdlIHRyaWVzIHRvIGZpeCB0aGUgbnVsbAorICAgICAgICBw b2ludGVyIGRlcmVmZXJlbmNlIGluIEhpc3RvcnlJdGVtOjpzaG91bGREb1NhbWVEb2N1bWVudE5h dmlnYXRpb25Uby4KKworICAgICAgICAqIHNyYy9XZWJGcmFtZUltcGwuY3BwOgorICAgICAgICAo V2ViS2l0OjpXZWJGcmFtZUltcGw6OmxvYWRIaXN0b3J5SXRlbSk6CisgICAgICAgIENoZWNrIG51 bGxuZXNzIG9mIGhpc3RvcnlJdGVtIGFuZCBjdXJyZW50SXRlbS4KKwogMjAxMi0wOS0xMCAgSm9h bm1hcmllIERpZ2dzICA8amRpZ2dzQGlnYWxpYS5jb20+CiAKICAgICAgICAgUHJvcGVybHkgZXhw b3NlIDxsZWdlbmQ+IGVsZW1lbnRzIHRvIEFUcwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9j aHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3Jj L1dlYkZyYW1lSW1wbC5jcHAKaW5kZXggMWQxODdmMDllMDAwZjU5MzNmMzA0OTEwMGViMDgzMjMy YmMyMGI0Ny4uZWJmNzc4YjgwM2JiZjllZWIwMzcyMTNiNGY2MTgxZWVlOWYxMjEwZSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYktpdC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcAorKysgYi9T b3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUltcGwuY3BwCkBAIC0xMDI2LDEwICsx MDI2LDEyIEBAIHZvaWQgV2ViRnJhbWVJbXBsOjpsb2FkSGlzdG9yeUl0ZW0oY29uc3QgV2ViSGlz dG9yeUl0ZW0mIGl0ZW0pCiB7CiAgICAgUmVmUHRyPEhpc3RvcnlJdGVtPiBoaXN0b3J5SXRlbSA9 IFBhc3NSZWZQdHI8SGlzdG9yeUl0ZW0+KGl0ZW0pOwogICAgIEFTU0VSVChoaXN0b3J5SXRlbSk7 CisgICAgaWYgKCFoaXN0b3J5SXRlbSkKKyAgICAgICAgcmV0dXJuOwogCiAgICAgbV9mcmFtZS0+ bG9hZGVyKCktPnByZXBhcmVGb3JIaXN0b3J5TmF2aWdhdGlvbigpOwogICAgIFJlZlB0cjxIaXN0 b3J5SXRlbT4gY3VycmVudEl0ZW0gPSBtX2ZyYW1lLT5sb2FkZXIoKS0+aGlzdG9yeSgpLT5jdXJy ZW50SXRlbSgpOwotICAgIG1faW5TYW1lRG9jdW1lbnRIaXN0b3J5TG9hZCA9IGN1cnJlbnRJdGVt LT5zaG91bGREb1NhbWVEb2N1bWVudE5hdmlnYXRpb25UbyhoaXN0b3J5SXRlbS5nZXQoKSk7Cisg ICAgbV9pblNhbWVEb2N1bWVudEhpc3RvcnlMb2FkID0gY3VycmVudEl0ZW0gJiYgY3VycmVudEl0 ZW0tPnNob3VsZERvU2FtZURvY3VtZW50TmF2aWdhdGlvblRvKGhpc3RvcnlJdGVtLmdldCgpKTsK ICAgICBtX2ZyYW1lLT5wYWdlKCktPmdvVG9JdGVtKGhpc3RvcnlJdGVtLmdldCgpLAogICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgRnJhbWVMb2FkVHlwZUluZGV4ZWRCYWNrRm9yd2FyZCk7 CiAgICAgbV9pblNhbWVEb2N1bWVudEhpc3RvcnlMb2FkID0gZmFsc2U7Cg== 164664 2012-09-18 22:55:46 -0700 2012-09-19 00:20:18 -0700 Patch 2 bug-96352-20120919145526.patch text/plain 2000 tkent U3VidmVyc2lvbiBSZXZpc2lvbjogMTI4OTU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmluZGV4 IDM1Mzk5ZDc2OWFlYTliOGQ2NDA2ZGUxOTFkNDgxN2ZjNDlmZjM3ODEuLmUyZDk1ZWVmMzIwM2Fl NTcxODg0NWI1NWYwNmIyN2VhOWEyZGJjYzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCkBA IC0xLDMgKzEsMjMgQEAKKzIwMTItMDktMTAgIEtlbnQgVGFtdXJhICA8dGtlbnRAY2hyb21pdW0u b3JnPgorCisgICAgICAgIFtDaHJvbWl1bV0gRml4IGNyYXNoIGluIFdlYkZyYW1lSW1wbDo6bG9h ZEhpc3RvcnlJdGVtCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD05NjM1MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgIFdlIGhhdmUgc29tZSBjcmFzaCByZXBvcnRzIHdpdGggdGhlIGZvbGxvd2luZyBzdGFjazoK KyAgICAgICAgICAtIEhpc3RvcnlJdGVtOjpzaG91bGREb1NhbWVEb2N1bWVudE5hdmlnYXRpb25U by4KKyAgICAgICAgICAtIFdlYkZyYW1lSW1wbDo6bG9hZEhpc3RvcnlJdGVtCisgICAgICAgICAg Li4uCisKKyAgICAgICAgV2UgZG9uJ3QgaGF2ZSByZXByb2R1Y2libGUgc3RlcHMsIGFuZCBub3Qg c3VyZSB3aGF0J3MgdGhlIHJvb3QKKyAgICAgICAgY2F1c2UuIEFueXdheSB3ZSBzaG91bGQgY2hl Y2sgbnVsbG5lc3Mgb2YgY3VycmVudEl0ZW0gYmVjYXVzZQorICAgICAgICBIaXN0b3J5Q29udHJv bGxlcjo6bV9jdXJyZW50SXRlbSBjYW4gYmUgMC4KKworICAgICAgICAqIHNyYy9XZWJGcmFtZUlt cGwuY3BwOgorICAgICAgICAoV2ViS2l0OjpXZWJGcmFtZUltcGw6OmxvYWRIaXN0b3J5SXRlbSk6 CisgICAgICAgIENoZWNrIG51bGxuZXNzIG9mIGN1cnJlbnRJdGVtLgorCiAyMDEyLTA5LTE4ICBB bHBoYSBMYW0gIDxoY2xhbUBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgVW5yZXZpZXdlZCwgcm9s bGluZyBvdXQgcjEyODkzOS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vc3Jj L1dlYkZyYW1lSW1wbC5jcHAgYi9Tb3VyY2UvV2ViS2l0L2Nocm9taXVtL3NyYy9XZWJGcmFtZUlt cGwuY3BwCmluZGV4IDQ3ODU0YzhjMDkwNWUwMDdjZTY4M2I5NGRhYjI1YjFhMDAzYjI3OTAuLjZk NjhhNmYyM2UzN2Q1NTIwZTBlNjM1ZTVjMGVmZmIyYzdmY2FhNmUgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJLaXQvY2hyb21pdW0vc3JjL1dlYkZyYW1lSW1wbC5jcHAKKysrIGIvU291cmNlL1dlYktp dC9jaHJvbWl1bS9zcmMvV2ViRnJhbWVJbXBsLmNwcApAQCAtMTAyOSw3ICsxMDI5LDcgQEAgdm9p ZCBXZWJGcmFtZUltcGw6OmxvYWRIaXN0b3J5SXRlbShjb25zdCBXZWJIaXN0b3J5SXRlbSYgaXRl bSkKIAogICAgIG1fZnJhbWUtPmxvYWRlcigpLT5wcmVwYXJlRm9ySGlzdG9yeU5hdmlnYXRpb24o KTsKICAgICBSZWZQdHI8SGlzdG9yeUl0ZW0+IGN1cnJlbnRJdGVtID0gbV9mcmFtZS0+bG9hZGVy KCktPmhpc3RvcnkoKS0+Y3VycmVudEl0ZW0oKTsKLSAgICBtX2luU2FtZURvY3VtZW50SGlzdG9y eUxvYWQgPSBjdXJyZW50SXRlbS0+c2hvdWxkRG9TYW1lRG9jdW1lbnROYXZpZ2F0aW9uVG8oaGlz dG9yeUl0ZW0uZ2V0KCkpOworICAgIG1faW5TYW1lRG9jdW1lbnRIaXN0b3J5TG9hZCA9IGN1cnJl bnRJdGVtICYmIGN1cnJlbnRJdGVtLT5zaG91bGREb1NhbWVEb2N1bWVudE5hdmlnYXRpb25Ubyho aXN0b3J5SXRlbS5nZXQoKSk7CiAgICAgbV9mcmFtZS0+cGFnZSgpLT5nb1RvSXRlbShoaXN0b3J5 SXRlbS5nZXQoKSwKICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIEZyYW1lTG9hZFR5cGVJ bmRleGVkQmFja0ZvcndhcmQpOwogICAgIG1faW5TYW1lRG9jdW1lbnRIaXN0b3J5TG9hZCA9IGZh bHNlOwo=