[Webkit-unassigned] [Bug 25562] Potential crash after ApplicationCacheStorage::storeNewestCache() fails.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25562





------- Comment #13 from andreip at google.com  2009-06-02 13:57 PDT -------
Thanks Alexey,

(In reply to comment #11)
> (From update of attachment 30702 [review])
> 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.

Done.

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

Fixed.

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

Agreed.

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

IMHO, we should reinstate the old cache to match what is on the disk. Else, the
user will get the new version of the content after a failed update and will
fallback to the old content once the browser is restarted. But I will send an
email to WHATWG / Hixie and ask this question while pointing to the discussion
in this bug.

All the best,
Andrei


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.