370195 - sql device for the offline cache

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Dave Camp (:dcamp)]

Attachment: v1

this patch adapts darin's sql cache device for the offline cache.

Currently it evicts entries that haven't been fetched for a specified number of days (defaults to 60).

Comment 1

[Dave Camp (:dcamp)]

Also, this doesn't yet have the securityinfo caching darin mentioned.

Comment 2

[Dave Camp (:dcamp)]

Attachment: v2

This version adds "ownership" to the offline cache device.  Unowned entries are evicted.  The prefetch service makes sure the ownership is up-to-date for <link rel="offline-resource"> links.

There's still a max size preference (set to 500megs) but it isn't actually implemented.

Comment 3

[Dave Camp (:dcamp)]

Attachment: v3

New version;  this one adds some helpers for managing the list of offline resources.

Comment 4

[Dave Camp (:dcamp)]

Attachment: v4

* Fixes the build if MOZ_STORAGE isn't defined (by disabling the offline cache build)

* Deals with out-of-space by dooming new entries (under the assumption that a currently-running web app can at least show the user that it wasn't able to sync, rather than an old app being pushed out)

* This version of the patch patches nsDiskCacheDeviceSQL.[h,cpp] rather than moving it to nsOfflineCacheDevice.cpp - after landing it might be nice to rename the file.

Comment 5

[Dave Camp (:dcamp)]

Attachment: v4.1

(other patch had a rogue unrelated file deletion)

Comment 6

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

Hm, do we really want this to take up 512 MB of users' disks?

netwerk/cache/public/Makefile.in

Your indentation seems inconsistent with the other lines

netwerk/cache/public/nsIOfflineCacheSession.idl

+                                                             Cache
+     * entries owned by a domain will not be purged from the offline cache.

What if the space limit is exceeded?

Hm... why the CStringArrayRef versions?

Also - what happens if the cache doesn't yet have an entry for the given key,
is the call essentially ignored or does it "take effect" as soon as such an
entry is added?

What happens if I attempt to remove a key that doesn't exist?

(please add the answers to these questions to the comments :) )

netwerk/cache/src/Makefile.in
+CPPSRCS += \
+	nsDiskCacheDeviceSQL.cpp

please add a $(NULL) on a new line

netwerk/cache/src/nsCacheService.cpp
+nsresult nsCacheService::SetOfflineOwnedKeys(nsCacheSession * session,

Code style here is to put the "nsresult" on its own line

+    if (session->StoragePolicy() != nsICache::STORE_OFFLINE) return NS_ERROR_NOT_AVAILABLE;

a linebreak would be nice

netwerk/cache/src/nsCacheSession.cpp

+NS_IMPL_ISUPPORTS2(nsCacheSession,
+                   nsICacheSession,
+                   nsIOfflineCacheSession)

perhaps you should only allow QI to nsIOfflineCacheSession when this is for an
offline session. you can do it with the interface map macros and
NS_INTERFACE_MAP_ENTRY_CONDITIONAL.

please prefer C++ casts over C-style ones, e.g. here:
+  *count = (PRUint32)keyArray.Count();
+  char **ret = (char**)nsMemory::Alloc(sizeof(char*) * (*count));

+    ret[i] = nsCRT::strdup(keyArray[i]->get());

wrong allocator, use NS_strdup (to go with NS_Free. nsCRT::strdup has another
free function)

+      nsMemory::Free((void*)ret);

no need to cast to void*

\ No newline at end of file

please fix :)

netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
+// helper function for directly exposing the same data file binding
+// path algorithm used in nsOfflineCacheBinding::Create

Could nsOfflineCacheBinding::Create be changed to use this function, to avoid
duplicating the algorithm? though... I note that you just moved this function,
so feel free to leave it as-is

+  *aDescription = nsCRT::strdup("Offline cache device");

Hm, this was wrong before... need to use NS_strdup here too (would be great if
you could fix this for all functions here that return strings via xpcom
interfaces)

-      do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
+      do_GetService("@mozilla.org/storage/service;1", &rv);

Why this change?

+  rv = mDB->CreateFunction("cache_eviction_observer", -1,
+                           new nsOfflineCacheEvictionFunction(this));

Why not pass 3 as the number of arguments?

Also, passing around objects with zero refcount makes me nervous... I'd store
the nsOfflineCacheEvictionFunction in an nsCOMPtr before passing it here

+    rv = binding->mDataFile->IsFile(&isFile);
+    NS_ENSURE_SUCCESS(rv, nsnull);
+    if (!isFile)

Isn't it much more likely that the function call fails than that it returns
false? I.e. isn't it more likely that the file doesn't exist than that it is
(say) a directory?

My point being: I think you should treat failure rv the same as !isFile

+    // mark as active
+    AutoResetStatement updateStatement(mStatement_UpdateEntryFlags);
+    rec.flags &= 0x1;

Shouldn't that be |=?

