Closed Bug 442813 Opened 16 years ago Closed 16 years ago

Implement fallback entries

Categories

(Core :: Networking: Cache, defect, P1)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: dcamp, Assigned: mayhemer)

References

()

Details

(Keywords: fixed1.9.1)

Attachments

(5 files, 9 obsolete files)

Implement fallback entries from the HTML5 offline apps spec.
Status: NEW → ASSIGNED
Attached patch First working draft (obsolete) — Splinter Review
This implements:
- opportunistic caching
- fallback on load error
- white list
Attachment #327662 - Flags: review?(dcamp)
Blocks: 443023
Comment on attachment 327662 [details] [diff] [review]
First working draft

>diff --git a/content/base/src/nsContentSink.cpp b/content/base/src/nsContentSink.cpp
>@@ -929,7 +930,13 @@
>   }
> 
>   // Start the update
>-  updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc, isTop);
>+  rv = updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc, isTop);
>+  if (NS_ERROR_DOCUMENT_IS_FOREIGN == rv)
>+  {
>+      nsCOMPtr<nsIWebNavigation> webNav(do_QueryInterface(pwindow->GetDocShell()));
>+      webNav->Stop(nsIWebNavigation::STOP_CONTENT | nsIWebNavigation::STOP_NETWORK);
>+      webNav->Reload(0);
>+  }
> }

Looks like this was meant for the cache selection algorithm patch.

>diff --git a/netwerk/base/public/nsIApplicationCache.idl b/netwerk/base/public/nsIApplicationCache.idl
>--- a/netwerk/base/public/nsIApplicationCache.idl
>+++ b/netwerk/base/public/nsIApplicationCache.idl
>@@ -44,7 +44,10 @@
>  * application cache belongs to a group of application caches
>  * associated with a given cache manifest URI.
>  */
>-[scriptable, uuid(a9cfbeef-8f8d-49c3-b899-303390383ae9)]
>+
>+interface nsIUTF8StringEnumerator;
>+
>+[scriptable, uuid(a3f53d2a-0439-4106-ace6-0fd9edbf44cc)]
> interface nsIApplicationCache : nsISupports
> {
>     /**
>@@ -115,6 +118,14 @@
>     void unmarkEntry(in ACString key, in unsigned long type);
> 
>     /**
>+     * 
>+     */
>+    void markFallbackAndNetworkEntries(
>+        in nsIUTF8StringEnumerator opportunisticNamespaces,
>+        in nsIUTF8StringEnumerator fallbackEntries,
>+        in nsIUTF8StringEnumerator networkEntries);

This method needs docs.  In particular, the fact that opportunisticNamespaces and fallbackEntries are parallel structures, while networkEntries is not, is kind of strange.  It should be at least documented, and possibly split into separate functions.

>+    /**
>      * Gets the types for a given entry.
>      */
>     unsigned long getTypes(in ACString key);
>@@ -125,8 +136,29 @@
>     void gatherEntries(in PRUint32 type,
>                        out unsigned long count,
>                        [array, size_is(count)] out string keys);
>+
>+    /**
>+     * Returns fallback entry mapped by the namespace mathing the participant

Typo, s/mathing/matching/.

>+     * if there is any such mapping. Otherwise returns empty string. Also checks
>+     * for presence of the participant on the whitelist.
>+     *
>+     * @param bypassForeign
>+     *      PR_TRUE: check the entry is not foreign, if so, empty string
>+     *          is returned.
>+     *      PR_FALSE: do not check the entry for foreign flag.
>+     * @param isOnWhitelist
>+     *      out, PR_TRUE when opportunisticNamespaceParticipant
>+     *      is on whitelist, otherwise PR_FALSE.
>+     */
>+    ACString fallbackOrWhitelistEntry(in ACString opportunisticNamespaceParticipant, 
>+            in PRBool bypassForeign, out PRBool isOnWhitelist);
>+
>+    /**
>+     * Walks all active caches for a namespace matching the 'key' and 
>+     * adds it to the matching cache as opportunistically cached entry.
>+     */
>+    void cacheOpportunistically(in ACString key);

I think this should be a method of nsIApplicationCacheService - it actually operates on every cache BUT the one serviced by the nsIApplicationCache.

There might be a more descriptive name too, like propagateOpportunisticCacheEntry() or something.

>diff --git a/netwerk/cache/src/Makefile.in b/netwerk/cache/src/Makefile.in
>--- a/netwerk/cache/src/Makefile.in
>+++ b/netwerk/cache/src/Makefile.in
>@@ -52,6 +52,7 @@
> 		  string \
> 		  necko \
> 		  pref \
>+		  prefetch \
> 		  $(NULL)
> 

I don't think this is necessary.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.cpp b/netwerk/protocol/http/src/nsHttpChannel.cpp
>--- a/netwerk/protocol/http/src/nsHttpChannel.cpp
>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>@@ -875,6 +875,53 @@
> 
>     LOG(("nsHttpChannel::ProcessNormal [this=%x]\n", this));
> 
>+    PRBool succeeded;
>+    rv = GetRequestSucceeded(&succeeded);
>+    if (NS_SUCCEEDED(rv) && !succeeded)
>+    {
>+        PRBool loadFromAppCache = PR_FALSE;
>+        if (mCacheEntry)
>+        {
>+            nsCacheStoragePolicy policy;
>+            nsresult rv = mCacheEntry->GetStoragePolicy(&policy);
>+            loadFromAppCache = (NS_SUCCEEDED(rv) && policy == nsICache::STORE_OFFLINE);
>+        }

Can we get in a situation where the request failed when loaded from the application cache?  The application cache is not supposed to have items that failed (5.7.4.8).

>+        if (!loadFromAppCache)
>+        {
>+            // Fallback URI is being seek during cache entry open.
>+            // If it is empty but we have a cache we are loading
>+            // whitelisted entry. Bypass fallback for it.
>+            if ((mLoadFlags & LOAD_INITIAL_DOCUMENT_URI) &&
>+                    !mApplicationCache &&
>+                    mFallbackURISpec.IsEmpty())
>+            {
>+                nsCAutoString cacheKey;
>+                GenerateCacheKey(cacheKey);
>+
>+                nsCOMPtr<nsIApplicationCacheService> appCacheService =
>+                    do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID);
>+
>+                if (appCacheService)
>+                    appCacheService->OpportunisticApplicationCache(
>+                            cacheKey, getter_AddRefs(mApplicationCache));
>+
>+                if (mApplicationCache)
>+                {
>+                    PRBool isOnWhitelist;
>+                    mApplicationCache->FallbackOrWhitelistEntry(cacheKey, PR_TRUE, &isOnWhitelist, mFallbackURISpec);
>+                }
>+            }
>+
>+            if (!mFallbackURISpec.IsEmpty())
>+            {
>+                rv = ProcessFallback();
>+                if (NS_SUCCEEDED(rv))
>+                    return rv;
>+            }
>+        }
>+    }

