[webkit-reviews] review denied: [Bug 25562] Potential crash after ApplicationCacheStorage::storeNewestCache() fails. : [Attachment 30017] Check return value of ApplicationCacheStorage::storeNewestCache()

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 25562: Potential crash after ApplicationCacheStorage::storeNewestCache()
fails.
https://bugs.webkit.org/show_bug.cgi?id=25562

Attachment 30017: Check return value of
ApplicationCacheStorage::storeNewestCache()
https://bugs.webkit.org/attachment.cgi?id=30017&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 Check return value of ApplicationCacheStorage::storeNewestCache()
+	 at its call site in ApplicationCacheGroup::checkIfLoadIsComplete().
+	 https://bugs.webkit.org/show_bug.cgi?id=25562
+
+	 * loader/appcache/ApplicationCacheGroup.cpp:
+	 (WebCore::ApplicationCacheGroup::checkIfLoadIsComplete):

I'd structure the ChangeLog differently:

	https://bugs.webkit.org/show_bug.cgi?id=25562
	Potential crash after ApplicationCacheStorage::storeNewestCache() fails


	* loader/appcache/ApplicationCacheGroup.cpp:
	(WebCore::ApplicationCacheGroup::checkIfLoadIsComplete):
	Check return value of ApplicationCacheStorage::storeNewestCache(), and
<...>
	if it failed.

+	     // We failed to store the new cache. Report an error.
+	     postListenerTask(&DOMApplicationCache::callErrorListener,
m_associatedDocumentLoaders);

Is this sufficient? I would expect cache failure steps to be run
<http://www.whatwg.org/specs/web-apps/current-work/#cache-failure-steps>, just
like in case when subresource loading fails. With this code, we just get a
non-stored newest cache, which will likely cause problems later on.

r-, because I think that cache failure steps need to be run here.