Bug 36196 - REGRESSION(r52795): Cannot open complex webarchive files in Safari due to WebKit regression.
: REGRESSION(r52795): Cannot open complex webarchive files in Safari due to Web...
Status: RESOLVED FIXED
: WebKit
Page Loading
: 528+ (Nightly build)
: PC Windows XP
: P1 Major
Assigned To:
:
: InRadar, Regression
:
:
  Show dependency treegraph
 
Reported: 2010-03-16 15:17 PST by
Modified: 2010-03-19 18:21 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-03-16 15:17:02 PST
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 From 2010-03-16 15:20:15 PST -------
<rdar://problem/7561198>
------- Comment #2 From 2010-03-16 15:23:14 PST -------
Created an attachment (id=50843) [details]
A webarchive file that demonstrates the regression.
------- Comment #3 From 2010-03-16 18:18:52 PST -------
I'll take a look
------- Comment #4 From 2010-03-16 18:36:45 PST -------
is the webarchive file good?
------- Comment #5 From 2010-03-16 18:41:48 PST -------
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 From 2010-03-16 21:29:02 PST -------
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 From 2010-03-18 09:54:14 PST -------
(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 From 2010-03-18 10:13:09 PST -------
Created an attachment (id=51041) [details]
the patch that fixes the problem
------- Comment #9 From 2010-03-18 11:04:08 PST -------
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 From 2010-03-18 11:37:38 PST -------
(From update of attachment 51041 [details])
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 From 2010-03-18 11:42:34 PST -------
(In reply to comment #10)
> I think Andy's working on this.

Yes I am.
------- Comment #12 From 2010-03-19 13:58:41 PST -------
Created an attachment (id=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 From 2010-03-19 14:05:41 PST -------
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 From 2010-03-19 14:21:00 PST -------
(From update of attachment 51185 [details])
> 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 From 2010-03-19 14:23:02 PST -------
(From update of attachment 51185 [details])
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 From 2010-03-19 14:23:55 PST -------
Ah, Brady is right! My bad.
------- Comment #17 From 2010-03-19 15:41:15 PST -------
Created an attachment (id=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 From 2010-03-19 18:20:58 PST -------
(From update of attachment 51196 [details])
Clearing flags on attachment: 51196

Committed r56289: <http://trac.webkit.org/changeset/56289>
------- Comment #19 From 2010-03-19 18:21:04 PST -------
All reviewed patches have been landed.  Closing bug.