+nsOfflineCacheDevice::OpenInputStreamForEntry(nsCacheEntry      *entry,
                                            nsCacheAccessMode  mode,

Please fix the indentation of the parameters here

+      // the entry will overran the cache capacity, doom the entry
+      // and abort

s/overran/overrun/, right?

So once the cache is filled up, no new entries can be added?

+      NS_ASSERTION(NS_SUCCEEDED(rv),"DoomEntry() failed.");

please add a space after the comma

+  nsDependentCString clientIDStr(clientID);

If you move this some lines up, you can use it where you currently have
nsDependentCString(clientID)
(in SetOwnedKeys)

+nsOfflineCacheDevice::SetCapacity(PRUint32 capacity)
+{
+    mCacheCapacity = capacity;

Please put back the * 1024 here :)


uriloader/prefetch/Makefile.in
 		  pref \
+                  dom \
+                  unicharutil \

Inconsistent indentation here

uriloader/prefetch/nsPrefetchService.cpp
+static NS_DEFINE_IID(kCacheServiceCID, NS_CACHESERVICE_CID);

Well, firstly this should be _CID, but I'd prefer if you used the contract ID
instead

+/* XXX: is there really not a string split function anywhere?  Should something

nsCStringArray has one, but I'm not aware of a unicode split function, indeed.

+    nsAutoString subString;

you don't seem to use this variable

But it seems a lot simpler to move this code to the current code in
nsHTMLContentSink/nsContentSink that looks for these <link>s.

Comment 7

[Dave Camp (:dcamp)]

Attachment: v5

Comment 8

[Dave Camp (:dcamp)]

(In reply to comment #6)

Fixed stuff, some specific comments:

> Hm, do we really want this to take up 512 MB of users' disks?

I dropped this to 100 MB until we have the right management UI.

> netwerk/cache/public/nsIOfflineCacheSession.idl
> 
> +                                                             Cache
> +     * entries owned by a domain will not be purged from the offline cache.
> 
> What if the space limit is exceeded?
> 
> Hm... why the CStringArrayRef versions?
> 
> Also - what happens if the cache doesn't yet have an entry for the given key,
> is the call essentially ignored or does it "take effect" as soon as such an
> entry is added?
> 
> What happens if I attempt to remove a key that doesn't exist?
> 
> (please add the answers to these questions to the comments :) )

I reworked the comments in that file.
 
> +  *aDescription = nsCRT::strdup("Offline cache device");
> 
> Hm, this was wrong before... need to use NS_strdup here too (would be great if
> you could fix this for all functions here that return strings via xpcom
> interfaces)

I fixed that up for everything in netwerk/cache/src

> -      do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
> +      do_GetService("@mozilla.org/storage/service;1", &rv);
> 
> Why this change?

So, pulling storage/build/ into the necko tier to get that header would have dragged too much;  I suppose I could do it with a LOCAL_INCLUDE if you want.
> uriloader/prefetch/nsPrefetchService.cpp
> +static NS_DEFINE_IID(kCacheServiceCID, NS_CACHESERVICE_CID);
> 
> Well, firstly this should be _CID, but I'd prefer if you used the contract ID
> instead

Did that, and switched the other CIDs to use contract ids in that file too.

> But it seems a lot simpler to move this code to the current code in
> nsHTMLContentSink/nsContentSink that looks for these <link>s.

Moved that inot the content sink.

Comment 9

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

netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
   LOG(("nsOfflineCacheDevice::RemoveOwnedKey [cid=%s]\n", clientID));
[...]
+  if (isOwned) return NS_ERROR_NOT_AVAILABLE;

Shouldn't that be !isOwned?

uriloader/prefetch/nsPrefetchService.cpp
+            do_GetService(NS_CACHESERVICE_CONTRACTID,
+                          &rv);

I think this'd look nicer w/o this linebreak


I think you don't need the makefile changes in uriloader anymore, or the nsIDOM* headers

content/base/src/nsContentSink.cpp
+  if (FindInReadable(NS_LITERAL_CSTRING("#"), specStart, specEnd)) {

Should probably use FindCharInReadable here

However, I'm not a content/ peer, so a content peer will have to review that part too.

r=biesi on everything except content, sr=biesi on content

Comment 10

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

Comment on attachment 262560 [details] [diff] [review]
v5

Oh... forgot to mention:

in nsContentSink.cpp:

+  } else
+    offlineCacheSession->AddOwnedKey(ownerHost, ownerSpec, spec);

content/ style is to add {} even for single-line if bodies.

Comment 11

[Dave Camp (:dcamp)]

Attachment: v6

Updated for biesi's comments.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 264264 [details] [diff] [review]
v6

- In nsContentSink::AddOfflineResource(const nsAString &aHref):

+  nsCOMPtr<nsIURI> uri;
+  NS_NewURI(getter_AddRefs(uri), aHref,
+            charset.IsEmpty() ? nsnull : PromiseFlatCString(charset).get(),
+            mDocumentBaseURI);
+
+  // only http and https urls can be marked as offline resources
+  rv = uri->SchemeIs("http", &match);

Check that the NS_NewURI() call succeeded before you dereference the uri you created.

Other than that the content changes look good to me.

Comment 13

[Dave Camp (:dcamp)]

Attachment: v7

added a check on NS_NewURI().

Comment 14

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

checked in.

Comment 15

[Jim Jeffery not reading bug-mail 1/2/11]

Did this perhaps cause: https://bugzilla.mozilla.org/show_bug.cgi?id=380741