Bug 40074 – ApplicationCacheStorage::storeNewestCache() Crash WebKit when openDatabase(true) failed

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

[Lyon Chen]

Right now in ApplicationCacheStorage.cpp when openDatabase(true) is called it always assume it will succeed, but it could fail, for example when appcache storage is set to a storage card and the storage card is now unplugged.

Plan to add m_database.isOpen() check after every openDatabase(true) call.

[Lyon Chen]

Created attachment 57674 [details]
patch for 40074 with coding style fix.

[Lyon Chen]

Created attachment 57708 [details]
patch for 40074 with updated comments.

[Darin Adler]

Normally we require regression tests for fixes. Is there some way to test this? How did you test it?

[Lyon Chen]

Hi, Darin, I actually got the crash first before I know this is an issue. I just set the storage location to somewhere nonexistent and then will get the crash without the fix.

(In reply to comment #3)

[Darin Adler]

(In reply to comment #4)
> Hi, Darin, I actually got the crash first before I know this is an issue. I just set the storage location to somewhere nonexistent and then will get the crash without the fix.

I'm not sure I understand fully. Is there way to turn that into a test other people can run? Maybe something in DumpRenderTree?

[Lyon Chen]

(In reply to comment #5)

I don't think we can test it with DumpRenderTree. And as there's no way to change the AppCache storage location of Chrome, Safari by user, I am not sure how to test this with these 2 browsers.
But for our test phone, I can use a SD card as AppCache location, and if that card is unplugged, then it will crash.

[WebKit Commit Bot]

Comment on attachment 57708 [details]
patch for 40074 with updated comments.

Rejecting patch 57708 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 2
Parsed 2 diffs from patch file(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/loader/appcache/ApplicationCacheStorage.cpp
patch: **** malformed patch at line 65: @@ -742,6 +750,10 @@ bool ApplicationCacheStorage::storeNewestCache(ApplicationCacheGroup* group)


Full output: http://webkit-commit-queue.appspot.com/results/3101012

[Lyon Chen]

Created attachment 57876 [details]
patch for 40074.

After reading the failed commit log, not sure exactly why it failed at line #65. But re-submit a new patch updated to newest master anyway.

[WebKit Commit Bot]

Comment on attachment 57876 [details]
patch for 40074.

Rejecting patch 57876 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 57876, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
cgi?id=57876&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=40074&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 57876 from bug 40074.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Lyon Chen]

Created attachment 57892 [details]
patch for 40074 with reviewer info updated

Update patch for 40074 with correct reviewer info.

[WebKit Commit Bot]

Comment on attachment 57892 [details]
patch for 40074 with reviewer info updated

Clearing flags on attachment: 57892

Committed r60729: <http://trac.webkit.org/changeset/60729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andrei Popescu]

IMHO, I think this can be and should be tested with a layout test. This can be similar to how we test for the app cache hitting the maximum size:

http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/appcache/max-size.html

One could add a method on the LayoutTestController to configure the storage location to some invalid path. I think that could trigger the bug.