Bug 140755

Summary:

Crash in URL::protocol() after appcache load fails

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

WebCore Misc.

Assignee:

Alexey Proskuryakov <ap>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, commit-queue, dbates, japhet, joepeck

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
proposed fixd	dbates: review+
patch for landing	commit-queue: commit-queue-

Alexey Proskuryakov

Reported 2015-01-21 20:31:22 PST

bing.com search results were causing a crash today. rdar://problem/7881290

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2015-01-21 20:44:14 PST

Created attachment 245113 [details] proposed fixd

Daniel Bates

Comment 2 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?

Alexey Proskuryakov

Comment 3 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.

WebKit Commit Bot

Comment 4 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

Alexey Proskuryakov

Comment 5 2015-01-22 13:05:48 PST

Committed <http://trac.webkit.org/r178937>.

Alexey Proskuryakov

Comment 6 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.

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