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

[bugzilla-daemon at webkit.org]

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


ap at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|New Bugs                    |WebKit Misc.
     Ever Confirmed|0                           |1
         OS/Version|Mac OS X 10.5               |All
           Platform|PC                          |All




------- Comment #6 from ap at webkit.org  2009-05-07 03:58 PDT -------
I'm yet to think through what happens here, but here are some preliminary
comments.

It would be easier to understand this patch if it had detailed per-function
comments in the ChangeLog.

+           
mainResourceCache->group()->associateDocumentLoaderWithCache(documentLoader,
mainResourceCache, false);

We are not doing this in code consistently, but we try to stop using booleans
as parameters in cases like this, which are difficult to read. You could define
an enum to have named parameters, or you could split this function in two.

+void ApplicationCacheGroup::associateDocumentLoaderWithCache(DocumentLoader*
loader, ApplicationCache* cache, bool wasPendingEntry)

There is a slight clash of terminology between HTML5 and WebCore - the pending
entries correspond to document loaders, not to any entries in m_pendingEntries
map. My choice was to have a comment before definition
m_pendingMasterResourceLoaders, and to avoid using the word "entry" for pending
master resource loaders. The argument name doesn't follow this convention.

What are the possible side effects of cacheStorage().storeNewestCache() that
need to be undone when this function returns false?

+    // The mapped flag is true if the loader was a 'pending master entry' and
false otherwise.

This comment could be a little more helpful if it had some sort of a hint at
why it is important to remember this forever (my guess is that it's transient
information that is only necessary during update, in which case it would be
best to keep it in a separate data member).

It's too bad that we don't have any way to test this. One way to simulate
errors would be to create a shim library for sqlite that would return errors
when asked to. That's out of the scope for this bug, but would be extremely
helpful for improving appcache robustness, as well as other uses of sqlite in
WebCore.


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