SUMMARY: An empty page is displayed when opening a saved webarchive file in Safari. This feature regressed in http://trac.webkit.org/changeset/52795. This issue seems to be specific to windows and has been reproduced in Windows 7, Vista and XP. This is not reproducible on Safari/Mac. STEPS TO REPRODUCE: 1) Go to www.yahoo.com 2) Click File->Save As... to save as webarchive. 4) Create a new tab and open the saved webarchive file. EXPECTED RESULTS: Webarchive contents are displayed in the new tab. ACTUAL RESULTS: An empty page is presented.
<rdar://problem/7561198>
Created attachment 50843 [details] A webarchive file that demonstrates the regression.
I'll take a look
is the webarchive file good?
never mind. I can load the webarchive with safari. I guess there's a bug in SharedBuffer::createWithContentsOfFile() Are you using http://trac.webkit.org/browser/trunk/WebCore/platform/win/SharedBufferWin.cpp? If not, you must set "m_size" in the returned SharedBuffer
Yes, we use SharedBufferWin.cpp's version of SharedBuffer::createWithContentsOfFile(). Safari is also a CF platform on Windows, so will call the non-trivial version of maybeTransferPlatformData() in SharedBufferCF.cpp.
(In reply to comment #6) > Yes, we use SharedBufferWin.cpp's version of > SharedBuffer::createWithContentsOfFile(). Safari is also a CF platform on > Windows, so will call the non-trivial version of maybeTransferPlatformData() in > SharedBufferCF.cpp. I've found the bug. will post a fix soon
Created attachment 51041 [details] the patch that fixes the problem
This can be verified with any webarchive file bigger than 4K. There're already many webarchive files under LayoutTests\webarchive, so no need to add another layout test for this.
Comment on attachment 51041 [details] the patch that fixes the problem This code change looks good. Unfortunately, I don't think we should land it yet because there's actually no layout tests covering this. The webarchive tests are disabled in DRT win because the proper support was never added. Since this was such a complete regression in not only WebArchives but possibly other areas of code that exercise SharedBuffer -> CFData, I think we need to add the DRT support and enabled the tests before we land this. I think Andy's working on this.
(In reply to comment #10) > I think Andy's working on this. Yes I am.
Created attachment 51185 [details] Yong's patch with a manual test case In the interest of fixing the regression, let's land the patch with a manual test for now while DRT support for webarchive files is added to the Windows port.
Yong, in the course of investigating this, I think I found another issue with http://trac.webkit.org/changeset/52795. If I wrote the following routine, I believe it would result in infinite recursion: SharedBuffer* sharedBufferWithCFDataAndData(CFDataRef cfData, const char* data, unsigned dataLength) { SharedBuffer* buffer = new SharedBuffer(cfData); buffer->append(data, dataLength); return buffer; } This is due to the interaction between SharedBuffer::append() and SharedBuffer::maybeTransferPlatformData(). A simple fix might be to add a flag that is set to true when maybeTransferPlatformData() is executing to break the recursion. This probably belongs in a separate bug report, which I'll create.
Comment on attachment 51185 [details] Yong's patch with a manual test case > Property changes on: WebCore/manual-tests/WebKitSite.webarchive > ___________________________________________________________________ > Added: svn:executable > + * To avoid problems with newlines this needs to have its MIME type set to a binary type. Also, it should not have its executable bit set. > + // Internal data in SharedBuffer can be segmented. We need to get the contiguous > + // buffer. Comment should be on a single line, and have one space after the period. r=me, but commit-queue- because we can't land the archive without the MIME type
Comment on attachment 51185 [details] Yong's patch with a manual test case I know this is your first manual test, and when I was describing them to you I DEFINITELY should've mentioned this - my fault. The manual tests should be an html file with a description of the test. Something pointing to this bug, the symptom, and what you should see to indicate success. Turns out that with file:// urls, you can embed webarchives as the src of an iframe. So an html file describing the bug and the webarchive in an iframe would be much more useful here. Also, the ChangeLog needs to be updated to include the manual test files.
Ah, Brady is right! My bad.
Created attachment 51196 [details] Yong's patch with a manual test case (v2) Set the mime-type of WebKitSite.webarchive correctly and address comments from Darin and Brady.
Comment on attachment 51196 [details] Yong's patch with a manual test case (v2) Clearing flags on attachment: 51196 Committed r56289: <http://trac.webkit.org/changeset/56289>
All reviewed patches have been landed. Closing bug.