165852 2016-12-14 09:11:02 -0800 WebContent crash under WebCore::CachedResource::load in WebCore::FrameLoader::outgoingReferrer const 2016-12-14 10:55:08 -0800 1 1 1 Unclassified WebKit Page Loading WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 koivisto webkit-unassigned beidson cdumez commit-queue dbates japhet oldest_to_newest 1259750 0 koivisto 2016-12-14 09:11:02 -0800 Null ptr 1259751 1 koivisto 2016-12-14 09:11:16 -0800 <rdar://problem/27297153> 1259755 2 297093 koivisto 2016-12-14 09:17:57 -0800 Created attachment 297093 patch 1259761 3 297093 cdumez 2016-12-14 09:24:11 -0800 Comment on attachment 297093 patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review > Source/WebCore/loader/FrameLoader.cpp:929 > // because they need to be contained in iframes with the srcdoc. Getting a null frame would mean that we have a top-level frame with an srcdoc document. This is weird unless the iframe that has the srcdoc is not yet (or no longer in the tree), i.e. detached. 1259770 4 297093 cdumez 2016-12-14 09:27:38 -0800 Comment on attachment 297093 patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review >> Source/WebCore/loader/FrameLoader.cpp:929 >> // because they need to be contained in iframes with the srcdoc. > > Getting a null frame would mean that we have a top-level frame with an srcdoc document. This is weird unless the iframe that has the srcdoc is not yet (or no longer in the tree), i.e. detached. Based on the trace, I suspect this would be reproducible if we: 1. Created an iframe with an srcdoc but not add it to the document 2. Add an HTMLImageElement to that srcdoc 3. Set the src attribute on that HTMLImageElement If so, then this comment and assertion are wrong and your fix is indeed right. 1259792 5 koivisto 2016-12-14 09:53:06 -0800 > If so, then this comment and assertion are wrong and your fix is indeed > right. The theory has been that we shouldn't even try to do any loads in disconnected frames so we shouldn't get here in the first place. That's why I didn't remove the assert. 1259793 6 cdumez 2016-12-14 09:56:22 -0800 (In reply to comment #5) > > If so, then this comment and assertion are wrong and your fix is indeed > > right. > > The theory has been that we shouldn't even try to do any loads in > disconnected frames so we shouldn't get here in the first place. That's why > I didn't remove the assert. I believe that if you set the src of an HTMLImageElement, the load will go through. It goes through even if the HTMLImageElement is itself detached. 1259798 7 koivisto 2016-12-14 10:14:31 -0800 I tried this but it doesn't hit the case: <iframe srcdoc="text" onload="test()"></iframe> <script> function test() { const iframe = document.querySelector("iframe"); const contentDocument = iframe.contentDocument; document.body.removeChild(iframe); const img = contentDocument.createElement("img"); img.setAttribute("src", "foo.png"); } </script> Note that this doesn't necessarily have anything to do with srcdoc, local m_frame could be null. 1259799 8 cdumez 2016-12-14 10:16:23 -0800 (In reply to comment #7) > I tried this but it doesn't hit the case: > > <iframe srcdoc="text" onload="test()"></iframe> > <script> > function test() { > const iframe = document.querySelector("iframe"); > const contentDocument = iframe.contentDocument; > document.body.removeChild(iframe); > const img = contentDocument.createElement("img"); > img.setAttribute("src", "foo.png"); > } > </script> > > Note that this doesn't necessarily have anything to do with srcdoc, local > m_frame could be null. FrameLoader::m_frame is a reference so if it is null, we have bigger issues. 1259807 9 koivisto 2016-12-14 10:27:08 -0800 > FrameLoader::m_frame is a reference so if it is null, we have bigger issues. True, like dead frame loader. Anyway, I don't know how to repro. 1259808 10 297093 cdumez 2016-12-14 10:28:07 -0800 Comment on attachment 297093 patch View in context: https://bugs.webkit.org/attachment.cgi?id=297093&action=review Ok, well we tried. I could not write a test case either. > Source/WebCore/loader/FrameLoader.cpp:932 > + if (!frame) nit: I would have used a ternary. 1259810 11 koivisto 2016-12-14 10:31:26 -0800 > nit: I would have used a ternary. I like early bailouts for exceptional cases. I feel ternary sort-of signals that both cases are on the same level. 1259814 12 297093 commit-queue 2016-12-14 10:55:03 -0800 Comment on attachment 297093 patch Clearing flags on attachment: 297093 Committed r209817: <http://trac.webkit.org/changeset/209817> 1259815 13 commit-queue 2016-12-14 10:55:08 -0800 All reviewed patches have been landed. Closing bug. 297093 2016-12-14 09:17:57 -0800 2016-12-14 10:55:03 -0800 patch outgoingReferrer-nullptr.patch text/plain 1760 koivisto SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwOTgwNSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE2LTEyLTE0ICBBbnR0aSBL b2l2aXN0byAgPGFudHRpQGFwcGxlLmNvbT4KKworICAgICAgICBXZWJDb250ZW50IGNyYXNoIHVu ZGVyIFdlYkNvcmU6OkNhY2hlZFJlc291cmNlOjpsb2FkIGluIFdlYkNvcmU6OkZyYW1lTG9hZGVy OjpvdXRnb2luZ1JlZmVycmVyIGNvbnN0CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xNjU4NTIKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzI3Mjk3MTUz PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZXJl IGFwcGVhcnMgdG8gYmUgc29tZSBwYXRoIHdoZXJlIHdlIGdldCBoZXJlIHdpdGggYSBudWxsIGZy YW1lLgorICAgICAgICBObyB0ZXN0LCBkb24ndCBrbm93IGhvdyBleGFjdGx5IHRoaXMgaGFwcGVu cy4KKworICAgICAgICAqIGxvYWRlci9GcmFtZUxvYWRlci5jcHA6CisgICAgICAgIChXZWJDb3Jl OjpGcmFtZUxvYWRlcjo6b3V0Z29pbmdSZWZlcnJlcik6CisKKyAgICAgICAgICAgIE51bGwgY2hl Y2sgdGhlIGZyYW1lLgorCiAyMDE2LTEyLTE0ICBEYXZlIEh5YXR0ICA8aHlhdHRAYXBwbGUuY29t PgogCiAgICAgICAgIFtDU1MgUGFyc2VyXSBSZW1vdmUgV2Via2l0Q1NTVHJhbnNmb3JtVmFsdWUK SW5kZXg6IFNvdXJjZS9XZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAkocmV2aXNpb24gMjA5NzU3 KQorKysgU291cmNlL1dlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcAkod29ya2luZyBjb3B5 KQpAQCAtOTIzLDEyICs5MjMsMTQgQEAgU3RyaW5nIEZyYW1lTG9hZGVyOjpvdXRnb2luZ1JlZmVy cmVyKCkgYwogICAgIC8vIFNlZSBodHRwOi8vd3d3LndoYXR3Zy5vcmcvc3BlY3Mvd2ViLWFwcHMv Y3VycmVudC13b3JrLyNmZXRjaGluZy1yZXNvdXJjZXMKICAgICAvLyBmb3Igd2h5IHdlIHdhbGsg dGhlIHBhcmVudCBjaGFpbiBmb3Igc3JjZG9jIGRvY3VtZW50cy4KICAgICBGcmFtZSogZnJhbWUg PSAmbV9mcmFtZTsKLSAgICB3aGlsZSAoZnJhbWUtPmRvY3VtZW50KCktPmlzU3JjZG9jRG9jdW1l bnQoKSkgeworICAgIHdoaWxlIChmcmFtZSAmJiBmcmFtZS0+ZG9jdW1lbnQoKS0+aXNTcmNkb2NE b2N1bWVudCgpKSB7CiAgICAgICAgIGZyYW1lID0gZnJhbWUtPnRyZWUoKS5wYXJlbnQoKTsKICAg ICAgICAgLy8gU3JjZG9jIGRvY3VtZW50cyBjYW5ub3QgYmUgdG9wLWxldmVsIGRvY3VtZW50cywg YnkgZGVmaW5pdGlvbiwKICAgICAgICAgLy8gYmVjYXVzZSB0aGV5IG5lZWQgdG8gYmUgY29udGFp bmVkIGluIGlmcmFtZXMgd2l0aCB0aGUgc3JjZG9jLgogICAgICAgICBBU1NFUlQoZnJhbWUpOwog ICAgIH0KKyAgICBpZiAoIWZyYW1lKQorICAgICAgICByZXR1cm4gZW1wdHlTdHJpbmcoKTsKICAg ICByZXR1cm4gZnJhbWUtPmxvYWRlcigpLm1fb3V0Z29pbmdSZWZlcnJlcjsKIH0KIAo=