[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
----------------------------------------------------------------------------
  Attachment #30880|                            |review+
               Flag|                            |
  Attachment #30880|1                           |0
        is obsolete|                            |




------- Comment #15 from ap at webkit.org  2009-06-03 05:25 PDT -------
(From update of attachment 30880)
> Index: WebCore/ChangeLog

There are tabs in ChangeLog that will need to be fixed before landing.

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

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

> +        LogRecord() : m_resource(0), m_storageID(0) {}

We usually put a space in "{ }".

> +        RefPtr<ApplicationCacheResource> m_resource;

Can this be a plain pointer, not a RefPtr?

> +            if (!store(it->second.get(), cacheStorageID)) {
>                  return false;
> +            }

We don't put braces around single line blocks.

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

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


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