Bug 67983 – CRASH under WebCore::ArchiveResourceCollection::addAllResources loading WebArchive

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

I was seeing reproducible crashes loading a WebArchive which had a subresource
with a null mime type. I have a fix to follow with a test.

[Joseph Pecoraro]

Created attachment 107137 [details]
[TEST] Test WebArchive

[Joseph Pecoraro]

Created attachment 107138 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

This fix assumes that it is okay for a sub resource to have a null mime type.
Using another tool for working with webarchives seems to assume that
the mime type always exists for a valid webarchive.

[Joseph Pecoraro]

<rdar://problem/10113662>

[WebKit Review Bot]

Comment on attachment 107138 [details]
[PATCH] Proposed Fix

Attachment 107138 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9650091

New failing tests:
loader/test-loading-archive-subresource-null-mimetype.html

[Darin Adler]

Comment on attachment 107138 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=107138&action=review

r=me but I think the test needs to be put in a different directory or somehow otherwise skipped for non-webarchive platforms

> LayoutTests/ChangeLog:11
> +        * loader/test-loading-archive-subresource-null-mimetype.html: Added.

This test needs to be skipped on the many platforms that do not support CoreFoundation property list web archives. You should look for other similar tests and see how that works. I’m guessing that the tests normally go inside webarchive/loading rather than in loader to make sure they get skipped on platforms that do not have web archives.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:331
> +        LOG(Archives, "LegacyWebArchive - Main Resource MIME Type is required, but was null.");

The words Resource and Type should not be capitalized here.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:351
> +            RefPtr<ArchiveResource> subresource = createResource(subresourceDict);
> +            if (subresource)
> +                addSubresource(subresource.release());

It would be nicer to define this variable inside the if.

[Joseph Pecoraro]

Arg uploaded the patch with the test in wrong directory. I'll upload a new patch
with the results which should be fine on the bots.

[Joseph Pecoraro]

Created attachment 107195 [details]
[PATCH] Patch to Land - EWS Bot Test

This is for the bots. I addressed Darin's comments and fixed the test directory.

[Joseph Pecoraro]

Landed r95038 <http://trac.webkit.org/changeset/95038>.