146842 2015-07-10 11:06:40 -0700 Crash in HistoryController::updateForCommit dereferencing a null HistoryItem 2015-07-10 12:03:17 -0700 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 beidson beidson cdumez commit-queue japhet oldest_to_newest 1108685 0 beidson 2015-07-10 11:06:40 -0700 We're seeing crashes in HistoryController::updateForCommit dereferencing a null HistoryItem 38 WebCore: WebCore::HistoryController::updateForCommit() <== 30 WebCore: WebCore::HistoryController::updateForCommit() | 30 WebCore: WebCore::FrameLoader::transitionToCommitted(WebCore::CachedPage*) | 30 WebCore: WebCore::FrameLoader::commitProvisionalLoad() | 30 WebCore: WebCore::DocumentLoader::commitLoad(char const*, int) | 28 WebCore: WebCore::CachedRawResource::notifyClientsDataWasReceived(char const*, unsigned int) | | 28 WebCore: WebCore::CachedRawResource::addDataBuffer(WebCore::SharedBuffer&) | | 28 WebCore: WebCore::SubresourceLoader::didReceiveDataOrBuffer(char const*, int, WTF::PassRefPtr<WebCore::SharedBuffer>, long long, WebCore::DataPayloadType) | | 28 WebCore: WebCore::SubresourceLoader::didReceiveData(char const*, unsigned int, long long, WebCore::DataPayloadType) | | 28 WebKit: WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) The problem is when setCurrentItem(*m_provisionalItem) executes, but m_provisionalItem is null. In updateForCommit there's been a long standing "ASSERT(m_provisionalItem)", but it is still definitely null sometimes. For many years, setting the current item to null in release builds has been a thing. As of http://trac.webkit.org/changeset/179472 we stopped successfully handing the nullptr and started crashing instead. While we want to get to the bottom of why the provisional item might be null (which is why we have the ASSERT there), we should restore the previous "handles null" behavior to get rid of the crashes. <rdar://problem/21371589> 1108689 1 256593 beidson 2015-07-10 11:18:39 -0700 Created attachment 256593 Patch v1 1108692 2 256593 cdumez 2015-07-10 11:23:35 -0700 Comment on attachment 256593 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=256593&action=review > Source/WebCore/loader/FrameLoader.cpp:3184 > + history().setCurrentItem(&item); Here it cannot be null. > Source/WebCore/loader/HistoryController.cpp:474 > + setCurrentItem(m_provisionalItem.get()); Here, we would hit the assert on the previous line if it were null. > Source/WebCore/loader/HistoryController.cpp:515 > + setCurrentItem(m_provisionalItem.get()); Here, we already made sure it isn't null at the beginning of the method. > Source/WebCore/loader/HistoryController.cpp:564 > + setCurrentItem(m_provisionalItem.get()); Here, we already made sure it isn't null at the beginning of the method. > Source/WebCore/loader/HistoryController.cpp:667 > + setCurrentItem(item.ptr()); Here it cannot be null. 1108693 3 beidson 2015-07-10 11:26:58 -0700 (In reply to comment #2) > Comment on attachment 256593 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256593&action=review > > > Source/WebCore/loader/FrameLoader.cpp:3184 > > + history().setCurrentItem(&item); > > Here it cannot be null. I know. > > Source/WebCore/loader/HistoryController.cpp:515 > > + setCurrentItem(m_provisionalItem.get()); > > Here, we already made sure it isn't null at the beginning of the method. I know. > > Source/WebCore/loader/HistoryController.cpp:564 > > + setCurrentItem(m_provisionalItem.get()); > > Here, we already made sure it isn't null at the beginning of the method. I know. > > Source/WebCore/loader/HistoryController.cpp:667 > > + setCurrentItem(item.ptr()); > > Here it cannot be null. I know. > > Source/WebCore/loader/HistoryController.cpp:474 > > + setCurrentItem(m_provisionalItem.get()); > > Here, we would hit the assert on the previous line if it were null. Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null. This is not theoretical - It's definitely happening in release builds a non-trivial amount. 1108698 4 256593 cdumez 2015-07-10 11:34:52 -0700 Comment on attachment 256593 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=256593&action=review r=me to get rid of the crash short-term but it may be papering over a real bug :/ >>> Source/WebCore/loader/HistoryController.cpp:474 >>> + setCurrentItem(m_provisionalItem.get()); >> >> Here, we would hit the assert on the previous line if it were null. > > Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null. > > This is not theoretical - It's definitely happening in release builds a non-trivial amount. Then, I think we should add a FIXME comment indicating that our assertion above doesn't always hold true and that it should be investigated why. 1108701 5 beidson 2015-07-10 11:41:22 -0700 (In reply to comment #4) > Comment on attachment 256593 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=256593&action=review > > r=me to get rid of the crash short-term but it may be papering over a real > bug :/ There's definitely a real bug, and it's existed since at least 2009 without revealing itself. (That's when the assert was added) > >>> Source/WebCore/loader/HistoryController.cpp:474 > >>> + setCurrentItem(m_provisionalItem.get()); > >> > >> Here, we would hit the assert on the previous line if it were null. > > > > Yes, which is exactly what I described in the problem Description. But in release builds there is no ASSERT and we dereference null. > > > > This is not theoretical - It's definitely happening in release builds a non-trivial amount. > > Then, I think we should add a FIXME comment indicating that our assertion > above doesn't always hold true and that it should be investigated why. K. 1108705 6 beidson 2015-07-10 12:03:17 -0700 https://trac.webkit.org/changeset/186683 256593 2015-07-10 11:18:39 -0700 2015-07-10 11:34:52 -0700 Patch v1 patch text/plain 4979 beidson ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwOTFjYTk1Li43MWQ4MGYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMzMg QEAKIDIwMTUtMDctMTAgIEJyYWR5IEVpZHNvbiAgPGJlaWRzb25AYXBwbGUuY29tPgogCisgICAg ICAgIENyYXNoIGluIEhpc3RvcnlDb250cm9sbGVyOjp1cGRhdGVGb3JDb21taXQgZGVyZWZlcmVu Y2luZyBhIG51bGwgSGlzdG9yeUl0ZW0uCisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yMTM3MTU4 OT4gYW5kIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDY4NDIKKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBObyBuZXcgdGVz dHMgKFVua25vd24gaG93IHRvIHJlcHJvZHVjZSkuCisgICAgICAgIAorICAgICAgICBUaGlzIHBh dGNoIGJhc2ljYWxseSByb2xscyBiYWNrIHBhcnQgb2YgaHR0cDovL3RyYWMud2Via2l0Lm9yZy9j aGFuZ2VzZXQvMTc5NDcyLgorICAgICAgICAKKyAgICAgICAgcjE3OTQ3MiBjaGFuZ2VkIEhpc3Rv cnlDb250cm9sbGVyOjpzZXRDdXJyZW50SXRlbSgpIHRvIHRha2UgYSByZWZlcmVuY2UgaW5zdGVh ZCBvZiBhIHBvaW50ZXIuCisgICAgICAgIFVuZm9ydHVuYXRlbHksIHdlIHNvbWV0aW1lcyBjYWxs IHNldEN1cnJlbnRJdGVtKG51bGxwdHIpLgorICAgICAgICAKKyAgICAgICAgV2UnZCBsaWtlIHRv ICpub3QqIGRvIHRoYXQsIGFuZCB0aGVyZSBhcmUgYXNzZXJ0aW9ucyBpbiBwbGFjZSB0byB0cnkg dG8gY2F0Y2ggd2hlbiB3ZSBkbywKKyAgICAgICAgYnV0IGluIHRoZSBtZWFudGltZSBpdCBpcyBu b3QgdmFsaWQgdG8gZGVyZWZlcmVuY2UgbnVsbHB0ci4KKworICAgICAgICAqIGxvYWRlci9GcmFt ZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpGcmFtZUxvYWRlcjo6bG9hZFNhbWVEb2N1 bWVudEl0ZW0pOgorICAgICAgICAKKyAgICAgICAgKiBsb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6SGlzdG9yeUNvbnRyb2xsZXI6OnVwZGF0ZUZvckNvbW1p dCk6CisgICAgICAgIChXZWJDb3JlOjpIaXN0b3J5Q29udHJvbGxlcjo6cmVjdXJzaXZlVXBkYXRl Rm9yQ29tbWl0KToKKyAgICAgICAgKFdlYkNvcmU6Okhpc3RvcnlDb250cm9sbGVyOjpyZWN1cnNp dmVVcGRhdGVGb3JTYW1lRG9jdW1lbnROYXZpZ2F0aW9uKToKKyAgICAgICAgKFdlYkNvcmU6Okhp c3RvcnlDb250cm9sbGVyOjpzZXRDdXJyZW50SXRlbSk6IFRha2UgYSBwdHIgaW5zdGVhZCBvZiBh IHJlZi4KKyAgICAgICAgKFdlYkNvcmU6Okhpc3RvcnlDb250cm9sbGVyOjpjcmVhdGVJdGVtKToK KyAgICAgICAgKiBsb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIuaDoKKworMjAxNS0wNy0xMCAgQnJh ZHkgRWlkc29uICA8YmVpZHNvbkBhcHBsZS5jb20+CisKICAgICAgICAgQVNTRVJUIHJlc3Rvcmlu ZyBmcm9tIHBhZ2UgY2FjaGUgYXMgRG9jdW1lbnRMb2FkZXIgcmVhdHRhY2hlcyB0byBpdHMgRnJh bWUuCiAgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8yMTc2NjI4Mj4gYW5kIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDY3ODYKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dl YkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9GcmFt ZUxvYWRlci5jcHAKaW5kZXggOTAxYjY3Ni4uYjcyMDFlZSAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIv RnJhbWVMb2FkZXIuY3BwCkBAIC0zMTgxLDcgKzMxODEsNyBAQCB2b2lkIEZyYW1lTG9hZGVyOjps b2FkU2FtZURvY3VtZW50SXRlbShIaXN0b3J5SXRlbSYgaXRlbSkKICAgICBpZiAoRnJhbWVWaWV3 KiB2aWV3ID0gbV9mcmFtZS52aWV3KCkpCiAgICAgICAgIHZpZXctPnNldFdhc1Njcm9sbGVkQnlV c2VyKGZhbHNlKTsKIAotICAgIGhpc3RvcnkoKS5zZXRDdXJyZW50SXRlbShpdGVtKTsKKyAgICBo aXN0b3J5KCkuc2V0Q3VycmVudEl0ZW0oJml0ZW0pOwogICAgICAgICAKICAgICAvLyBsb2FkSW5T YW1lRG9jdW1lbnQoKSBhY3R1YWxseSBjaGFuZ2VzIHRoZSBVUkwgYW5kIG5vdGlmaWVzIGxvYWQg ZGVsZWdhdGVzIG9mIGEgImZha2UiIGxvYWQKICAgICBsb2FkSW5TYW1lRG9jdW1lbnQoaXRlbS51 cmwoKSwgaXRlbS5zdGF0ZU9iamVjdCgpLCBmYWxzZSk7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9sb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvbG9hZGVy L0hpc3RvcnlDb250cm9sbGVyLmNwcAppbmRleCA0NTRkZDBhLi5jMDgzMWJhIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIuY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2xvYWRlci9IaXN0b3J5Q29udHJvbGxlci5jcHAKQEAgLTQ3MSw3ICs0NzEsNyBA QCB2b2lkIEhpc3RvcnlDb250cm9sbGVyOjp1cGRhdGVGb3JDb21taXQoKQogICAgICAgICAvLyBO b3RlIHByZXZpb3VzSXRlbSBtdXN0IGJlIHNldCBiZWZvcmUgd2UgY2xvc2UgdGhlIFVSTCwgd2hp Y2ggd2lsbAogICAgICAgICAvLyBoYXBwZW4gd2hlbiB0aGUgZGF0YSBzb3VyY2UgaXMgbWFkZSBu b24tcHJvdmlzaW9uYWwgYmVsb3cKICAgICAgICAgQVNTRVJUKG1fcHJvdmlzaW9uYWxJdGVtKTsK LSAgICAgICAgc2V0Q3VycmVudEl0ZW0oKm1fcHJvdmlzaW9uYWxJdGVtKTsKKyAgICAgICAgc2V0 Q3VycmVudEl0ZW0obV9wcm92aXNpb25hbEl0ZW0uZ2V0KCkpOwogICAgICAgICBtX3Byb3Zpc2lv bmFsSXRlbSA9IG51bGxwdHI7CiAKICAgICAgICAgLy8gVGVsbCBhbGwgb3RoZXIgZnJhbWVzIGlu IHRoZSB0cmVlIHRvIGNvbW1pdCB0aGVpciBwcm92aXNpb25hbCBpdGVtcyBhbmQKQEAgLTUxMiw3 ICs1MTIsNyBAQCB2b2lkIEhpc3RvcnlDb250cm9sbGVyOjpyZWN1cnNpdmVVcGRhdGVGb3JDb21t aXQoKQogICAgICAgICAgICAgdmlldy0+c2V0V2FzU2Nyb2xsZWRCeVVzZXIoZmFsc2UpOwogCiAg ICAgICAgIC8vIE5vdyBjb21taXQgdGhlIHByb3Zpc2lvbmFsIGl0ZW0KLSAgICAgICAgc2V0Q3Vy cmVudEl0ZW0oKm1fcHJvdmlzaW9uYWxJdGVtKTsKKyAgICAgICAgc2V0Q3VycmVudEl0ZW0obV9w cm92aXNpb25hbEl0ZW0uZ2V0KCkpOwogICAgICAgICBtX3Byb3Zpc2lvbmFsSXRlbSA9IG51bGxw dHI7CiAKICAgICAgICAgLy8gUmVzdG9yZSBmb3JtIHN0YXRlICh3b3JrcyBmcm9tIGN1cnJlbnRJ dGVtKQpAQCAtNTYxLDcgKzU2MSw3IEBAIHZvaWQgSGlzdG9yeUNvbnRyb2xsZXI6OnJlY3Vyc2l2 ZVVwZGF0ZUZvclNhbWVEb2N1bWVudE5hdmlnYXRpb24oKQogICAgICAgICByZXR1cm47CiAKICAg ICAvLyBDb21taXQgdGhlIHByb3Zpc2lvbmFsIGl0ZW0uCi0gICAgc2V0Q3VycmVudEl0ZW0oKm1f cHJvdmlzaW9uYWxJdGVtKTsKKyAgICBzZXRDdXJyZW50SXRlbShtX3Byb3Zpc2lvbmFsSXRlbS5n ZXQoKSk7CiAgICAgbV9wcm92aXNpb25hbEl0ZW0gPSBudWxscHRyOwogCiAgICAgLy8gSXRlcmF0 ZSBvdmVyIHRoZSByZXN0IG9mIHRoZSB0cmVlLgpAQCAtNTc3LDExICs1NzcsMTEgQEAgdm9pZCBI aXN0b3J5Q29udHJvbGxlcjo6dXBkYXRlRm9yRnJhbWVMb2FkQ29tcGxldGVkKCkKICAgICBtX2Zy YW1lTG9hZENvbXBsZXRlID0gdHJ1ZTsKIH0KIAotdm9pZCBIaXN0b3J5Q29udHJvbGxlcjo6c2V0 Q3VycmVudEl0ZW0oSGlzdG9yeUl0ZW0mIGl0ZW0pCit2b2lkIEhpc3RvcnlDb250cm9sbGVyOjpz ZXRDdXJyZW50SXRlbShIaXN0b3J5SXRlbSogaXRlbSkKIHsKICAgICBtX2ZyYW1lTG9hZENvbXBs ZXRlID0gZmFsc2U7CiAgICAgbV9wcmV2aW91c0l0ZW0gPSBtX2N1cnJlbnRJdGVtOwotICAgIG1f Y3VycmVudEl0ZW0gPSAmaXRlbTsKKyAgICBtX2N1cnJlbnRJdGVtID0gaXRlbTsKIH0KIAogdm9p ZCBIaXN0b3J5Q29udHJvbGxlcjo6c2V0Q3VycmVudEl0ZW1UaXRsZShjb25zdCBTdHJpbmdXaXRo RGlyZWN0aW9uJiB0aXRsZSkKQEAgLTY2NCw3ICs2NjQsNyBAQCBSZWY8SGlzdG9yeUl0ZW0+IEhp c3RvcnlDb250cm9sbGVyOjpjcmVhdGVJdGVtKCkKICAgICBpbml0aWFsaXplSXRlbShpdGVtKTsK ICAgICAKICAgICAvLyBTZXQgdGhlIGl0ZW0gZm9yIHdoaWNoIHdlIHdpbGwgc2F2ZSBkb2N1bWVu dCBzdGF0ZQotICAgIHNldEN1cnJlbnRJdGVtKGl0ZW0pOworICAgIHNldEN1cnJlbnRJdGVtKGl0 ZW0ucHRyKCkpOwogICAgIAogICAgIHJldHVybiBpdGVtOwogfQpkaWZmIC0tZ2l0IGEvU291cmNl L1dlYkNvcmUvbG9hZGVyL0hpc3RvcnlDb250cm9sbGVyLmggYi9Tb3VyY2UvV2ViQ29yZS9sb2Fk ZXIvSGlzdG9yeUNvbnRyb2xsZXIuaAppbmRleCA5ZDQxOGZlLi5lZjI0NTIxIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIuaAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9sb2FkZXIvSGlzdG9yeUNvbnRyb2xsZXIuaApAQCAtNzQsNyArNzQsNyBAQCBwdWJs aWM6CiAgICAgdm9pZCB1cGRhdGVGb3JGcmFtZUxvYWRDb21wbGV0ZWQoKTsKIAogICAgIEhpc3Rv cnlJdGVtKiBjdXJyZW50SXRlbSgpIGNvbnN0IHsgcmV0dXJuIG1fY3VycmVudEl0ZW0uZ2V0KCk7 IH0KLSAgICB2b2lkIHNldEN1cnJlbnRJdGVtKEhpc3RvcnlJdGVtJik7CisgICAgdm9pZCBzZXRD dXJyZW50SXRlbShIaXN0b3J5SXRlbSopOwogICAgIHZvaWQgc2V0Q3VycmVudEl0ZW1UaXRsZShj b25zdCBTdHJpbmdXaXRoRGlyZWN0aW9uJik7CiAgICAgYm9vbCBjdXJyZW50SXRlbVNob3VsZEJl UmVwbGFjZWQoKSBjb25zdDsKICAgICBXRUJDT1JFX0VYUE9SVCB2b2lkIHJlcGxhY2VDdXJyZW50 SXRlbShIaXN0b3J5SXRlbSopOwo=