389223 - update the offline cache atomically

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Dave Camp (:dcamp)]

Attachment: first stab

To prevent spotty network connections from rendering an offline application unusable, resources should be updated in the cache as a group.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 273377 [details] [diff] [review]
first stab

netwerk/cache/public/nsICacheService.idl
+     * This is used by the offline cache.  The offline cache lets clients
+     * accumulate entries in a temporary client and merge them in as a group.

It seems like a good idea to reference nsIOfflineCacheSession.mergeTemporaryClient here, so that people don't have to search for it :)


netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
+    StatementSql( mStatement_ListOwners,        "SELECT DISTINCT Domain, URI FROM moz_cache_owners WHERE ClientID = ?;"),

missing space before (

+    StatementSql( mStatement_SwapClientID,       "UPDATE OR REPLACE moz_cache SET ClientID = ? WHERE ClientID = ?;")

here too


Hmm... Wouldn't this leave the old cache file lying around uselessly?

Comment 2

[Dave Camp (:dcamp)]

(In reply to comment #1)

> Hmm... Wouldn't this leave the old cache file lying around uselessly?

The eviction observer should be triggered on the old row when a new row replaces it.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 273377 [details] [diff] [review]
first stab

- In nsDOMOfflineLoadStatusList::UpdateCompleted():

+  for (PRUint32 i = 0; i < numItems; i++) {
[...]
+      nsresult rv = SendLoadEvent(NS_LITERAL_STRING(LOADCOMPLETED_STR),
+                                  mLoadCompletedEventListeners,
+                                  wrapper);
+      NS_ENSURE_SUCCESS(rv, rv);

Would it not make sense to keep going here even if sending one load event failed? If we return early here it means that load events won't be dispatched at all for other updates in this group. And the caller of this doesn't care about the return value (it's only called by an observer function), so you might as well keep going IMO.

- In nsDOMOfflineLoadStatusList::Observe():

-      rv = WatchUpdate(update);
+      rv = UpdateAdded(update);
       NS_ENSURE_SUCCESS(rv, rv);

Here...

+      rv = UpdateCompleted(update);
+      NS_ENSURE_SUCCESS(rv, rv);

and here you might as well ignore the return value as it's pointless to return it to the caller here.

- In netwerk/cache/public/nsICacheService.idl:

+    /**
+     * Return a unique, temporary cache client ID.
+     *
+     * This is used by the offline cache.  The offline cache lets clients
+     * accumulate entries in a temporary client and merge them in as a group.
+     */
+    ACString createTemporaryClient(in nsCacheStoragePolicy storagePolicy);

Maybe call this createTemporaryClientID(), as it's the ID that's being created, not the client. Or?

- In nsICachingChannel.idl:

+    attribute string offlineCacheClientID;

Any reason not to use ACString here to avoid forcing users of this to do manual free()s etc?

sr=jst

Comment 4

[Dave Camp (:dcamp)]

Attachment: as committed

Checking in dom/src/offline/nsDOMOfflineLoadStatusList.cpp;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.cpp,v  <--  nsDOMOfflineLoadStatusList.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in dom/src/offline/nsDOMOfflineLoadStatusList.h;
/cvsroot/mozilla/dom/src/offline/nsDOMOfflineLoadStatusList.h,v  <--  nsDOMOfflineLoadStatusList.h
new revision: 1.3; previous revision: 1.2
done
Checking in netwerk/base/public/nsICachingChannel.idl;
/cvsroot/mozilla/netwerk/base/public/nsICachingChannel.idl,v  <--  nsICachingChannel.idl
new revision: 1.13; previous revision: 1.12
done
Checking in netwerk/cache/public/nsICacheService.idl;
/cvsroot/mozilla/netwerk/cache/public/nsICacheService.idl,v  <--  nsICacheService.idl
new revision: 1.9; previous revision: 1.8
done
Checking in netwerk/cache/public/nsIOfflineCacheSession.idl;
/cvsroot/mozilla/netwerk/cache/public/nsIOfflineCacheSession.idl,v  <--  nsIOfflineCacheSession.idl
new revision: 1.3; previous revision: 1.2
done
Checking in netwerk/cache/src/nsCacheService.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsCacheService.cpp,v  <--  nsCacheService.cpp
new revision: 1.114; previous revision: 1.113
done
Checking in netwerk/cache/src/nsCacheService.h;
/cvsroot/mozilla/netwerk/cache/src/nsCacheService.h,v  <--  nsCacheService.h
new revision: 1.50; previous revision: 1.49
done
Checking in netwerk/cache/src/nsCacheSession.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsCacheSession.cpp,v  <--  nsCacheSession.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.cpp;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp,v  <--  nsDiskCacheDeviceSQL.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in netwerk/cache/src/nsDiskCacheDeviceSQL.h;
/cvsroot/mozilla/netwerk/cache/src/nsDiskCacheDeviceSQL.h,v  <--  nsDiskCacheDeviceSQL.h
new revision: 1.6; previous revision: 1.5
done
Checking in netwerk/protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.314; previous revision: 1.313
done
Checking in netwerk/protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.91; previous revision: 1.90
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.cpp;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp,v  <--  nsOfflineCacheUpdate.cpp
new revision: 1.3; previous revision: 1.2
done
Checking in uriloader/prefetch/nsOfflineCacheUpdate.h;
/cvsroot/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.h,v  <--  nsOfflineCacheUpdate.h
new revision: 1.2; previous revision: 1.1
done

Comment 5

[Dave Camp (:dcamp)]

Attachment: bustage fix

this patch was needed to fix the build on win32

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> (In reply to comment #1)
> 
> > Hmm... Wouldn't this leave the old cache file lying around uselessly?
> 
> The eviction observer should be triggered on the old row when a new row
> replaces it.

Sorry, saw this comment only now, but that is not the case:
http://www.sqlite.org/lang_conflict.html
"When this conflict resolution strategy deletes rows in order to satisfy a constraint, it does not invoke delete triggers on those rows. This behavior might change in a future release."