[webkit-reviews] review denied: [Bug 25562] Potential crash after ApplicationCacheStorage::storeNewestCache() fails. : [Attachment 30702] Fix for 25562 (take 4)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 29 09:56:12 PDT 2009


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 30702: Fix for 25562 (take 4)
https://bugs.webkit.org/attachment.cgi?id=30702&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Discussed this on IRC in some depth. I have two technical concerns about this
patch:
1) Since failing ApplicationCacheStorage::store() leaves memory objects with
hanging StorageIDs, it's hard to prove that this broken invariant doesn't cause
problems later on.
2) There is a good deal of confusion about m_pendingMasterResourceLoaders and
"pending master entries", e.g. in comments about NewDocumentLoader. The names
New/OldDocumentLoader names are not good, which I think is a sign of using data
structures that don't match the logic well.

I'm really holding you up to a somewhat higher bar than customary, sorry for
being so picky. That's because appcache code is rather complicated, and because
this doesn't have automated testing to guarantee sustained progress in future
patches. So, we'd better get it right on the first try.

More globally, it doesn't seem obvious at all that reinstating the old cache is
indeed the best behavior. We already have a fully downloaded up to date
application in memory, why not use it? Looks like the spec should talk about
out of quota situations.


More information about the webkit-reviews mailing list