Bug 140755 - Crash in URL::protocol() after appcache load fails
Summary: Crash in URL::protocol() after appcache load fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-01-21 20:31 PST by Alexey Proskuryakov
Modified: 2015-01-22 14:14 PST (History)
5 users (show)

See Also:


Attachments
proposed fixd (7.39 KB, patch)
2015-01-21 20:44 PST, Alexey Proskuryakov
dbates: review+
Details | Formatted Diff | Diff
patch for landing (7.39 KB, patch)
2015-01-22 11:55 PST, Alexey Proskuryakov
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-01-21 20:31:22 PST
bing.com search results were causing a crash today.

rdar://problem/7881290
Comment 1 Alexey Proskuryakov 2015-01-21 20:44:14 PST
Created attachment 245113 [details]
proposed fixd
Comment 2 Daniel Bates 2015-01-22 10:08:44 PST
Comment on attachment 245113 [details]
proposed fixd

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

> Source/WebCore/ChangeLog:12
> +        state after failing to load. We get rid of the cache once main resource finishes

Nit: "...once main..." => "...once the main...".

> LayoutTests/http/tests/appcache/404-resource-with-slow-main-resource.php:1
> +<html manifest="resources/404-resource.manifest">

As far as I can tell this page doesn't depend on quicks mode. So, I suggest we add the HTML5 doctype to the top of this file: <!DOCTYPE html>.

> LayoutTests/http/tests/appcache/404-resource-with-slow-main-resource.php:13
> +    document.getElementById('result').innerHTML = 'SUCCESS';

Nit: This line used single quotes for string literals and the line below using double quotes. We should pick one style and stick with it for consistency.

> LayoutTests/http/tests/appcache/404-resource-with-slow-main-resource.php:21
> +// The test needs to attempt to load a subresource after appcache load fails. There is no way
> +// to observe appcache loading progress before onload fires, so we just let it run for a while.
> +window.setTimeout(finish, 1000);

:( I wish there was a better way to write this test and avoid this setTimeout(). I do not know of a better way at the moment.

> LayoutTests/http/tests/appcache/404-resource-with-slow-main-resource.php:24
> +<div id=container>

Nit: The quoting style used for HTML attributes on this line differs from the quoting style you used above (e.g. <div id="result">). We should pick one style and stick with it throughout the test for consistency. I suggest we surround the attribute value "container" in double quotes.

> LayoutTests/http/tests/appcache/404-resource-with-slow-main-resource.php:27
> +        usleep(100000);

How did you come to the decision to wait 100000 microseconds? Trial and error?
Comment 3 Alexey Proskuryakov 2015-01-22 11:55:04 PST
Created attachment 245153 [details]
patch for landing

Applied most comments, except for the one below.

> As far as I can tell this page doesn't depend on quicks mode. So, I suggest
> we add the HTML5 doctype to the top of this file: <!DOCTYPE html>.

The test doesn't depend on standards mode either, so I don't see what the benefit of adding a noise line to it would be.

> How did you come to the decision to wait 100000 microseconds? Trial and
> error?

This was the first value that I tried, and it worked. I don't think that it matters much; the 1 second timeout to finish is a lot more questionable, as it makes the test always take a whole second. As you said there doesn't appear to be any better way.
Comment 4 WebKit Commit Bot 2015-01-22 11:58:08 PST
Comment on attachment 245153 [details]
patch for landing

Rejecting attachment 245153 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 245153, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Dan Bates found in /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/4869605826756608
Comment 5 Alexey Proskuryakov 2015-01-22 13:05:48 PST
Committed <http://trac.webkit.org/r178937>.
Comment 6 Alexey Proskuryakov 2015-01-22 14:14:04 PST
The test was failing on Mavericks Release WK1, there was no console message about appcache load failure. Tweaked the timeouts, hoping that this would help.