111022 2013-02-27 16:01:59 -0800 Cache partitioning does not affect iframe MainResources 2013-03-04 16:25:25 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jeffrey+webkit jeffrey+webkit abarth ddkilzer japhet mjs webkit.review.bot oldest_to_newest 843592 0 jeffrey+webkit 2013-02-27 16:01:59 -0800 With the recently added cache partitioning patch, subresources are properly getting separated into the relevant partitions, but iframe MainResources are not currently being partitioned. 843618 1 190626 jeffrey+webkit 2013-02-27 17:00:27 -0800 Created attachment 190626 Patch 843840 2 190626 abarth 2013-02-28 00:55:40 -0800 Comment on attachment 190626 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > Source/WebCore/loader/FrameLoader.cpp:1199 > + if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document()) How can m_frame->tree()->top()->document() be zero? > Source/WebCore/loader/FrameLoader.cpp:1200 > + request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition()); m_frame->tree()->top()->document() <-- I'm not sure that's idiomatically the best way to find the topDocument. Maybe it doesn't matter now that we don't have that disconnected frame stuff. 843896 3 jeffrey+webkit 2013-02-28 01:53:54 -0800 (In reply to comment #2) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > > Source/WebCore/loader/FrameLoader.cpp:1199 > > + if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document()) > > How can m_frame->tree()->top()->document() be zero? I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs. > > Source/WebCore/loader/FrameLoader.cpp:1200 > > + request.setCachePartition(m_frame->tree()->top()->document()->securityOrigin()->cachePartition()); > > m_frame->tree()->top()->document() <-- I'm not sure that's idiomatically the best way to find the topDocument. Maybe it doesn't matter now that we don't have that disconnected frame stuff. I was under the impression that Document::topDocument() did that, but it doesn't seem to. Did that change at some point? I'm all for a better idiom, but I didn't know of one. 843897 4 abarth 2013-02-28 01:56:48 -0800 Frame::document() is only ever zero while we're initializing the frame. The concept of a disconnected frame is gone from the codebase, which is probably why you don't see it in topDocument. 845823 5 190626 ap 2013-03-02 10:23:30 -0800 Comment on attachment 190626 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1 > +CONSOLE MESSAGE: line 18: FAIL Could you please make this test display produce more positive when it passes? 845825 6 ap 2013-03-02 10:23:45 -0800 produce *something* more positive 845828 7 190626 ap 2013-03-02 10:25:19 -0800 Comment on attachment 190626 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review >> LayoutTests/http/tests/cache/partitioned-cache-iframe-expected.txt:1 >> +CONSOLE MESSAGE: line 18: FAIL > > Could you please make this test display produce more positive when it passes? Or rather, it's not clear to me why the test is expected to fail now - ChangeLog does not explain this. 845910 8 jeffrey+webkit 2013-03-02 21:55:17 -0800 Perhaps I should change it to say "The cache is not partitioned" instead of "FAIL", as that's what's going on here. This test does not pass, per se, in any shipping ports at the moment, hence why it says "FAIL" right now. 846919 9 190626 ddkilzer 2013-03-04 15:31:07 -0800 Comment on attachment 190626 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review r=me >>> Source/WebCore/loader/FrameLoader.cpp:1199 >>> + if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document()) >> >> How can m_frame->tree()->top()->document() be zero? > > I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs. This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame(): bool Frame::isSubFrame() { return tree()->top() != self; } Not necessary for this patch, but please consider a follow-up patch. (Or implement the opposite: Frame::isTopFrame().) -- Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this: if (m_frame->isSubFrame()) { if (Document *topDocument = m_frame->tree()->top()->document()) request.setCachePartition(topDocument->securityOrigin()->cachePartition()); } 846921 10 ddkilzer 2013-03-04 15:31:46 -0800 (In reply to comment #9) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > r=me Plus the changes that Alexey already mentioned (that you said you had already made locally). 846925 11 190626 darin 2013-03-04 15:32:50 -0800 Comment on attachment 190626 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review >>>> Source/WebCore/loader/FrameLoader.cpp:1199 >>>> + if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document()) >>> >>> How can m_frame->tree()->top()->document() be zero? >> >> I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs. > > This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame(): > > bool Frame::isSubFrame() > { > return tree()->top() != self; > } > > Not necessary for this patch, but please consider a follow-up patch. (Or implement the opposite: Frame::isTopFrame().) > > -- > > Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this: > > if (m_frame->isSubFrame()) { > if (Document *topDocument = m_frame->tree()->top()->document()) > request.setCachePartition(topDocument->securityOrigin()->cachePartition()); > } If we added functions like that they should be in FrameTree, not on Frame itself. frame->tree()->isTop() and/or frame->tree()->isSubframe() 846936 12 jeffrey+webkit 2013-03-04 15:39:13 -0800 (In reply to comment #11) > (From update of attachment 190626 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190626&action=review > > >>>> Source/WebCore/loader/FrameLoader.cpp:1199 > >>>> + if (m_frame->tree()->top() != m_frame && m_frame->tree()->top()->document()) > >>> > >>> How can m_frame->tree()->top()->document() be zero? > >> > >> I've hit cases where a frame's document was 0, but I wasn't sure when exactly those were. Maybe it's only for subframes? That would render that clause useless. I just wanted to make sure I didn't accidentally run into null pointer derefs. > > > > This code would read clearer if "m_frame->tree()->top() != m_frame" were extracted into a method called isSubFrame(): > > > > bool Frame::isSubFrame() > > { > > return tree()->top() != self; > > } > > > > Not necessary for this patch, but please consider a follow-up patch. (Or implement the opposite: Frame::isTopFrame().) > > > > -- > > > > Also, it would be nice not to call "m_frame->tree()->top()->document()" twice, so maybe you could do this: > > > > if (m_frame->isSubFrame()) { > > if (Document *topDocument = m_frame->tree()->top()->document()) > > request.setCachePartition(topDocument->securityOrigin()->cachePartition()); > > } > > If we added functions like that they should be in FrameTree, not on Frame itself. > > frame->tree()->isTop() > > and/or > > frame->tree()->isSubframe() This idiom appears to also be used in MainResourceLoader::willSendRequest, so it should likely be done in a follow up patch (although it holds onto frame->tree()->top() and then uses it later in the function in much the same way I could be doing in this patch--it only saves one call to top()). 846983 13 jeffrey+webkit 2013-03-04 16:19:04 -0800 <rdar://problem/13342678> 846989 14 jeffrey+webkit 2013-03-04 16:25:25 -0800 Committed r144694: <http://trac.webkit.org/changeset/144694> 190626 2013-02-27 17:00:27 -0800 2013-03-04 15:32:50 -0800 Patch bug-111022-20130227165640.patch text/plain 4874 jeffrey+webkit U3VidmVyc2lvbiBSZXZpc2lvbjogMTQ0MTEzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzNhNjI4M2QxODY4ODQ5 ZGIyN2FlNzhlMmQ3Yzc3ZTQ0ZGYxZWQ0ZS4uNWMyNDk2OTJjMThmNDVjODlkMmUwMTRiNDI0MmUw N2NkZDc5MzYxZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE3IEBACisyMDEzLTAyLTI3ICBKZWZm cmV5IFBmYXUgIDxqcGZhdUBhcHBsZS5jb20+CisKKyAgICAgICAgQ2FjaGUgcGFydGl0aW9uaW5n IGRvZXMgbm90IGFmZmVjdCBpZnJhbWUgTWFpblJlc291cmNlcworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTExMDIyCisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgU2V0IHRoZSBwYXJ0aXRpb24gb2YgYSBtYWlu IHJlc291cmNlIHJlcXVlc3QgaWYgaW4gYSBzdWJmcmFtZS4KKworICAgICAgICBUZXN0OiBodHRw L3Rlc3RzL2NhY2hlL3BhcnRpdGlvbmVkLWNhY2hlLWlmcmFtZS5odG1sCisKKyAgICAgICAgKiBs b2FkZXIvRnJhbWVMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RnJhbWVMb2FkZXI6Omxv YWRVUkwpOgorCiAyMDEzLTAyLTI2ICBTdGVwaGVuIENoZW5uZXkgIDxzY2hlbm5leUBjaHJvbWl1 bS5vcmc+CiAKICAgICAgICAgZmVEaXNwbGFjZW1lbnRNYXAgZmlsdGVyIGdldHMgY29sb3Igc3Bh Y2Ugd3JvbmcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9GcmFtZUxvYWRlci5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9sb2FkZXIvRnJhbWVMb2FkZXIuY3BwCmluZGV4IDNmNTAxYmUx NWRjMjcyNDFkNTQ5ZGU1MTA2NjBmMDNlNGI3ZTkxZGEuLjY4MmZkOWQyZDUyMzQzMjFlOTg4OWJh M2MwNDRkYjg1N2UyMGVjNjEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL2xvYWRlci9GcmFt ZUxvYWRlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvbG9hZGVyL0ZyYW1lTG9hZGVyLmNwcApA QCAtMTE5NSw2ICsxMTk1LDEwIEBAIHZvaWQgRnJhbWVMb2FkZXI6OmxvYWRVUkwoY29uc3QgS1VS TCYgbmV3VVJMLCBjb25zdCBTdHJpbmcmIHJlZmVycmVyLCBjb25zdCBTdHJpCiAgICAgICAgIFJl ZlB0cjxTZWN1cml0eU9yaWdpbj4gcmVmZXJyZXJPcmlnaW4gPSBTZWN1cml0eU9yaWdpbjo6Y3Jl YXRlRnJvbVN0cmluZyhyZWZlcnJlcik7CiAgICAgICAgIGFkZEhUVFBPcmlnaW5JZk5lZWRlZChy ZXF1ZXN0LCByZWZlcnJlck9yaWdpbi0+dG9TdHJpbmcoKSk7CiAgICAgfQorI2lmIEVOQUJMRShD QUNIRV9QQVJUSVRJT05JTkcpCisgICAgaWYgKG1fZnJhbWUtPnRyZWUoKS0+dG9wKCkgIT0gbV9m cmFtZSAmJiBtX2ZyYW1lLT50cmVlKCktPnRvcCgpLT5kb2N1bWVudCgpKQorICAgICAgICByZXF1 ZXN0LnNldENhY2hlUGFydGl0aW9uKG1fZnJhbWUtPnRyZWUoKS0+dG9wKCktPmRvY3VtZW50KCkt PnNlY3VyaXR5T3JpZ2luKCktPmNhY2hlUGFydGl0aW9uKCkpOworI2VuZGlmCiAgICAgYWRkRXh0 cmFGaWVsZHNUb1JlcXVlc3QocmVxdWVzdCwgbmV3TG9hZFR5cGUsIHRydWUpOwogICAgIGlmIChu ZXdMb2FkVHlwZSA9PSBGcmFtZUxvYWRUeXBlUmVsb2FkIHx8IG5ld0xvYWRUeXBlID09IEZyYW1l TG9hZFR5cGVSZWxvYWRGcm9tT3JpZ2luKQogICAgICAgICByZXF1ZXN0LnNldENhY2hlUG9saWN5 KFJlbG9hZElnbm9yaW5nQ2FjaGVEYXRhKTsKZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5n ZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxvZwppbmRleCA3MzQ5NjAxODBkOGUxNmRiMGUyMWFk NmQ3MjBmNTlmNzc3NjE1ZDEwLi4yNTJkOWY4ZThlNWE5ZGE4OWZlZmMxYWRmNWU3Y2U1NzUwYTg3 YjEzIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMv Q2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQgQEAKKzIwMTMtMDItMjcgIEplZmZyZXkgUGZhdSAgPGpw ZmF1QGFwcGxlLmNvbT4KKworICAgICAgICBDYWNoZSBwYXJ0aXRpb25pbmcgZG9lcyBub3QgYWZm ZWN0IGlmcmFtZSBNYWluUmVzb3VyY2VzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3Jn L3Nob3dfYnVnLmNnaT9pZD0xMTEwMjIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICAqIGh0dHAvdGVzdHMvY2FjaGUvcGFydGl0aW9uZWQtY2FjaGUtaWZy YW1lLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICogaHR0cC90ZXN0cy9jYWNoZS9wYXJ0 aXRpb25lZC1jYWNoZS1pZnJhbWUuaHRtbDogQWRkZWQuCisgICAgICAgICogaHR0cC90ZXN0cy9j YWNoZS9yZXNvdXJjZXMvcmFuZG9tLWNhY2hlZC1pZnJhbWUuY2dpOiBBZGRlZC4KKwogMjAxMy0w Mi0yNiAgU3RlcGhlbiBDaGVubmV5ICA8c2NoZW5uZXlAY2hyb21pdW0ub3JnPgogCiAgICAgICAg IGZlRGlzcGxhY2VtZW50TWFwIGZpbHRlciBnZXRzIGNvbG9yIHNwYWNlIHdyb25nCmRpZmYgLS1n aXQgYS9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL2NhY2hlL3BhcnRpdGlvbmVkLWNhY2hlLWlmcmFt ZS1leHBlY3RlZC50eHQgYi9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL2NhY2hlL3BhcnRpdGlvbmVk LWNhY2hlLWlmcmFtZS1leHBlY3RlZC50eHQKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMC4uZThjMTQ4YjkwYjQ2ZjE1ZmUz ZWE3NDI0OWUwMGJkNTg3YjE5YWE1NgotLS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2h0 dHAvdGVzdHMvY2FjaGUvcGFydGl0aW9uZWQtY2FjaGUtaWZyYW1lLWV4cGVjdGVkLnR4dApAQCAt MCwwICsxLDIgQEAKK0NPTlNPTEUgTUVTU0FHRTogbGluZSAxODogRkFJTAorCmRpZmYgLS1naXQg YS9MYXlvdXRUZXN0cy9odHRwL3Rlc3RzL2NhY2hlL3BhcnRpdGlvbmVkLWNhY2hlLWlmcmFtZS5o dG1sIGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9jYWNoZS9wYXJ0aXRpb25lZC1jYWNoZS1pZnJh bWUuaHRtbApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwLi41YWJmOThjMzlkNzUzYTQ3YjBkNTdjNDc4N2NlZjRmMDMzYTEz ODc2Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9jYWNoZS9wYXJ0 aXRpb25lZC1jYWNoZS1pZnJhbWUuaHRtbApAQCAtMCwwICsxLDMzIEBACis8aHRtbD4KKzxoZWFk PgorPHNjcmlwdD4KK2lmICh3aW5kb3cudGVzdFJ1bm5lcikgeworICAgIHRlc3RSdW5uZXIuZHVt cEFzVGV4dCgpOworICAgIHRlc3RSdW5uZXIud2FpdFVudGlsRG9uZSgpOworICAgIGludGVybmFs cy5zZXR0aW5ncy5zZXRTdG9yYWdlQmxvY2tpbmdQb2xpY3koJ0Jsb2NrVGhpcmRQYXJ0eScpOwor fQorCit2YXIgdmFsdWUgPSB3aW5kb3cubG9jYXRpb24uaHJlZi5tYXRjaCgvXD92YWx1ZT0oW14m XSopLyk7CitpZiAodmFsdWUpCisgICAgdmFsdWUgPSB2YWx1ZVsxXTsKKword2luZG93Lm9ubWVz c2FnZSA9IGZ1bmN0aW9uKGV2ZW50KSB7CisgICAgdmFyIGNhY2hlZFZhbHVlID0gZXZlbnQuZGF0 YTsKKyAgICBpZiAodmFsdWUgIT09IG51bGwpIHsKKyAgICAgICAgaWYgKGNhY2hlZFZhbHVlID09 IHZhbHVlKQorICAgICAgICAgICAgY29uc29sZS5sb2coIkZBSUwiKTsKKyAgICAgICAgZWxzZQor ICAgICAgICAgICAgY29uc29sZS5sb2coIlBBU1MiKTsKKyAgICAgICAgaWYgKHdpbmRvdy50ZXN0 UnVubmVyKSB7CisgICAgICAgICAgICB0ZXN0UnVubmVyLm5vdGlmeURvbmUoKTsKKyAgICAgICAg ICAgIGludGVybmFscy5zZXR0aW5ncy5zZXRTdG9yYWdlQmxvY2tpbmdQb2xpY3koJ0FsbG93QWxs Jyk7CisgICAgICAgIH0KKyAgICB9IGVsc2UKKyAgICAgICAgd2luZG93LmxvY2F0aW9uID0gImh0 dHA6Ly9sb2NhbGhvc3Q6ODAwMC9jYWNoZS9wYXJ0aXRpb25lZC1jYWNoZS1pZnJhbWUuaHRtbD92 YWx1ZT0iICsgY2FjaGVkVmFsdWU7Cit9Cis8L3NjcmlwdD4KKzwvaGVhZD4KKzxib2R5PgorPGlm cmFtZSBzcmM9Imh0dHA6Ly8xMjcuMC4wLjE6ODAwMC9jYWNoZS9yZXNvdXJjZXMvcmFuZG9tLWNh Y2hlZC1pZnJhbWUuY2dpIj48L2lmcmFtZT4KKzwvYm9keT4KKzwvaHRtbD4KZGlmZiAtLWdpdCBh L0xheW91dFRlc3RzL2h0dHAvdGVzdHMvY2FjaGUvcmVzb3VyY2VzL3JhbmRvbS1jYWNoZWQtaWZy YW1lLmNnaSBiL0xheW91dFRlc3RzL2h0dHAvdGVzdHMvY2FjaGUvcmVzb3VyY2VzL3JhbmRvbS1j YWNoZWQtaWZyYW1lLmNnaQpuZXcgZmlsZSBtb2RlIDEwMDc1NQppbmRleCAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi45MGU4M2NiYjk2NDU1ZjBiZmEwYzAxNTgzY2Q4 M2JiMjgzNmFkYjNlCi0tLSAvZGV2L251bGwKKysrIGIvTGF5b3V0VGVzdHMvaHR0cC90ZXN0cy9j YWNoZS9yZXNvdXJjZXMvcmFuZG9tLWNhY2hlZC1pZnJhbWUuY2dpCkBAIC0wLDAgKzEsMTYgQEAK KyMhL3Vzci9iaW4vcGVybCAtdworCitwcmludCAiQ29udGVudC10eXBlOiB0ZXh0L2h0bWxcbiI7 CitwcmludCAiQ2FjaGUtY29udHJvbDogbWF4LWFnZT02MDAwMFxuIjsKK3ByaW50ICJFVGFnOiBc IjEyMzQ1Njc4OVwiXG4iOworcHJpbnQgIlxuIjsKKworbXkgJHJhbmRvbV9udW1iZXIgPSBpbnQo cmFuZCgxMDAwMDAwMDAwMDAwKSk7CitwcmludCA8PCAiRU5EIjsKKworPHNjcmlwdD4KK3ZhciBy YW5kb21OdW1iZXIgPSAgJHJhbmRvbV9udW1iZXI7Cit0b3AucG9zdE1lc3NhZ2UocmFuZG9tTnVt YmVyLCAiKiIpOworPC9zY3JpcHQ+CisKK0VORAo=