Bug 36196 – REGRESSION(r52795): Cannot open complex webarchive files in Safari due to WebKit regression.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Andy Estes]

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.

[Andy Estes]

<rdar://problem/7561198>

[Andy Estes]

Created attachment 50843 [details]
A webarchive file that demonstrates the regression.

[Yong Li]

I'll take a look

[Yong Li]

is the webarchive file good?

[Yong Li]

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

[Andy Estes]

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.

[Yong Li]

(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

[Yong Li]

Created attachment 51041 [details]
the patch that fixes the problem

[Yong Li]

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.

[Brady Eidson]

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.

[Andy Estes]

(In reply to comment #10)
> I think Andy's working on this.

Yes I am.

[Andy Estes]

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.

[Andy Estes]

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.

[Darin Adler]

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

[Brady Eidson]

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.

[Darin Adler]

Ah, Brady is right! My bad.

[Andy Estes]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.