RESOLVED FIXED 140755
Crash in URL::protocol() after appcache load fails 
https://bugs.webkit.org/show_bug.cgi?id=140755
Summary Crash in URL::protocol() after appcache load fails
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+
DetailsFormatted DiffDiff
patch for landing (7.39 KB, patch)
2015-01-22 11:55 PST, Alexey Proskuryakov 		commit-queue: commit-queue-
DetailsFormatted DiffDiff
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.