108589 2013-01-31 23:47:50 -0800 REGRESSION (r137607): Loading of archives as substitute data is broken 2013-02-04 14:21:27 -0800 1 1 1 Unclassified WebKit Page Loading 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED InRadar, Regression P1 Normal --- 1 ap japhet beidson japhet jeffm webkit.review.bot oldest_to_newest 822395 0 ap 2013-01-31 23:47:50 -0800 It is possible to call FrameLoader::load with substitute data that is a WebArchive (or another type of archive, such as MHTML for Chromium). This is exposed via both WebKit1 and WebKit2 API on Mac, see e.g. WKPageLoadWebArchiveData() for the latter. Our bisection shows that this functionality got broken in <http://trac.webkit.org/changeset/137607>. By the time MainDocumentLoader tries to decode the archive, the data it tries to use is null. Here is how a debug stack trace looks like, with an assertion being fired: ASSERTION FAILED: data /Volumes/Data/SafariSources/OpenSource/Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp(269) : static PassRefPtr<WebCore::LegacyWebArchive> WebCore::LegacyWebArchive::create(const WebCore::KURL &, WebCore::SharedBuffer *) 1 0x104657d5a WebCore::LegacyWebArchive::create(WebCore::KURL const&, WebCore::SharedBuffer*) 2 0x10350402f WTF::PassRefPtr<WebCore::Archive> WebCore::archiveFactoryCreate<WebCore::LegacyWebArchive>(WebCore::KURL const&, WebCore::SharedBuffer*) 3 0x103503f18 WebCore::ArchiveFactory::create(WebCore::KURL const&, WebCore::SharedBuffer*, WTF::String const&) 4 0x1038f26ee WebCore::DocumentLoader::maybeCreateArchive() 5 0x1038f254c WebCore::DocumentLoader::finishedLoading() 6 0x1046772d1 WebCore::MainResourceLoader::didFinishLoading(double) 7 0x104677085 WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) 8 0x1046773e6 WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction) 9 0x10467732b WebCore::MainResourceLoader::callContinueAfterContentPolicy(void*, WebCore::PolicyAction) 10 0x1046779f8 WebCore::MainResourceLoader::responseReceived(WebCore::CachedResource*, WebCore::ResourceResponse const&) 11 0x104675824 WebCore::MainResourceLoader::handleSubstituteDataLoadNow(WebCore::RunLoopTimer<WebCore::MainResourceLoader>*) 12 0x104679f83 WebCore::RunLoopTimer<WebCore::MainResourceLoader>::fired() One easy way to reproduce this that I'm aware of is: 1. Open http://www.apple.com/startpage/ in Safari. 2. Select Email This Page from the Share menu (a button to the left of address bar, or just Command+I). 3. In the new email message that appears in Mail.app, change the "Send as web content" selection at top right to PDF. 822396 1 ap 2013-01-31 23:51:02 -0800 In Radar as <rdar://problem/13089321>. It seems easy to find some way to access substitute data from maybeCreateArchive(), but not easy to decide which design is right. I suspect that more code paths that use DocumentLoader::mainResourceData() function for substitute data could be broken. Nate, would you be interested to take a look at this? 823194 2 japhet 2013-02-01 13:24:02 -0800 (In reply to comment #1) > In Radar as <rdar://problem/13089321>. > > It seems easy to find some way to access substitute data from maybeCreateArchive(), but not easy to decide which design is right. I suspect that more code paths that use DocumentLoader::mainResourceData() function for substitute data could be broken. > > Nate, would you be interested to take a look at this? I think this is a pretty simple fix, MainResourceLoader::resourceData() should check m_substituteData for data if it is valid. I haven't tried to write a test yet, though. 823263 3 ap 2013-02-01 14:21:03 -0800 Do you think that other callers of this function would be OK with that? An archive is not really truly the main resource, even though it's handled like that at the first stage of loading. I do not know much about how archives work. 823357 4 japhet 2013-02-01 15:33:19 -0800 (In reply to comment #3) > Do you think that other callers of this function would be OK with that? An archive is not really truly the main resource, even though it's handled like that at the first stage of loading. Another option, it appears that we can safely exit early and get the right result in DocumentLoader::maybeCreateArchive() if mainResourceData() returns null. I don't know whether that has side effects, though. I'm trying to decide whether there are other situations where this underlying issue (Non-empty SubstituteData loads leave DocumentLoader::mainResourceData() empty) could be a problem. My guess is Yes, but I don't have definitive proof. 823426 5 186185 japhet 2013-02-01 16:41:25 -0800 Created attachment 186185 patch (no test) I'm not sure I like this solution, and I'm not sure if there's a good way to test this (besides the repro steps in the initial report), but here's what I'm thinking at the moment. 823515 6 186185 buildbot 2013-02-01 19:31:47 -0800 Comment on attachment 186185 patch (no test) Attachment 186185 did not pass win-ews (win): Output: http://queues.webkit.org/results/16332442 823695 7 186185 ap 2013-02-02 13:03:54 -0800 Comment on attachment 186185 patch (no test) View in context: https://bugs.webkit.org/attachment.cgi?id=186185&action=review > Source/WebCore/loader/DocumentLoader.cpp:134 > if (m_mainResourceLoader) > return m_mainResourceLoader->resourceData(); I've built r137606 now, and how it worked was that m_mainResourceLoader->resourceData() was returning non-null data. Is it intentional that it no longer works this way? 823773 8 ap 2013-02-02 23:36:44 -0800 The reason ResourceData used to be non-null is that the data was accumulated in ResourceLoader once received. Now that MainResourceLoader does not inherit from ResourceLoader, the data goes nowhere. A fix that restores previous behavior would be one that makes MainResourceLoader::resourceData() return actual data for substitute loads. I can think of two approaches - either try MainResourceLoader::m_substituteData when MainResourceLoader::m_resource is null, or make sure to generate m_resource for these loads. Which one better fits the overall design? 824572 9 japhet 2013-02-04 10:59:36 -0800 (In reply to comment #8) > The reason ResourceData used to be non-null is that the data was accumulated in ResourceLoader once received. Now that MainResourceLoader does not inherit from ResourceLoader, the data goes nowhere. > > A fix that restores previous behavior would be one that makes MainResourceLoader::resourceData() return actual data for substitute loads. I can think of two approaches - either try MainResourceLoader::m_substituteData when MainResourceLoader::m_resource is null, or make sure to generate m_resource for these loads. > > Which one better fits the overall design? Given that m_resource has no knowledge of SubstituteData loads, fixing this in MainResourceLoader::resourceData() probably makes more sense. Will fix shortly. 824653 10 186438 japhet 2013-02-04 12:32:26 -0800 Created attachment 186438 Fix in MainResourceLoader 824658 11 186438 ap 2013-02-04 12:44:49 -0800 Comment on attachment 186438 Fix in MainResourceLoader View in context: https://bugs.webkit.org/attachment.cgi?id=186438&action=review It seems wasteful to copy all the data. Is there a way to redesign the storage to avoid such copying? > Source/WebCore/loader/MainResourceLoader.cpp:213 > + if (m_substituteData.isValid()) > + return ResourceBuffer::create(m_substituteData.content()->data(), m_substituteData.content()->size()); > return m_resource ? m_resource->resourceBuffer() : 0; I'd have written this differently, putting common case first. But this is OK too. 824694 12 japhet 2013-02-04 13:23:36 -0800 (In reply to comment #11) > (From update of attachment 186438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186438&action=review > > It seems wasteful to copy all the data. Is there a way to redesign the storage to avoid such copying? > > > Source/WebCore/loader/MainResourceLoader.cpp:213 > > + if (m_substituteData.isValid()) > > + return ResourceBuffer::create(m_substituteData.content()->data(), m_substituteData.content()->size()); > > return m_resource ? m_resource->resourceBuffer() : 0; > > I'd have written this differently, putting common case first. But this is OK too. Good point, will land with the reordering. 824699 13 186448 japhet 2013-02-04 13:27:08 -0800 Created attachment 186448 Patch for landing 824750 14 186438 darin 2013-02-04 14:11:58 -0800 Comment on attachment 186438 Fix in MainResourceLoader View in context: https://bugs.webkit.org/attachment.cgi?id=186438&action=review >>> Source/WebCore/loader/MainResourceLoader.cpp:213 >>> return m_resource ? m_resource->resourceBuffer() : 0; >> >> I'd have written this differently, putting common case first. But this is OK too. > > Good point, will land with the reordering. When it comes to early return, I like “unusual case first”, but I guess here there is a small performance reason to put the common case first. 824752 15 japhet 2013-02-04 14:14:21 -0800 (In reply to comment #14) > (From update of attachment 186438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186438&action=review > > >>> Source/WebCore/loader/MainResourceLoader.cpp:213 > >>> return m_resource ? m_resource->resourceBuffer() : 0; > >> > >> I'd have written this differently, putting common case first. But this is OK too. > > > > Good point, will land with the reordering. > > When it comes to early return, I like “unusual case first”, but I guess here there is a small performance reason to put the common case first. I strongly agree with you in the case where the unusual case is much shorter than the common case. When they're both so short, I'm happy to do whatever the reviewer suggests :) 824759 16 186448 webkit.review.bot 2013-02-04 14:21:22 -0800 Comment on attachment 186448 Patch for landing Clearing flags on attachment: 186448 Committed r141811: <http://trac.webkit.org/changeset/141811> 824760 17 webkit.review.bot 2013-02-04 14:21:27 -0800 All reviewed patches have been landed. Closing bug. 186185 2013-02-01 16:41:25 -0800 2013-02-04 10:59:42 -0800 patch (no test) arch.txt text/plain 1340 japhet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MTY1OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDEzLTAyLTAxICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjEzNzYw Nyk6IExvYWRpbmcgb2YgYXJjaGl2ZXMgYXMgc3Vic3RpdHV0ZSBkYXRhIGlzIGJyb2tlbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA4NTg5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBsb2FkZXIvRG9j dW1lbnRMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6RG9jdW1lbnRMb2FkZXI6Om1haW5S ZXNvdXJjZURhdGEpOiBSZXR1cm4gdGhlIGNvbnRlbnQgZnJvbQorICAgICAgICAgICAgU3Vic3Rp dHV0ZURhdGEgYXMgbWFpblJlc291cmNlRGF0YSBpZiBwcmVzZW50LgorCiAyMDEzLTAyLTAxICBC ZW5qYW1pbiBQb3VsYWluICA8YnBvdWxhaW5AYXBwbGUuY29tPgogCiAgICAgICAgIENsZWFuIHRo ZSBTdHJpbmctPkF0b21pY1N0cmluZyBjb252ZXJzaW9uIGZvciBBbmltYXRpb25Db250cm9sbGVy OjpwYXVzZUFuaW1hdGlvbkF0VGltZQpJbmRleDogU291cmNlL1dlYkNvcmUvbG9hZGVyL0RvY3Vt ZW50TG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9sb2FkZXIvRG9jdW1l bnRMb2FkZXIuY3BwCShyZXZpc2lvbiAxNDE2MjUpCisrKyBTb3VyY2UvV2ViQ29yZS9sb2FkZXIv RG9jdW1lbnRMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0xMjgsNiArMTI4LDggQEAKIHsK ICAgICBpZiAobV9tYWluUmVzb3VyY2VEYXRhKQogICAgICAgICByZXR1cm4gbV9tYWluUmVzb3Vy Y2VEYXRhOworICAgIGlmIChtX3N1YnN0aXR1dGVEYXRhLmlzVmFsaWQoKSkKKyAgICAgICAgcmV0 dXJuIFJlc291cmNlQnVmZmVyOjpjcmVhdGUobV9zdWJzdGl0dXRlRGF0YS5jb250ZW50KCktPmRh dGEoKSwgbV9zdWJzdGl0dXRlRGF0YS5jb250ZW50KCktPnNpemUoKSk7CiAgICAgaWYgKG1fbWFp blJlc291cmNlTG9hZGVyKQogICAgICAgICByZXR1cm4gbV9tYWluUmVzb3VyY2VMb2FkZXItPnJl c291cmNlRGF0YSgpOwogICAgIHJldHVybiAwOwo= 186438 2013-02-04 12:32:26 -0800 2013-02-04 14:11:58 -0800 Fix in MainResourceLoader arch2.txt text/plain 1331 japhet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MTc5MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDEzLTAyLTA0ICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjEzNzYw Nyk6IExvYWRpbmcgb2YgYXJjaGl2ZXMgYXMgc3Vic3RpdHV0ZSBkYXRhIGlzIGJyb2tlbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA4NTg5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBsb2FkZXIvTWFp blJlc291cmNlTG9hZGVyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1haW5SZXNvdXJjZUxvYWRl cjo6cmVzb3VyY2VEYXRhKTogUmV0dXJuIHRoZSBjb250ZW50IGZyb20KKyAgICAgICAgICAgIFN1 YnN0aXR1dGVEYXRhIGFzIG1haW5SZXNvdXJjZURhdGEgaWYgcHJlc2VudC4KKwogMjAxMy0wMi0w NCAgVG9tIFNlcGV6ICA8dHNlcGV6QGNocm9taWl1bS5vcmc+CiAKICAgICAgICAgWFNTIEF1ZGl0 b3IgYnlwYXNzIHZpYSBzdmcgdGFncyBhbmQgeGxpbms6aHJlZgpJbmRleDogU291cmNlL1dlYkNv cmUvbG9hZGVyL01haW5SZXNvdXJjZUxvYWRlci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dl YkNvcmUvbG9hZGVyL01haW5SZXNvdXJjZUxvYWRlci5jcHAJKHJldmlzaW9uIDE0MTc4NSkKKysr IFNvdXJjZS9XZWJDb3JlL2xvYWRlci9NYWluUmVzb3VyY2VMb2FkZXIuY3BwCSh3b3JraW5nIGNv cHkpCkBAIC0yMDcsNiArMjA3LDkgQEAKIAogUGFzc1JlZlB0cjxSZXNvdXJjZUJ1ZmZlcj4gTWFp blJlc291cmNlTG9hZGVyOjpyZXNvdXJjZURhdGEoKQogeworICAgIEFTU0VSVCghbV9yZXNvdXJj ZSB8fCAhbV9zdWJzdGl0dXRlRGF0YS5pc1ZhbGlkKCkpOworICAgIGlmIChtX3N1YnN0aXR1dGVE YXRhLmlzVmFsaWQoKSkKKyAgICAgICAgcmV0dXJuIFJlc291cmNlQnVmZmVyOjpjcmVhdGUobV9z dWJzdGl0dXRlRGF0YS5jb250ZW50KCktPmRhdGEoKSwgbV9zdWJzdGl0dXRlRGF0YS5jb250ZW50 KCktPnNpemUoKSk7CiAgICAgcmV0dXJuIG1fcmVzb3VyY2UgPyBtX3Jlc291cmNlLT5yZXNvdXJj ZUJ1ZmZlcigpIDogMDsKIH0KIAo= 186448 2013-02-04 13:27:08 -0800 2013-02-04 14:21:22 -0800 Patch for landing bug-108589-20130204132347.patch text/plain 1597 japhet SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE0MTc5MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDEzLTAyLTA0ICBOYXRlIENo YXBpbiAgPGphcGhldEBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjEzNzYw Nyk6IExvYWRpbmcgb2YgYXJjaGl2ZXMgYXMgc3Vic3RpdHV0ZSBkYXRhIGlzIGJyb2tlbgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTA4NTg5CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgQWxleGV5IFByb3NrdXJ5YWtvdi4KKworICAgICAgICAqIGxvYWRl ci9NYWluUmVzb3VyY2VMb2FkZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWFpblJlc291cmNl TG9hZGVyOjpyZXNvdXJjZURhdGEpOiBSZXR1cm4gdGhlIGNvbnRlbnQgZnJvbQorICAgICAgICAg ICAgU3Vic3RpdHV0ZURhdGEgYXMgbWFpblJlc291cmNlRGF0YSBpZiBwcmVzZW50LgorCiAyMDEz LTAyLTA0ICBUb20gU2VwZXogIDx0c2VwZXpAY2hyb21paXVtLm9yZz4KIAogICAgICAgICBYU1Mg QXVkaXRvciBieXBhc3MgdmlhIHN2ZyB0YWdzIGFuZCB4bGluazpocmVmCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9sb2FkZXIvTWFpblJlc291cmNlTG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3Vy Y2UvV2ViQ29yZS9sb2FkZXIvTWFpblJlc291cmNlTG9hZGVyLmNwcAkocmV2aXNpb24gMTQxNzg1 KQorKysgU291cmNlL1dlYkNvcmUvbG9hZGVyL01haW5SZXNvdXJjZUxvYWRlci5jcHAJKHdvcmtp bmcgY29weSkKQEAgLTIwNyw3ICsyMDcsMTIgQEAgYm9vbCBNYWluUmVzb3VyY2VMb2FkZXI6Omlz UG9zdE9yUmVkaXJlYwogCiBQYXNzUmVmUHRyPFJlc291cmNlQnVmZmVyPiBNYWluUmVzb3VyY2VM b2FkZXI6OnJlc291cmNlRGF0YSgpCiB7Ci0gICAgcmV0dXJuIG1fcmVzb3VyY2UgPyBtX3Jlc291 cmNlLT5yZXNvdXJjZUJ1ZmZlcigpIDogMDsKKyAgICBBU1NFUlQoIW1fcmVzb3VyY2UgfHwgIW1f c3Vic3RpdHV0ZURhdGEuaXNWYWxpZCgpKTsKKyAgICBpZiAobV9yZXNvdXJjZSkKKyAgICAgICAg cmV0dXJuIG1fcmVzb3VyY2UtPnJlc291cmNlQnVmZmVyKCk7CisgICAgaWYgKG1fc3Vic3RpdHV0 ZURhdGEuaXNWYWxpZCgpKQorICAgICAgICByZXR1cm4gUmVzb3VyY2VCdWZmZXI6OmNyZWF0ZSht X3N1YnN0aXR1dGVEYXRhLmNvbnRlbnQoKS0+ZGF0YSgpLCBtX3N1YnN0aXR1dGVEYXRhLmNvbnRl bnQoKS0+c2l6ZSgpKTsKKyAgICByZXR1cm4gMDsKIH0KIAogdm9pZCBNYWluUmVzb3VyY2VMb2Fk ZXI6OnJlZGlyZWN0UmVjZWl2ZWQoQ2FjaGVkUmVzb3VyY2UqIHJlc291cmNlLCBSZXNvdXJjZVJl cXVlc3QmIHJlcXVlc3QsIGNvbnN0IFJlc291cmNlUmVzcG9uc2UmIHJlZGlyZWN0UmVzcG9uc2Up Cg==