[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 #18 from andreip at google.com  2009-06-03 07:37 PDT -------
(In reply to comment #15)
> (From update of attachment 30880 [review])
> > Index: WebCore/ChangeLog
> 
> There are tabs in ChangeLog that will need to be fixed before landing.
> 

Fixed.

> >  void ApplicationCacheGroup::setNewestCache(PassRefPtr<ApplicationCache> newestCache)
> >  {
> > -    ASSERT(!m_caches.contains(newestCache.get()));
> > +    // We can set an old cache as the new cache. This can happen if we failed
> > +    // to store a newly downloaded cache to the database and we are now reinstating
> > +    // the former newest cache.
> >  
> >      m_newestCache = newestCache;
> 
> This comment will look a bit surprising - it would be perfect in ChangeLog, but
> it answers a question that wasn't asked for a person reading the code without
> assertion.
> 

Moved to Changelog.

> > +class ResourceStorageIDLogger {
> 
> All the "log" names look as if we were logging something to console. I'd
> probably say "journal", not "log", e.g. storageIDJournal.add() instead of
> logger.log(). Maybe there's a better yet name.
> 

Used "journal" instead.

> > +        LogRecord() : m_resource(0), m_storageID(0) {}
> 
> We usually put a space in "{ }".
> 

Added.

> > +        RefPtr<ApplicationCacheResource> m_resource;
> 
> Can this be a plain pointer, not a RefPtr?
> 

It can. I was over-cautious, I guess. The resources are guaranteed to live for
at least as long as the cache object does. But the cache should survive until
after storeNewsetCache() has returned.

> > +            if (!store(it->second.get(), cacheStorageID)) {
> >                  return false;
> > +            }
> 
> We don't put braces around single line blocks.
> 

Removed braces.

> Please consider addressing some of these comments, but this looks good enough
> to be landed as is, too.
> 

Sure, done.

> > But I will send an email to WHATWG / Hixie and ask this question while pointing to
> > the discussion in this bug.
> 
> Please do! It's great to get a well defined defined behavior where we used to
> crash - it would be even better to have an agreed upon behavior.
> 

Ok, sending in a bit.

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