I might be reading something incorrectly, but I think this is wrong.  I think what we want is more like:

if (!mApplicationCache && flags include LOAD_CHECK_OFFLINE_CACHE) {
  appCacheService->OpportunisticApplicationCache(cacheKey, &mApplicationCache);
}

if (mApplicationCache) { // we're loading from an application cache
  PRBool isOnWhitelist;
  mApplicationCache->FallbackOrWhiltelistEntry(cacheKey, PR_TRUE, &isOnWhiteList, mFallbackURISpec);
}

if (!mFallbackURISpec.IsEmpty()) {
   ...
}

> nsresult
>+nsHttpChannel::ProcessFallback()
>+{
>+    nsresult rv;
>+
>+    LOG(("nsHttpChannel::ProcessNotModified [this=%x]\n", this)); 

Need to update the logging here.

>+    mURI = nsnull;
>+
>+    rv = NS_NewURI(getter_AddRefs(mURI), mFallbackURISpec);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    rv = mURI->GetAsciiSpec(mSpec);
>+    if (NS_FAILED(rv)) return rv;
>+
>+    if (mCacheEntry) {
>+        mCacheEntry->Doom();
>+        CloseCacheEntry();
>+    }
>+
>+    if (mOfflineCacheEntry) {
>+      mOfflineCacheEntry->Doom();
>+      CloseOfflineCacheEntry();
>+    }
>+    mCacheForOfflineUse = PR_FALSE;

We should probably do something to make sure that during an ACTUAL cache update (not an opportunistic one), this actually fails.

>     nsresult rv;
>@@ -1442,14 +1533,53 @@
>     }
> 
>     if (!mApplicationCache ||
>-        (NS_FAILED(rv) && rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION)) 
>+        (NS_FAILED(rv) && rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION))
>     {
>+        if (mApplicationCache) {

You probably want to include an !mCacheForOfflineUse clause here too - fallback and whitelisting shouldn't happen while updating the cache.

> 
>+    PRBool succeeded;
>+    if ((mLoadFlags & LOAD_DOCUMENT_URI) && NS_SUCCEEDED(GetRequestSucceeded(&succeeded)) && succeeded)
>+    {
>+        PRBool loadFromAppCache = PR_FALSE;
>+        if (mCacheEntry)
>+        {
>+            nsCacheStoragePolicy policy;
>+            nsresult rv = mCacheEntry->GetStoragePolicy(&policy);
>+            loadFromAppCache = (NS_SUCCEEDED(rv) && policy == nsICache::STORE_OFFLINE);
>+        }
>+
>+        if (!loadFromAppCache && mApplicationCache)
>+        {
>+            nsCAutoString cacheKey;
>+            GenerateCacheKey(cacheKey);
>+            mApplicationCache->CacheOpportunistically(cacheKey);
>+        }

This needs comments explaining what it's doing.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.h b/netwerk/protocol/http/src/nsHttpChannel.h
>@@ -294,6 +295,14 @@
>     // redirection specific data.
>     PRUint8                           mRedirectionLimit;
> 
>+    // Offline Web Application spec, section 4.7.5.1.4:
>+    // If the browsing context is assigned a cache and URL 
>+    // to load was not fetched from it but matched one of
>+    // its opportunistic namespaces, we store the fallback
>+    // entry URL to this member to be potentially used
>+    // on resource fetch error as a fallback URI.
>+    nsCString                         mFallbackURISpec;
>+

The spec changes a lot, I don't think it's safe to name a specific section number.

I didn't review the nsDiskCacheDeviceSQL.cpp changes, I'll do that tomorrow.
Attached patch Second working version (obsolete) — Splinter Review
All comments addressed except:
- cacheOpportunistically left in nsIApplicationCache because we need the client id which is being held inside of nsIApplicationCache. I also fixed hidden bug - OPPORTUNISTIC item type was not set nor carried to other caches.
- in method nsHttpChannel::ProcessNormal the condition is correct. I omit choose of opportunistic cache in case the mFallbackURISpec is not empty. It is actually an optimization, fallback uri is determined sooner in OpenCacheEntry.
Attachment #327662 - Attachment is obsolete: true
Attachment #327882 - Flags: review?(dcamp)
Attachment #327662 - Flags: review?(dcamp)
(In reply to comment #3)
> Created an attachment (id=327882) [details]
> Second working version
> 
> All comments addressed except:
> - cacheOpportunistically left in nsIApplicationCache because we need the client
> id which is being held inside of nsIApplicationCache. 

Couldn't it be nsIApplicationCacheService::CacheOpportunistically(nsIApplicationCache *sourceCache, nsACString key) ?

Depends on: 443450
Comment on attachment 327882 [details] [diff] [review]
Second working version

>+    StatementSql ( mStatement_OpportunisticallyCache,         "REPLACE INTO moz_cache (ClientID, Key, MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime, ItemType)"

This will create duplicate entries in moz_cache for a given file, but if you don't either refcounting or duplicate that file, the first one to be deleted will delete the file for everyone.

I personally wouldn't be opposed to leaving the implementation of CacheOpportunistically() for a followup bug - I think it's a bit of a corner case, and shouldn't hold up testing of the rest of this stuff.  But if you have a quick solution to this problem, feel free to include it.
Blocks: 444807
Attached patch v3 (obsolete) — Splinter Review
Comment 4 and comment 5 addressed in bug 444807. For now opportunistic caching is made just in the single cache, the one associated with the channel.
Attachment #327882 - Attachment is obsolete: true
Attachment #329137 - Flags: review?(jst)
Attachment #329137 - Flags: review?(cbiesinger)
Attachment #327882 - Flags: review?(dcamp)
Attachment #329137 - Flags: review?(bzbarsky)
Comment on attachment 329137 [details] [diff] [review]
v3

Please add -U 8 to your diff flags?

>+++ b/netwerk/base/public/nsIApplicationCache.idl
>+     * Adds to this cache mapping of opportunistic namespaces
>+     * to fallback entries and (independently on it) a list of 
>+     * entries on the whitelist. There is a single method because
>+     * it is usual the lists are stored in the same table and insert
>+     * might be surrounded by a single transaction (performance).

Perhaps something like:

  Add to this cache a mapping from opportunistic namespaces
  to fallback entries and an online whitelist.  These two setters
  are combined into a single method so that the underlying
  implementation can optimize storing them as needed.

That said, how much of a perf hit would it really be to do the two separate things?  How commonly would the methods be called?  How commonly would _both_ be called?  What's the lag inherent in two separate transactions?

It really feels to me like these should be two separate methods, not one.

>+     * @param opportunisticNamespaces, fallbackEntries
>+     *      arrays mapping namespace to a fallback. both arrays
>+     *      must be the same length and are processed together or
>+     *      may both be empty.

They're not arrays, they're enumerators.  How about:

  These enumerators provide a mapping from opportunistic namespaces
  to fallback entries.  They must produce equal numbers of strings
  (possibly 0), and for each string from the opportunisticNamespaces
  enumerator the corresponding string from the fallbackEntries
  enumerator is the fallback entry for that opportunistic namespace.  

It would also be nice to somehow document that all these things are URIs or URI prefixes, or point to the relevant part of the HTML5 spec, or something.

>@@ -125,8 +147,23 @@ interface nsIApplicationCache : nsISuppo

>+     * Returns fallback entry mapped by the namespace matching the participant
>+     * if there is any such mapping. Otherwise returns empty string. Also checks
>+     * for presence of the participant on the whitelist.

Perhaps

  Returns the fallback entry for the given URI, if there is one.
  Also checks whether the given URI is on the online whitelist.

That first string argument _is_ a URI, right?  Or something else?  It's got a long name that doesn't match anything in the spec; it would be good to have some documentation about what it is.

I assume that again this is a single method for performance reason?  Perhaps it would be better named gatherURIData or some such, just in case we add more data that needs fetching?

>+     * @param bypassForeign
>+     *      PR_TRUE: check the entry is not foreign, if so, empty string
>+     *          is returned.
>+     *      PR_FALSE: do not check the entry for foreign flag.

So this doesn't affect the isOnWhitelist part?  Also, I'd use |true| and |false| (and |bool| for the arg), and say:

  true: Don't return the fallback entry if it's foreign
  false: Return any fallback entry found, even it it's foreign

>+     * @param isOnWhitelist
>+     *      out, PR_TRUE when opportunisticNamespaceParticipant
>+     *      is on whitelist, otherwise PR_FALSE.

"is on the online whitelist"

Document that if isOnWhitelist is true then the return value will always be empty?

>@@ -165,4 +202,19 @@ interface nsIApplicationCacheService : n
>+    /**
>+     * Finds the best active cache for the key by traversing opportunistic
>+     * namespaces matching the 'key'.
>+     */
>+    nsIApplicationCache opportunisticApplicationCache(in ACString key);

What is "key" exactly?  Should this be a URI?  How do I tell what I should be passing to this method?  This is a problem with a lot of these interface changes...  It looks like "key" is the same as the key used on nsIApplicationCache.  It's worth documenting that.

Should this method be called findOpportunisticApplicationCache?

>+     * Sets opportunistic item type on the key.

  "Flags the key as being opportunistically cached"

perhaps?

>+     * This method is not part of the application cache
>+     * interface because when fully implemented
>+     * propagates the entry to all caches.

  "it needs to propagate the flag to all caches which match the key"?

Or something?  Or does it really hit _all_ caches?  Again, what is |key|?  Is it the nsIApplicationCache "key"?

>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
>+nsApplicationCache::FallbackOrWhitelistEntry(const nsACString &ns, PRBool bypassForeign,
>+                                PRBool *_isOnWhitelist, nsACString& _retval)

Please fix the indent. Also, what is |ns|?  It's named something quite different in the IDL.

I'm not sure about the use of '_' for out params.  If you feel a need to flag them, use outSomething.  And please give the retval a sane name that indicates what it really is.

>+NS_IMETHODIMP nsApplicationCache::MarkFallbackAndNetworkEntries(

File style is to put class::method on the line after the NS_IMETHODIMP.

>+  mozStorageTransaction transaction(mDevice->mDB, PR_FALSE);

We don't need to check mValid here?

>+  while (NS_SUCCEEDED(opportunisticNamespaces->HasMore(&hasMore)) && hasMore 
>+      && NS_SUCCEEDED(fallbackEntries->HasMore(&hasMore)) && hasMore)

Please put that second '&&' at the end of the first line?

>@@ -889,7 +939,7 @@ nsOfflineCacheDevice::Init()
>-                         "  Flags           INTEGER,\n"
>+                         "  Flags           INTEGER DEFAULT 0,\n"

Do we need to worry about existing profiles which have tables where it wasn't defaulted to 0?

>+                         " ClientID TEXT,\n"
>+                         " FallbackKey TEXT,\n"
>+                         " Whitelist INTEGER\n"

It might be good to document what this integer means.

Also, what does FallbackKey mean?  Isn't it a fallback entry URI?  Or is it a Key value for the moz_cache table?  If so, document that relationship.

Is this ClientID related to the other ClientID columns floating around?

>+    StatementSql ( mStatement_BindEntry,         "INSERT INTO moz_cache (ClientID, Key, MetaData, Generation, Flags, DataSize, FetchCount, LastFetched, LastModified, ExpirationTime) VALUES(?,?,?,?,?,?,?,?,?,?);" ), 

Please don't add the trailing whitespace here.

>+    StatementSql ( mStatement_FindFallbackEntry,       "SELECT NameSpace, FallbackKey, Whitelist FROM moz_cache_opportunistic_mapping WHERE ClientID = ? AND SUBSTR(?, 1, LENGTH(NameSpace)) = NameSpace ORDER BY SUBSTR(ClientID, -25) DESC, NameSpace DESC;" ),

Where does the magic -25 come from?  At least document that, though I suspect a less-fragile approach might also be better....

Also, document that the order on NameSpace is DESC so that longer namespaces will come before earlier ones (assuming that's the reason; if not, document the real reason).  And perhaps document why the ordering on the SUBSTR() thing is DESC?

Is it worth tossing a LIMIT 1 in here?

>+    StatementSql ( mStatement_FindOpportunisticClient, "SELECT DISTINCT ClientID FROM moz_cache_opportunistic_mapping WHERE SUBSTR(?, 1, LENGTH(NameSpace)) = NameSpace ORDER BY SUBSTR(ClientID, -25) DESC;" )

Again, document the -25 thing.  I'd really like to know where it's coming from.

>+nsOfflineCacheDevice::FallbackEntry(const nsACString &clientID,
>+                                  const nsACString &nsParticipant,
>+                                  PRBool bypassForeign,
>+                                  PRBool *_isOnWhitelist,
>+                                  nsACString &_retval)

Please fix the indent.

What's |nsParticipant|?

Again, ditch the '_' and give retval a better name.

>+    if (!*_isOnWhitelist && bypassForeign)

That first condition is always true, since if *_isOnWhitelist we returned, right?  Why even check it here?

>+nsOfflineCacheDevice::CacheOpportunistically(const nsACString &cid, 
>+                                    const nsACString &key)

Please fix the indent.  And that "cid" is what's called clientID elsewhere in this file, right?  Please use the same naming convention?

>+nsOfflineCacheDevice::AddFallbackMapping(const nsACString &clientID,
>+                       const nsACString &opportunisticNS,
>+                       const nsACString &fallbackEntry)

Please fix the indent.

>+  // Empty fallback entry means whitelist presence

That wasn't documented in the API...

Should this really be an INSERT and not INSERT OR REPLACE kinda thing?  How are we keeping this table from bloating?

>+NS_IMETHODIMP nsOfflineCacheDevice::OpportunisticApplicationCache(
>+    const nsACString & key, nsIApplicationCache **_retval NS_OUTPARAM)

Fix the line-breaking and indent.  And rename _retval to something meaningful.

Why the NS_OUTPARAM here but not elsewhere?

>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.h
>+  nsresult FallbackEntry(const nsACString &clientID,
>+                                  const nsACString &ns,
>+                                  PRBool bypassForeign,
>+                                  PRBool *_isOnWhitelist,
>+                                  nsACString &_retval);
>+  nsresult CacheOpportunistically(const nsACString &clientID,
>+                                  const nsACString &key);
>+  nsresult AddFallbackMapping(const nsACString &clientID,
>+                         const nsACString &opportunisticNS,
>+                         const nsACString &fallbackEntry);

Whichever of those strings don't come from IDL (clientID, say) should be nsCString or nsCSubstring instead.

And please fix the indents.

>+  nsresult AddNetworkEntry(const nsACString &clientID,
>+                         const nsACString &networkEntry);

This doesn't seem to be implemented anywhere.

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp

>+        // Fallback URI is being seek during cache entry open.

I can't figure out what that's trying to say.

> +    if (NS_SUCCEEDED(rv) && !succeeded) {

The spec says to also do fallback on network errors.  Do we?  Try a DNS error, for example.

>+        if ((mLoadFlags & LOAD_INITIAL_DOCUMENT_URI) &&
>+                !mApplicationCache &&
>+                mFallbackURISpec.IsEmpty()) 

Please fix the indent.

>+            nsCAutoString cacheKey;
>+            GenerateCacheKey(cacheKey);

So... why is this the right thing to pass in?  In particular, does this do what it should for POST requests?  Should we even be hitting this code for POST requests?

>+        if (!mFallbackURISpec.IsEmpty()) {
>+            rv = ProcessFallback();
>+            if (NS_SUCCEEDED(rv))
>+                return rv;

But if NS_FAILED(rv) then ProcessFallback has put us into a bogus state (e.g. null mURI!).  I don't think that's a good idea at all, since we're about to press on to other code.  If rv is failure, we should probably be bailing completely or something (doing onstart/onstop but not much else), no?

>+nsHttpChannel::ProcessFallback()
>+    rv = NS_NewURI(getter_AddRefs(mURI), mFallbackURISpec);

Do we want the fallback URI to end up in the URL bar?  If not, we need to do some more flag-munging here.

>@@ -1442,15 +1523,58 @@ nsHttpChannel::OpenCacheEntry(PRBool off
>+        if (mApplicationCache && !mCacheForOfflineUse) {
>+            // Don't allow fallbacks when fetching to offline cache (positive mCacheForOfflineUse flag).

The comments and code don't seem to match here.

>+            // We must check for opportunistic or whitelist match. If it fails we must fail the resource load.

Why?  This feels like it could use a bit more explanation of what exactly this code is doing and more importantly why it's doing it.

>+            PRBool onWhitelist;
>+            rv = mApplicationCache->FallbackOrWhitelistEntry(cacheKey, PR_FALSE, &onWhitelist, mFallbackURISpec);
>+            if (NS_FAILED(rv) || (mFallbackURISpec.IsEmpty() && !onWhitelist)) {

There are some serious 80-column issues here.

>+                    rv = session->OpenCacheEntry(mFallbackURISpec, 
>+                                        nsICache::ACCESS_READ, PR_FALSE,

I'd line-break before the "Open" there.

>+                    goto fallbackFromOfflineCache;

Can we please avoid goto? It makes the code a lot harder to maintain, especially as we go for more helper classes, etc.

>+    else if (mApplicationCache && !mCacheForOfflineUse)
>+        // XXX The condition is hack: we must allow network load when updating but 
>+        //     mCacheForOfflineUse is not exactly indication of that.

"is a hack".  Which "that"?  That we're updating?


I'm  not sure why exactly we're adding this code chunk.

>@@ -4479,6 +4608,23 @@ nsHttpChannel::OnStopRequest(nsIRequest 
>+    // When mCacheOpportunistically flag is set we must store/map the entry

"When the ..."

>+    // in all active application caches with matching opportunistic namespace.

Except we don't do that yet, right?

>+++ b/netwerk/protocol/http/src/nsHttpChannel.h
>+    PRUint32                          mCacheOpportunistically   : 1;

Document this member, please.


>+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp

I'm sorta skimming this part, since I don't have time to sort out the manifest format and double-check the parsing, so this is more of an sr than an r=.  I assume dcamp will handle the r= for this code.

>+          rv = mOpportunisticNSs[i]->Equals(fallbackURI, &equals);
>+          if (NS_FAILED(rv) || equals)
>+            break;
>+        }
>+        if (equals)
>+            break;

If NS_FAILED(rv), this is reading random memory.  That seems unnecessary.

>+        rv = NS_IsSchemeHostPortEqual(mURI, namespaceURI, &equals);

This needs to be updated to the changes in other bugs, no?

>+        if (NS_FAILED(mURI->SchemeIs(scheme.get(), &equals)) || !equals)

Why is this only a scheme check?

>     case PARSE_NETWORK_ENTRIES: {
>+        if (NS_FAILED(mURI->SchemeIs(scheme.get(), &equals)) || !equals)
>+            break;

Why is this only a scheme check?

>+nsOfflineCacheUpdate::CreateSpecEnumerator(nsCOMArray<nsIURI>& uris)
>+      nsCAutoString spec;
>+      uris[i]->GetAsciiSpec(spec);
>+      array->AppendCString(spec);

Make |spec| an nsCString, given what you do with it.

>@@ -1102,6 +1192,20 @@ nsOfflineCacheUpdate::AddExistingItems(P
>+                nsCAutoString oppNS;

This could use a better name.

>+                    pass = (oppNS.Compare(keys[i], PR_FALSE, oppNS.Length()) == 0);

StringBeginsWith() ?

The HTTP parts _really_ need biesi or darin to look at them...  I wouldn't bet on me catching mistakes in some of the control flow, even if I spent several days staring at it....
Attachment #329137 - Flags: review?(bzbarsky) → review-
I assume there will be tests for this stuff at some point before 1.9.1 final?
Attachment #329137 - Flags: review?(jst)
Attachment #329137 - Flags: review?(cbiesinger)
Attached patch v4 (obsolete) — Splinter Review
(In reply to comment #7)
> (From update of attachment 329137 [details] [diff] [review])
> >@@ -889,7 +939,7 @@ nsOfflineCacheDevice::Init()
> >-                         "  Flags           INTEGER,\n"
> >+                         "  Flags           INTEGER DEFAULT 0,\n"
> 
> Do we need to worry about existing profiles which have tables where it wasn't
> defaulted to 0?
> 

Removed, will be probably reintroduced in a different bug implementing opportunistic caching among all matching caches (bug 444807)

> >+    StatementSql ( mStatement_FindFallbackEntry,       "SELECT NameSpace, FallbackKey, Whitelist FROM moz_cache_opportunistic_mapping WHERE ClientID = ? AND SUBSTR(?, 1, LENGTH(NameSpace)) = NameSpace ORDER BY SUBSTR(ClientID, -25) DESC, NameSpace DESC;" ),
> 
> Is it worth tossing a LIMIT 1 in here?
> 

No, we need to walk and check (in some cases) if there is not a FOREIGN bit flag set on the entry in cache (another table). This could actually be implemented as a join or an additional condition. Would follow-up potentially be ok?

> >+            nsCAutoString cacheKey;
> >+            GenerateCacheKey(cacheKey);
> 
> So... why is this the right thing to pass in?  In particular, does this do what
> it should for POST requests?  Should we even be hitting this code for POST
> requests?

A cache key is here the right thing to pass to OpportunisticApplicationCache method, it is the cache key that the application cache service works with.

The opportunistic caching is not made for POST request now.

> >+    else if (mApplicationCache && !mCacheForOfflineUse)
> >+        // XXX The condition is hack: we must allow network load when updating but 
> >+        //     mCacheForOfflineUse is not exactly indication of that.
> 
> "is a hack".  
> I'm  not sure why exactly we're adding this code chunk.

It is not a hack (I previously didn't understand correctly). The flag is true exactly when we are writing to an offline cache entry during manifest entries fetch (the 'update process'). 

This condition is here to prevent fetch from network when there is assigned an offline cache to fetch from. The spac says to prevent all network traffic including HTTP expire or cache headers check.

> >+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp
> >+        if (NS_FAILED(mURI->SchemeIs(scheme.get(), &equals)) || !equals)
> 
> Why is this only a scheme check?
> 

Spec says to check just the schema. In the time this patch was written it was not known we have to remove reference from URL, now added.



Also, I changed the fallback to be a redirect with one exception: LOAD_REPLACE is not set what preserves the original uri (uri of the page that failed to load or is not existing) in the address bar and history. To allow security code and document.location to see the new URI I introduced new load flag LOAD_OPPORTUNISTIC_FALLBACK. When loacation is getting its URI it checks this flag and uses original URI of the channel. Also script security manager uses new function from nsNetUtils that uses the target (real) URL also when LOAD_OPPORTUNISTIC_FALLBACK flag is set.
Attachment #329137 - Attachment is obsolete: true
Attachment #334322 - Flags: review?(bzbarsky)
Is it at all possible to post an interdiff here?  That would make it a lot easier to review...
Attached patch interdiff to v4 (obsolete) — Splinter Review
> Would follow-up potentially be ok?

Yep.

> The opportunistic caching is not made for POST request now.

Good.

>+++ b/netwerk/base/public/nsIApplicationCache.idl
>+     *      If isOnWhitelist is true the result value is empty - items
>+     *      cannot match opportunistic namespace and be on the white
>+     *      list simultaneosly.

  "... cannot match an opportunistic namespace and ..."

>+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
>@@ -923,36 +998,67 @@ nsOfflineCacheDevice::Init()
>+  // Whitelist: this integer can be threated a boolaen expression indicating

"treated as a boolean expression"

>+  // presence of the NameSpace on the white list. This is pre-implementation
>+  // allowing white listing the same way as opporunistic caching.

"This allows us to implement whitelisting using the mechanism we use for opportunistic caching"

>+    StatementSql ( mStatement_FindFallbackEntry,       "SELECT NameSpace, FallbackURI, Whitelist FROM moz_cache_opportunistic_mapping WHERE ClientID = ? AND SUBSTR(?, 1, LENGTH(NameSpace)) = NameSpace ORDER BY NameSpace DESC;" ),

This still needs documentation about why ORDER BY is by "NameSpace DESC".

>+    StatementSql ( mStatement_InsertFallbackEntry,     "INSERT INTO moz_cache_opportunistic_mapping (ClientID, NameSpace, FallbackURI, Whitelist) VALUES(?, ?, ?, ?); "),

Again, how are we avoiding bloat from repeated rows here?

>+nsOfflineCacheDevice::AddNetworkEntry(const nsCSubstring &clientID,
>+                         const nsCSubstring &networkEntry)

Please fix the indent here.

>+nsOfflineCacheDevice::OpportunisticApplicationCache(
>+    const nsACString &key,
>+    nsIApplicationCache **out)

And here.  You should fit in 80 columns, no?

>+nsOfflineCacheDevice::CacheOpportunistically(nsIApplicationCache* cache,
...
>+}
> nsresult

Need a blank line there, no?

>+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp
>+nsHttpChannel::ProcessFallback(PRBool *outFallbackPerformed)
> +    // Fallback URI is determined during cache entry open,

mFallbackURISpec ?

>+    // a cache we are loading whitelisted entry. Do not search

"a whitelisted entry"

>+    // for fallback URI now and completely bypass fallback for

"a fallback URI"?  Or "the"?  Is there only one possible one at this point?

>+    // failed load of URI on the white list.

"a failed load"?

>+            appCacheService->OpportunisticApplicationCache(

I still think this should be called FindOpportunisticApplicationCache.

>@@ -1420,29 +1496,85 @@ nsHttpChannel::OpenCacheEntry(PRBool off
>+            // "modification to networking model". When the resource is not
>+            // on the list specified by the manifest in (i.e. is not the

I'm not sure what that "in" is doing there.

>+            // already opportunistically cached item - cheked above) and

"checked"

>+                    // Automatically store this resourece in the offline cache

"resource"

> nsHttpChannel::SetupReplacementChannel(nsIURI       *newURI, 
>+    if (preserveURI) {

I would like all the stuff dealing with this split out into a separate patch, either as followup or preceding or whatever, for several reasons:

1)  I don't like it using up one of the few remaining channel flags we have.
2)  It's violating exisiting URI equality invariants, which means reviewing it requires a very careful code audit of all code that looks at channel URIs, channel original URIs, document URIs, docshell URIs, principals, window.location (since you're making those no longer match in all sorts of odd ways).  There's no way I'll be able to do such an audit before freeze, and I sort of doubt you've done it (though I'd be happy to be wrong).
3)  In general, I intensely dislike this business of "show one URI but behave as if you were loaded from a different URI for security purposes", and would rather that were reconsidered than mess around with security invariants.

I should note that in any case your code here doesn't match your IDL comments in nsIChannel.idl...

>@@ -4478,26 +4641,62 @@ nsHttpChannel::OnStopRequest(nsIRequest 
>+    if (NS_FAILED(mStatus) &&
>+        !mCanceled && // Don't fallback when canceled.
>+        !mLogicalOffset) { // It is undesirable to fall back in a middle of load

"in the middle".

Should we be falling back if mStatus is success but we got 404?

In any case, won't calling ProcessFallback mean more calls to OnStartRequest, etc?  That seems to violate the channel contract.

>+nsOfflineCacheUpdate::AddExistingItems(PRUint32 aType,
>+                                        nsCOMArray<nsIURI>* opportunisticNSfilter)

The indent here is off.

>+            for (PRInt32 j = 0; j < opportunisticNSfilter->Count() &&
>+                 !pass; ++j) {

Better to just put the init/condition/increment on three lines, so it's clear which is which.
Attachment #334322 - Flags: review?(bzbarsky) → review-
> I would like all the stuff dealing with this split out into a separate patch,
> either as followup or preceding or whatever, for several reasons:

As you said, I didn't do such audit, because I am not that deep expert to the code. I will separate the "URI play" part. Is it ok for you to implement fallback as a redirect w/o preserving original URI in the address bar and fix that 'detail' in the above mentioned follow-up patch?
Attached patch v5 (obsolete) — Splinter Review
Dave Camp and me made a new version. It reflects comment about the moz_cache_opportunistic_mapping table bloat (honzab+dcamp), fixes an opportunistic caching issue (top level documents was not opportunistically cached) (dcamp), deeply optimizes table indexes and queries (honzab) and reverts back the special behavior of redirect (dcamp). Fallback is now implemented as a redirect that is not (as the spec says) preserving original URI in the address bar and in a history entry.

Interdiff will follow.
Attachment #334322 - Attachment is obsolete: true
Attachment #334488 - Attachment is obsolete: true
Attachment #337283 - Flags: review?(bzbarsky)
Comment on attachment 337283 [details] [diff] [review]
v5

>--- a/uriloader/prefetch/Makefile.in
>+++ b/uriloader/prefetch/Makefile.in
>@@ -46,20 +46,23 @@ LIBRARY_NAME	= prefetch_s
> LIBRARY_NAME	= prefetch_s
> LIBXUL_LIBRARY	= 1
> REQUIRES	= xpcom \
> 		  dom \
> 		  string \
> 		  necko \
> 		  uriloader \
> 		  nkcache \
> 		  chardet \
> 		  pref \
>+		  caps \
>+		  xpconnect \
>+		  js \
> 		  $(NULL)

I don't think these changes are necessary.
(In reply to comment #16)
> I don't think these changes are necessary.

I will check and remove it after the review.
Flags: blocking1.9.1?
Attachment #337283 - Flags: review?(bzbarsky)
Attached patch v6 (obsolete) — Splinter Review
This is a somewhat significant reworking of the patch:

a) Rather than redirect, this patch just loads the fallback cache entry in the original channel.  There seems to be at least rough consensus on the whatwg list that this is the correct way to implement fallback.
b) There have been some proposals on the list to change namespace handling - making whitelist/bypass entries a namespace (rather than an exact-uri match), separating fallback from auto-caching behavior, etc.  This patch reworks the namespace support to more easily accommodate those changes.

Since a good chunk of this patch is http channel/cache munging, and bz asked us to get review from biesi on that stuff, I'm switching review over to him.  But I'd welcome a drive-by if you're interested, Boris.
Attachment #337283 - Attachment is obsolete: true
Attachment #339507 - Flags: review?(cbiesinger)
Attached patch v6.1 (obsolete) — Splinter Review
Come to think of it, we shouldn't need AsyncHandleFallback anymore, ProcessFallback should be fine.  It isn't a redirect anymore, it just calls ReadFromCache()).
Attachment #339507 - Attachment is obsolete: true
Attachment #339806 - Flags: review?(cbiesinger)
Attachment #339507 - Flags: review?(cbiesinger)
Comment on attachment 339806 [details] [diff] [review]
v6.1

+++ b/netwerk/base/public/nsIApplicationCache.idl
+    void Init(in unsigned long itemType,

should have a lowercase i

+    readonly attribute ACString namespaceSpec;
+    readonly attribute ACString data;

shouldn't those be UTF-8 strings, given that they can contain IDNs?

+++ b/netwerk/base/public/nsIApplicationCacheService.idl
+     * This method should also propogate the entry to other

propogate -> propagate

+     * @param cache
+     *      The cache in which the entry is cached now.

Can you make the comment line up with the variable name? (I.e. put "The" under "cache")

Similar for the other parameter

+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp

+#include "nsNetCid.h"

uppercase CID. helps with compilation on linux :-)

+NS_IMPL_ISUPPORTS1(nsApplicationCacheNamespace, nsIApplicationCacheNamespace);

shouldn't have a semicolon

--
will continue later
Comment on attachment 339806 [details] [diff] [review]
v6.1

+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.cpp
+  rv = mDB->ExecuteSimpleSQL(NS_LITERAL_CSTRING("DELETE FROM moz_cache_namespaces WHERE rowid IN (SELECT moz_cache_namespaces.rowid FROM moz_cache_namespaces LEFT OUTER JOIN moz_cache_groups ON (moz_cache_namespaces.ClientID = moz_cache_groups.ActiveClientID) WHERE moz_cache_groups.GroupID ISNULL)"));

What happened to the 80 column limit :/ But this isn't the first line to do that

+nsOfflineCacheDevice::CacheOpportunistically(nsIApplicationCache* cache,
+                                             const nsACString &key)
+{
+  NS_ENSURE_ARG(cache);

NS_ENSURE_ARG_POINTER?

+++ b/netwerk/cache/src/nsDiskCacheDeviceSQL.h
+  virtual ~nsApplicationCacheNamespace() {}

No need for that to be virtual and public.

+  NS_SCRIPTABLE NS_METHOD Init(PRUint32 itemType,

should be NS_IMETHOD instead of NS_METHOD, also for the other functions on this interface. That said, I'm not really a fan of implementing virtual functions inline, can you move them to the cpp file?

--
will continue later
Attached patch v7 (obsolete) — Splinter Review
New version incorporates review comments so far, and on biesi's suggestion implements fallbacks as a new channel rather than a new cache entry in the existing channel.

Interdiff coming...
Attachment #339806 - Attachment is obsolete: true
Attachment #340970 - Flags: superreview?(cbiesinger)
Attachment #340970 - Flags: review?(cbiesinger)
Attachment #339806 - Flags: review?(cbiesinger)
Attached patch v6.1 -> v7Splinter Review
+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp	Mon Sep 29 12:24:00 2008 -0700
+                // If we have a fallback URI (and we're not already

fallack -> fallback

+                if (!mFallbackChannel && !mFallbackKey.IsEmpty()){

Missing space before {

+    // transfer application cache information
+    if (mApplicationCache) {
+        nsCOMPtr<nsIApplicationCacheContainer> appCacheContainer;
+        if (appCacheContainer) {
+            appCacheContainer->SetApplicationCache(mApplicationCache);

??

What is this supposed to do? Because clearly it's not doing it, whatever it is.


+++ b/netwerk/protocol/http/src/nsHttpChannel.h	Mon Sep 29 12:24:00 2008 -0700
+   // True if mCacheForOfflineUse was set because we were caching

Incorrect indentation


+++ b/uriloader/prefetch/nsOfflineCacheUpdate.cpp	Mon Sep 29 12:24:00 2008 -0700
+        if (NS_FAILED(DropReferenceFromURL(uri)))
+            break;

Hm, you're modifying URIs here that were created in a completely different
place, I think that could be unexpected for the caller...

+    case PARSE_BYPASS_ENTRIES: {

This one doesn't need a security check?



+++ b/uriloader/prefetch/nsOfflineCacheUpdate.h
+    already_AddRefed<nsIArray> GetNamespaces()
+        { NS_IF_ADDREF(mNamespaces); return mNamespaces.get(); }

Is there a reason why you don't just return an nonaddrefed nsIArray*?

+    nsCOMPtr<nsIMutableArray> mNamespaces;

Can you add a comment on what the type of the entries is?

+    nsresult AddExistingItems(PRUint32 aType,
+                              nsTArray<nsCString>* namespaceFilter = nsnull);

Can you add a comment on how the filter will be used by this function?
Attachment #340970 - Flags: superreview?(cbiesinger)
Attachment #340970 - Flags: superreview+
Attachment #340970 - Flags: review?(cbiesinger)
Attachment #340970 - Flags: review+
Sorry that this took so long! And thanks for attaching the interdiff :)
Attachment #340970 - Attachment is obsolete: true
(In reply to comment #24)

> +    case PARSE_BYPASS_ENTRIES: {
> 
> This one doesn't need a security check?

No, it doesn't.  Bypass entries just specify a set of pages that don't follow the application cache rules (they follow normal network load rules).  The spec calls for a same-scheme check (which we do), but not a same-origin check.
Landed as http://hg.mozilla.org/mozilla-central/rev/13620c17990b
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attached patch bustage fixSplinter Review
Attachment #341147 - Flags: superreview?(cbiesinger)
Attachment #341147 - Flags: review?(cbiesinger)
Bustage fix restores the return NS_ERROR_DOCUMENT_NOT_CACHED when LOAD_ONLY_FROM_CACHE is specified.
Attachment #341147 - Flags: superreview?(cbiesinger)
Attachment #341147 - Flags: superreview+
Attachment #341147 - Flags: review?(cbiesinger)
Attachment #341147 - Flags: review+
Landed before 1.9.1 branched
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.