Bug 36196 - REGRESSION(r52795): Cannot open complex webarchive files in Safari due to WebKit regression.
Summary: REGRESSION(r52795): Cannot open complex webarchive files in Safari due to Web...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows XP
: P1 Major
Assignee: Nobody
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2010-03-16 15:17 PDT by Andy Estes
Modified: 2010-03-19 18:21 PDT (History)
4 users (show)

See Also:


Attachments
A webarchive file that demonstrates the regression. (901.70 KB, application/x-webarchive)
2010-03-16 15:23 PDT, Andy Estes
no flags Details
the patch that fixes the problem (1.40 KB, patch)
2010-03-18 10:13 PDT, Yong Li
beidson: review-
Details | Formatted Diff | Diff
Yong's patch with a manual test case (133.72 KB, patch)
2010-03-19 13:58 PDT, Andy Estes
beidson: review-
Details | Formatted Diff | Diff
Yong's patch with a manual test case (v2) (178.61 KB, patch)
2010-03-19 15:41 PDT, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2010-03-16 15:17:02 PDT
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.
Comment 1 Andy Estes 2010-03-16 15:20:15 PDT
<rdar://problem/7561198>
Comment 2 Andy Estes 2010-03-16 15:23:14 PDT
Created attachment 50843 [details]
A webarchive file that demonstrates the regression.
Comment 3 Yong Li 2010-03-16 18:18:52 PDT
I'll take a look
Comment 4 Yong Li 2010-03-16 18:36:45 PDT
is the webarchive file good?
Comment 5 Yong Li 2010-03-16 18:41:48 PDT
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
Comment 6 Andy Estes 2010-03-16 21:29:02 PDT
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.
Comment 7 Yong Li 2010-03-18 09:54:14 PDT
(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
Comment 8 Yong Li 2010-03-18 10:13:09 PDT
Created attachment 51041 [details]
the patch that fixes the problem
Comment 9 Yong Li 2010-03-18 11:04:08 PDT
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 10 Brady Eidson 2010-03-18 11:37:38 PDT
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.
Comment 11 Andy Estes 2010-03-18 11:42:34 PDT
(In reply to comment #10)
> I think Andy's working on this.

Yes I am.
Comment 12 Andy Estes 2010-03-19 13:58:41 PDT
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.
Comment 13 Andy Estes 2010-03-19 14:05:41 PDT
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 14 Darin Adler 2010-03-19 14:21:00 PDT
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 15 Brady Eidson 2010-03-19 14:23:02 PDT
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.
Comment 16 Darin Adler 2010-03-19 14:23:55 PDT
Ah, Brady is right! My bad.
Comment 17 Andy Estes 2010-03-19 15:41:15 PDT
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 18 WebKit Commit Bot 2010-03-19 18:20:58 PDT
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>
Comment 19 WebKit Commit Bot 2010-03-19 18:21:04 PDT
All reviewed patches have been landed.  Closing bug.