[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 #9 from andreip at google.com  2009-05-27 04:26 PDT -------
Hi Alexey,

Thanks a lot for the review. Back from vacation and a work trip so I can get
back to fixing this bug:

(In reply to comment #6)
> 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.
> 

Added.

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

Used an enum.

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

Fixed.

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

As far as I can tell, the following:

- The document loaders that correspond to the formerly-pedning master entries
need to be removed from m_associatedDocumentLoaders. We do this by calling
disassociateDocumentLoader().
- If there was an old newset cache for this application cache group, that needs
to be restored and the failed cache needs to be deleted.
- If there wasn't any old newset cache, then deleting the failed cache will
delete the groups as well.

> +    // 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).
> 

Added a comment above the new enum. I don't think we need to add a new member
for this.

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

The way I tested was to force a maximum size on the database file. We will be
able to do this properly when we fix 22700
(https://bugs.webkit.org/show_bug.cgi?id=22700) as that allows the chrome
client implementer to set the maximum size. The test cases can then try to
store something that's larger then the limit and cause SQLite to return an
error.

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.