Bug 67983 - CRASH under WebCore::ArchiveResourceCollection::addAllResources loading WebArchive
Summary: CRASH under WebCore::ArchiveResourceCollection::addAllResources loading WebAr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2011-09-12 21:08 PDT by Joseph Pecoraro
Modified: 2011-09-13 12:24 PDT (History)
4 users (show)

See Also:

[TEST] Test WebArchive (12.05 KB, application/x-webarchive)
2011-09-12 21:19 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (17.61 KB, patch)
2011-09-12 21:19 PDT, Joseph Pecoraro
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Patch to Land - EWS Bot Test (19.18 KB, patch)
2011-09-13 10:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-09-12 21:08:50 PDT
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.
Comment 1 Joseph Pecoraro 2011-09-12 21:19:19 PDT
Created attachment 107137 [details]
[TEST] Test WebArchive
Comment 2 Joseph Pecoraro 2011-09-12 21:19:40 PDT
Created attachment 107138 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2011-09-12 21:28:26 PDT
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.
Comment 4 Joseph Pecoraro 2011-09-12 21:35:47 PDT
Comment 5 WebKit Review Bot 2011-09-12 21:44:53 PDT
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:
Comment 6 Darin Adler 2011-09-13 08:40:11 PDT
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.
Comment 7 Joseph Pecoraro 2011-09-13 10:33:35 PDT
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.
Comment 8 Joseph Pecoraro 2011-09-13 10:50:38 PDT
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.
Comment 9 Joseph Pecoraro 2011-09-13 12:24:42 PDT
Landed r95038 <http://trac.webkit.org/changeset/95038>.