I actually know very little about how application caching (for offline mode, etc.) works, and how best to make it work with e10s. I hear Honza is familiar with this stuff, so assigning to him. Honza, could you map out a high-level summary of how application caching works, and what you expect we'll need to do to get it working in e10s? General pointers to docs on application caching (is there an RFC? etc.) would also be useful. Note that this is not regular HTTP caching, which should (fingers crossed!) "just work" without modification for e10s.

In a nut shell: offline web application caching (fetching) is engaged in both online and offline mode (w/ or w/o a net connection). A web page (the main page of an offline web app) refers a manifest. That is a text file listing URLs you wish to cache. When user allows "offline application permission" for the page's domain we run so called "application cache update process" that fetches the manifest and files by the list it defines. It happens in [1] invoked from [2]. From that moment the page and all other resources it refers to are fetched from the offline cache, exclusively. A short brief of web app cache is in [3]. nsHttpChannel is querying a cache session (of 3 possible kinds: memory, disk, offline) for a cache entry (identified simply by URL of the resource, interface nsICacheEntryDescriptor). When an application cache is associated with the channel, we use an offline cache session to get a cache entry. From that moment we work with an instance of nsICacheEntryDescriptor the same way as it would be a 'normal' HTTP cache entry. So, the difference how we work with an offline cache in nsHttpChannel is only in the way we obtain a cache entry. There are two ways nsHttpChannel gets associated with an application cache to start using it: 1. it is told to search explicitly for an application cache by doc shell in nsDocShell::DoURILoad when an offline application permission is allowed for domain of the URI. In nsHttpChannel::OpenCacheEntry we query nsIApplicationCacheService for instance of nsIApplicationCache that is our internal interface representing the cache group (cache group is identified by URL of the manifest). 2. it is told to inherit an application cache from its load context. It queries its notification callbacks for an application cache container. Actually, it does GetInterface for nsIApplicationCacheContainer on nsDocShell, which QIs the associated document that is our application cache container. nsHttpChannel then calls GetApplicationCache on it. If found nsIApplicationCache instance is found, we query it for a cache session and then get a cache entry from that session and use it to fetch from, that's all. By your last note (which is not fully true by the way ;), it seems that if the application cache association gets work, we are done. I expect that all the code I describe above is called on the parent process (opening a cache entry and querying notification callbacks). If it's so, then there is not much left to do. [1] http://mxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsOfflineCacheUpdate.cpp [2] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1181 [3] http://www.w3.org/TR/offline-webapps/

So to get the DocShell:DoOpenURI behavior, it sounds like we'll need to have HttpChannelChild implement nsIApplicationCacheChannel, and to send a boolean to the parent during AsyncOpen and pass it to SetChooseApplicationCache on the chrome channel. I'm not sure from Honza's comment where/when the alternate method occurs (#2, docshell gets QI'd to a nsIApplicationCacheContainer). If it's within chrome, we'll need to make HttpChannelParent QI to nsIApplicationCacheContainer (or not) as the sitation requires. From reading over the HTML 5 summary you sent (thanks), I see that app caching also involves SQL queries for getting/setting data, and also notifications for online/offline status, and a number of other things that might require IPC traffic ("provides a way for scripts to add and remove entries from the cache dynamically, and a way for applications to atomically update their cache to new files, optionally presenting custom UI during the update."). I assume these are all part of this bug, or are some of these (getting SQL queries to work cross-process) going to be dealt with and/or overlap with other work? CC-ing Shawn only because my brain returns his name when I "SELECT * from people_who_I_suspect_know_about_SQL_innards_in_Mozilla" :)

We haven't implemented the SQL storage backend, and I don't think we have any immediate plans to do so. There are several remaining pieces: * .localStorage (previously called .globalStorage). This doesn't have anything to do with necko and should have its own bug, although it's related. * offline cache. This is what this bug should be about. It's especially important on mobile devices * other notifications, such as offline/online status notifications. These are important, but probably should have separate bugs.

(In reply to comment #2) > I'm not sure from Honza's comment where/when the alternate method occurs (#2, > docshell gets QI'd to a nsIApplicationCacheContainer). If it's within chrome, > we'll need to make HttpChannelParent QI to nsIApplicationCacheContainer (or > not) as the sitation requires. I probably didn't document that very well. In nsDocShell::DoURILoad we set two properties on nsIApplicationCacheChannel. See [1]. This is the only place we drive the application cache decisions. Everything else is handled by the channel itself. When the channel is setup to inherit an application cache, *it QIs (GIs) its notification callbacks for nsIApplicationCacheContainer*, see [2], that, if present, gives us nsIApplicationCache object, that, if present, we get our application cache identifier from. This all happens during call to nsHttpChannel::AsyncOpen. [1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#8222 [2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#1700

(In reply to comment #2) > So to get the DocShell:DoOpenURI behavior, it sounds like we'll need to have > HttpChannelChild implement nsIApplicationCacheChannel, and to send a boolean to > the parent during AsyncOpen and pass it to SetChooseApplicationCache on the > chrome channel. > To make this clear to me, DocShell is gonna run on the content process functioning closely to how it works now in non-ipc code, right? If so, does child DocShell have access to the permission manager which is needed to decide if to use or not an application cache? As for the implementation plan: We may not access the application cache object (instance of nsIApplicationCache) on the content process, because to instantiate it we need access to the profile data, I assume inaccessible on the content process. So, to implement the cache selection algorithm that is about to run on the content process we may only work with cache groupID only (just a string, URL of the manifest, that uniquely represents the cache) in nsContentSink and nsDocument. I checked this. We have to propagate loadedFromApplicationCache attribute back to the channel child, to make this work. The parent channel actually just needs to get the application cache object (fill its mApplicationCache member). If we met conditions to INHERIT the application cache, we may tell the parent channel directly in HttpChannelChild::AsyncOpen to use a particular groupID to query for a cache later on the parent process. This would turn attribute boolean inheritApplicationCache to attribute string useApplicationCacheGroupID; If we want to let the channel CHOOSE the cache, we just set the flag via an existing interface, att boolean chooseApplicationCache, and use existing code. We'll probably need to do some ipc from nsDOMOfflineResourceList (on the content process) to the parent process, from methods nsDOMOfflineResourceList::SwapCache and GetStatus. Only thing I still don't know exactly how to do is how the cache update will get scheduled (IPC?) and how is it going to associate the document with an updated/new cache (also IPC?). Removing dep on 536292 as it is not needed after this analyzes. CC'ing Michal, we may probably work on this in parallel.

This is just a pre-release, not much tested, but basics seem to be working. Don't worry about the patch size, I have split one of a big files to 3 by making two duplicates of the original, so there is a lot of code deletion in the patch. What has not been changed: - the cache selection algorithm code, only slight changes; a document referring a manifest invokes a cache update or first fetch as before - content process is still working with nsIApplicationCache internal representation interface, but in a limited way: objects just hold the cache identifiers (group /a manifest/ id and client /a version/ id) What has been changed: - there is a new POfflineCacheUpdate protocol, managed by the content process - the relation is one document demanding an update to a one child update instance; the child is responsible for associating the document with the cache after the cache has been fetched - parent(s) observe a single update instance not much changed from the original version, just not directly working with document objects - window.applicationCache.state is now based on the update notifications; otherwise lot of rpc/sync would be need (so far not much tested) What is unsupported or broken: - dynamic items, that are anyway mozilla specific and not part of the w3c spec anymore (fix or remove completely in a different bug) - permission manager: deps on bug 564535, not sure of the permission UI bar to allow offline application caching for a domain, now hardcoded: every domain has offline privileges - missing notification after the document has finished loading (bug 566388), not critical (I invoke an update immediately after the cache selection algorithm) but should be fixed anyway - no backward compatibility with a single process version, not a big deal to finish (probably in a different bug)

Updates from the previous version: - backward compatibility with non-ipc build option - the update protocol is managed by IFrameEmbeding - some small enhancements

The hunk [1] can be removed from the patch. I have verified that bug 566388 fixed that issue. [1] https://bugzilla.mozilla.org/attachment.cgi?id=447762&action=diff#a/uriloader/prefetch/nsOfflineCacheUpdate.cpp_sec6

Comment on attachment 447762 [details] [diff] [review] v1 (pre-release) Assigning to Bjarne for +r, then biesi for +sr.

Merged to the current e10s tree.

Comment on attachment 453875 [details] [diff] [review] v1 - merged cbiesinger, can you take a first pass at this?

This is Fennec 2.0 Beta 1 blocker. We need to move this forward. So: - should I rebase the patch, or - should someone review/look at the current patch first?

If it needs rebasing, please do so. I'll start reviewing this tomorrow.

Punting on this for a little bit, in favor of b1 stuff. Will get back to it soon.

We want this in by Friday (!) if possible. I'm reviewing now. Honza, do you have time to update this over the next couple days if necessary?

I will be able to, I was so far stuck with some localStorage stuff, Fx4 blocking, now done.

Comment on attachment 453875 [details] [diff] [review] v1 - merged >diff --git a/content/base/src/nsContentSink.cpp b/content/base/src/nsContentSink.cpp >@@ -1165,23 +1132,27 @@ nsContentSink::ProcessOfflineManifest(co >+ // XXX TODO - get this information from the parent somehow, maybe through >+ // the channel or ... ? >+#if 0 > if (!nsContentUtils::OfflineAppAllowed(mDocument->NodePrincipal())) { > return; > } >+#endif The permission manager works in the child now, so you should be able to do this. >@@ -1212,20 +1183,27 @@ nsContentSink::ProcessOfflineManifest(co > > if (updateService) { > nsCOMPtr<nsIDOMDocument> domdoc = do_QueryInterface(mDocument); > updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc); > } > break; > } > case CACHE_SELECTION_RELOAD: { > // This situation occurs only for toplevel documents, see bottom > // of SelectDocAppCache method. >+ // The document has been loaded from a different offline cache group then 'than'. >diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp >@@ -5691,21 +5691,22 @@ nsDocShell::OnRedirectStateChange(nsICha >- appCacheChannel->SetChooseApplicationCache(ShouldCheckAppCache(newURI)); >+ // Permission will be checked in the parent process. >+ appCacheChannel->SetChooseApplicationCache(PR_TRUE); This can happen in the child now, but if you'd rather do it in the parent then that's fine too. >@@ -8332,22 +8317,22 @@ nsDocShell::DoURILoad(nsIURI * aURI, >- // application cache. >- appCacheChannel->SetChooseApplicationCache(ShouldCheckAppCache(aURI)); >+ // application cache. Permission will be checked in the parent process >+ appCacheChannel->SetChooseApplicationCache(PR_TRUE); Same here. >diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp >@@ -7889,20 +7889,36 @@ nsGlobalWindow::Observe(nsISupports* aSu >+ if (!nsCRT::strcmp(aTopic, "offline-cache-update-added")) { >+ if (mApplicationCache) >+ return NS_OK; >+ >+ // Instatiate the application object now. It observes update beloging to >+ // this window's document and corrently then updates the applicationCache 'Instatiate', 'beloging', 'corrently'? >+ // object state. >+ nsCOMPtr<nsIDOMOfflineResourceList> applicationCache; >+ GetApplicationCache(getter_AddRefs(applicationCache)); >+ nsCOMPtr<nsIObserver> observer = do_QueryInterface(applicationCache); >+ if (observer) >+ observer->Observe(aSubject, aTopic, aData); >+ >+ return NS_OK; >+ } >diff --git a/dom/ipc/TabParent.cpp b/dom/ipc/TabParent.cpp >+POfflineCacheUpdateParent* >+TabParent::AllocPOfflineCacheUpdate(const URI& aManifestURI, >+ const URI& aDocumentURI, >+ const nsCString& aClientID, >+ const PRBool& aPartial, >+ const PRBool& stickDocument) >+{ >+ nsRefPtr<OfflineCacheUpdateParent> update = new OfflineCacheUpdateParent(); >+ if (!update) >+ return nsnull; No need for the nullcheck. >+ nsresult rv = update->Schedule(aManifestURI, aDocumentURI, aClientID, >+ aPartial, stickDocument); >+ if (NS_FAILED(rv)) >+ return nsnull; >+ >+ POfflineCacheUpdateParent* result = update.get(); >+ update.forget(); >+ return result; return update.forget(); >diff --git a/dom/src/offline/nsDOMOfflineResourceList.cpp b/dom/src/offline/nsDOMOfflineResourceList.cpp > NS_IMETHODIMP > nsDOMOfflineResourceList::GetMozItems(nsIDOMDOMStringList **aItems) > { >+#if 0 // Currently unimplemented What should we do about this? If it's nonessential for this to work, OK. If it regresses non-e10s behavior (probably will, right? Who uses it?) then we should fix. Either way, followup bug? > NS_IMETHODIMP > nsDOMOfflineResourceList::MozHasItem(const nsAString& aURI, PRBool* aExists) > { >+#if 0 // Currently unimplemented Same here, and the other subsequent places. (Is this stuff the 'dynamic items' you were referring to?) > NS_IMETHODIMP > nsDOMOfflineResourceList::GetStatus(PRUint16 *aStatus) > { > nsresult rv = Init(); > > // Init may fail with INVALID_STATE_ERR if there is no manifest URI. > // The status attribute should not throw that exception, convert it > // to an UNCACHED. >- if (rv == NS_ERROR_DOM_INVALID_STATE_ERR || >- !nsContentUtils::OfflineAppAllowed(mDocumentURI)) { >+ // XXX TODO We currently don't have access to permissions from the content process We do now. :) > NS_IMETHODIMP > nsDOMOfflineResourceList::Update() > { > nsresult rv = Init(); > NS_ENSURE_SUCCESS(rv, rv); > >+ /* XXX TODO Cannot use permission manager on the content process Same. > NS_IMETHODIMP > nsDOMOfflineResourceList::SwapCache() > { > nsresult rv = Init(); > NS_ENSURE_SUCCESS(rv, rv); > >+ /* XXX TODO Cannot use permission manager on the content process Here too. I'll stop commenting on this now. :) > NS_IMETHODIMP >-nsDOMOfflineResourceList::Checking(nsIOfflineCacheUpdate *aUpdate) >+nsDOMOfflineResourceList::ApplicationCacheAvailable(nsIOfflineCacheUpdate *aUpdate, >+ nsIApplicationCache *aApplicationCache) > { >+ mAvailableApplicationCache = aApplicationCache; No need for the 'aUpdate' arg, right? > nsresult > nsDOMOfflineResourceList::CacheKeys() > { >+#if 0 What's this about? Different to the previous cases? >diff --git a/netwerk/base/public/nsIApplicationCache.idl b/netwerk/base/public/nsIApplicationCache.idl > * > * All application caches with the same group ID belong to a cache > * group. Each group has one "active" cache that will service future > * loads. Inactive caches will be removed from the cache when they are > * no longer referenced. > */ > [scriptable, uuid(663e2e2e-04a0-47b6-87b3-a122be46cb53)] Need to rev the uuid here. >diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h >+ PRUint8 mInheritApplicationCache : 1; >+ PRUint8 mChooseApplicationCache : 1; >+ PRUint8 mLoadedFromApplicationCache : 1; It looks like you left mChooseApplicationCache in nsHttpChannel (and you don't init it in the HttpBaseChannel constructor!), and it's not touched in HttpChannelChild, so you should pick which one you want. gcc should warn about member shadowing. :( >diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp >@@ -164,24 +168,78 @@ HttpChannelParent::RecvAsyncOpen(const I >+ nsCOMPtr<nsIApplicationCacheChannel> appCacheChan = >+ do_QueryInterface(mChannel); >+ nsCOMPtr<nsIApplicationCacheService> appCacheService = >+ do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID); >+ >+ // TODO Check the permission manager here!!!!!! Yes! :) >+ PRBool setChooseApplicationCache = chooseApplicationCache; >+ if ((appCacheChan) && (appCacheService)) { Extra parens. >+ // We might potentially want to drop this flag (that is TRUE by default) >+ // after we succefully associate the channel with an application cache >+ // reported by the channel child. Dropping it here may be too early. >+ appCacheChan->SetInheritApplicationCache(PR_FALSE); >+ if (!appCacheClientID.IsEmpty()) { >+ nsCOMPtr<nsIApplicationCache> appCache; >+ rv = appCacheService->GetApplicationCache(appCacheClientID, >+ getter_AddRefs(appCache)); >+ if (NS_SUCCEEDED(rv)) { >+ appCacheChan->SetApplicationCache(appCache); >+ setChooseApplicationCache = PR_FALSE; >+ } >+ } >+ >+ if (setChooseApplicationCache) { >+ nsCOMPtr<nsIOfflineCacheUpdateService> offlineUpdateService = >+ do_GetService("@mozilla.org/offlinecacheupdate-service;1", &rv); >+ if (NS_SUCCEEDED(rv)) { >+ rv = offlineUpdateService->OfflineAppAllowedForURI(uri, >+ nsnull, >+ &setChooseApplicationCache); >+ >+ // XXX FOR TESTING PURPOSES ONLY: >+ setChooseApplicationCache = PR_TRUE; Fix? >diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl >+ // This might have to be sync. If this fails we must fail the document load >+ // to avoid endless loop. >+ MarkOfflineCacheEntryAsForeign(); Hmm. Can you explain this? Under what conditions would it fail? >diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp >+NS_IMETHODIMP >+nsHttpChannel::MarkOfflineCacheEntryAsForeign() >+{ >+ if (!mApplicationCache) >+ return NS_ERROR_NOT_AVAILABLE; >+ >+ nsresult rv; >+ >+ nsCAutoString cacheKey; >+ rv = GenerateCacheKey(mPostID, cacheKey); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mApplicationCache->MarkEntry(cacheKey, >+ nsIApplicationCache::ITEM_FOREIGN); Bad indenting. >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return NS_OK; 'return rv' should be OK here. >diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/OfflineCacheUpdateChild.cpp >+void >+OfflineCacheUpdateChild::RefcountHitZero() > { >+ if (mIPCActivated) { >+ // ContentProcessChild::DeallocPOfflineCacheUpdate will delete this >+ OfflineCacheUpdateChild::Send__delete__(this); >+ } else { >+ delete this; // we never opened IPDL channel >+ } Bad indenting. > NS_IMETHODIMP >+OfflineCacheUpdateChild::Init(nsIURI *aManifestURI, >+ nsIURI *aDocumentURI) Same. Looks like a bunch of cases through this file. Make a pass over them? >+NS_IMETHODIMP >+OfflineCacheUpdateChild::InitPartial(nsIURI *aManifestURI, >+ const nsACString& clientID, >+ nsIURI *aDocumentURI) >+{ >+ // For now leaving this method, we may discover we need it. >+ return NS_ERROR_NOT_IMPLEMENTED; Add an NS_NOTREACHED here? >+NS_IMETHODIMP >+OfflineCacheUpdateChild::Schedule() >+{ >+ LOG(("OfflineCacheUpdateChild::Schedule [%p]", this)); > >+#ifdef MOZ_IPC >+ NS_ASSERTION(mWindow, "Window must be provided to the offline cache update child"); > #endif > >- // Observe xpcom-shutdown event >+ nsCOMPtr<nsPIDOMWindow> piWindow = >+ do_QueryInterface(mWindow); >+ mWindow = nsnull; >+ >+ nsIDocShell *docshell = piWindow->GetDocShell(); >+ >+ nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(docshell); >+ NS_ASSERTION(item, "doc shell tree item is null"); >+ if (!item) >+ return NS_ERROR_FAILURE; Think of assertions as fatal -- so we should either assert here, or warn & return failure, but not both. >+ // Need to reference itself here, because the IPC stack doesn't hold 'addref ourself'. >+ // a reference to us. Will be released in RecvFinish(). >+ child->SendPOfflineCacheUpdateConstructor(this, >+ IPC::URI(mManifestURI), >+ IPC::URI(mDocumentURI), >+ mClientID, >+ mPartialUpdate, >+ mDocument != nsnull /* Stick document */); Hmm, what does the /* comment */ mean? >+bool >+OfflineCacheUpdateChild::RecvFinish(const PRBool &succeeded, >+ const PRBool &isUpgrade) > { Were you going to Release() 'this' here? It looks like you do it in RecvNotifyStateEvent, in which case you should alter the comment above? >diff --git a/uriloader/prefetch/OfflineCacheUpdateGlue.cpp b/uriloader/prefetch/OfflineCacheUpdateGlue.cpp >+void >+OfflineCacheUpdateGlue::SetDocument(nsIDOMDocument *aDocument) >+{ >+ // The design is one document for one cache update on the content process. >+ NS_ASSERTION(!mDocument, "Seting more then a single document on a glue of offline cache update"); 'a glue of offline cache update' --> 'an instance of OfflineCacheUpdateGlue' >+NS_IMETHODIMP >+OfflineCacheUpdateGlue::ApplicationCacheAvailable(nsIOfflineCacheUpdate *aUpdate, >+ nsIApplicationCache *aApplicationCache) >+{ >+ NS_ENSURE_TRUE(aApplicationCache, NS_ERROR_NOT_AVAILABLE); Use NS_ENSURE_ARG(aApplicationCache) instead? Likewise elsewhere (e.g. in OfflineCacheUpdateParent.cpp). >diff --git a/uriloader/prefetch/OfflineCacheUpdateParent.cpp b/uriloader/prefetch/OfflineCacheUpdateParent.cpp >+OfflineCacheUpdateParent::OfflineCacheUpdateParent() >+ : mChildNotified(PR_FALSE) >+{ >+ // Make sure the service has been initialized >+ nsOfflineCacheUpdateService* service = >+ nsOfflineCacheUpdateService::EnsureService(); >+ if (!service) >+ return; >+ >+ LOG(("OfflineCacheUpdateParent::OfflineCacheUpdateParent [%p]", this)); >+} >+nsresult >+OfflineCacheUpdateParent::Schedule(const URI& aManifestURI, >+ const URI& aDocumentURI, >+ const nsCString& aClientID, >+ const PRBool& aPartial, >+ const PRBool& stickDocument) >+{ >+ // Make sure the service has been initialized >+ nsOfflineCacheUpdateService* service = >+ nsOfflineCacheUpdateService::EnsureService(); >+ if (!service) >+ return NS_ERROR_FAILURE; Hmm. Why do this in the constructor and here? Can probably kill off the constructor one, right? >+ >+ LOG(("OfflineCacheUpdateParent::RecvSchedule [%p]", this)); >+ >+ nsRefPtr<nsOfflineCacheUpdate> update; >+ nsCOMPtr<nsIURI> manifestURI(aManifestURI); >+ nsCOMPtr<nsIURI> documentURI(aDocumentURI); >+ >+ if (!aPartial) >+ service->FindUpdate(manifestURI, documentURI, getter_AddRefs(update)); >+ >+ if (!update) { >+ update = new nsOfflineCacheUpdate(); >+ if (!update) >+ return NS_ERROR_OUT_OF_MEMORY; No need for OOM tests. >+NS_IMETHODIMP >+OfflineCacheUpdateParent::UpdateStateChanged(nsIOfflineCacheUpdate *aUpdate, PRUint32 state) >+{ >+ LOG(("OfflineCacheUpdateParent::StateEvent [%p]", this)); >+ >+ if (!mChildNotified && >+ (state == nsIOfflineCacheUpdateObserver::STATE_FINISHED || >+ state == nsIOfflineCacheUpdateObserver::STATE_ERROR || >+ state == nsIOfflineCacheUpdateObserver::STATE_NOUPDATE || >+ state == nsIOfflineCacheUpdateObserver::STATE_OBSOLETE)) { >+ // Tell the child the particulars after the update has finished. >+ nsCString prevClientId; >+ nsCString currClientId; >+ >+ PRBool isUpgrade; >+ aUpdate->GetIsUpgrade(&isUpgrade); >+ PRBool succeeded; >+ aUpdate->GetSucceeded(&succeeded); >+ >+ SendFinish(succeeded, isUpgrade); >+ >+ // XXX No need for this when we remove... >+ mChildNotified = PR_TRUE; Not sure what this means? >+ >+ aUpdate->RemoveObserver(this); Do we need to unconditionally remove the observer? We unconditionally add it in Schedule()... >diff --git a/uriloader/prefetch/nsIOfflineCacheUpdate.idl b/uriloader/prefetch/nsIOfflineCacheUpdate.idl >+ * Informs the observer about an application being available to associate. >+ void applicationCacheAvailable(in nsIOfflineCacheUpdate aUpdate, >+ in nsIApplicationCache applicationCache); Add @param descriptions too? >diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdate.cpp >@@ -1452,70 +1450,72 @@ nsOfflineCacheUpdate::ManifestCheckCompl >- for (PRInt32 i = 0; i < mDocuments.Count(); i++) { >- newUpdate->AddDocument(mDocuments[i]); >+ // XXX Is this really needed? We need the list of all sticked documents >+ // only in case the manifest didn't change to cache all new implicit >+ // items. But here the manifest has changed for sure... >+ for (PRUint32 i = 0; i < mDocumentURIs.Count(); i++) { >+ newUpdate->StickDocument(mDocumentURIs[i]); Hmm. If it's not a problem, let's ditch it. >@@ -2077,65 +1909,21 @@ nsOfflineCacheUpdate::AddURI(nsIURI *aUR > NS_IMETHODIMP > nsOfflineCacheUpdate::AddDynamicURI(nsIURI *aURI) > { >+ return NS_ERROR_NOT_IMPLEMENTED; Does anyone use this? >diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdateService.cpp >-//----------------------------------------------------------------------------- > // nsOfflineCachePendingUpdate >+// E10S Fully a child code this... What does this mean? That it's only used in the child? I understand the concept of what's going on here, as you outlined in the previous comments, but I don't understand all the implications of the changes. What kind of tests do we have to cover this? I suspect we need more, and we certainly need them running and passing in the e10s case. In regard to comment 6, I'd like: * Explanations of what the #if 0'd code is (dynamic items?), and what we should do about it * Fixes for all the permission manager stuff * A bug filed on the missing notification after the doc has finished loading, if that's still an issue If you update the patch, please post an interdiff (just add a new patch in your mq?) so I can review the changes alone.

Thanks for this review. First, few reactions: > >- for (PRInt32 i = 0; i < mDocuments.Count(); i++) { > >- newUpdate->AddDocument(mDocuments[i]); > >+ // XXX Is this really needed? We need the list of all sticked documents > >+ // only in case the manifest didn't change to cache all new implicit > >+ // items. But here the manifest has changed for sure... > >+ for (PRUint32 i = 0; i < mDocumentURIs.Count(); i++) { > >+ newUpdate->StickDocument(mDocumentURIs[i]); > > Hmm. If it's not a problem, let's ditch it. > Thanks for poking at this. I realized an issue we have here. To explain the update process a bit: - we fetch the manifest (the list of things to cache) - we cache all the items - we check the manifest again - if it changed, we discard the just created cache group version and do the update again to preserve the cache update be atomic Carrying the real documents (objects) to the new update ensured, btw, that these documents got associated with the proper application cache group version after it is atomically fetched. But with this patch observers of the update do this association. But as you can see we do not carry the list of observers to the new update. We are not doing this now as well. So, I have to check how this exactly works now, w/o this patch, and probably fix it in a (blocking?) followup if too complicated. Just a note that this scenario happens quit rare, but on the other side, it may lead to a broken cached applications, that persist broken until a new manifest is populated. > What kind of tests do we have to cover this? I suspect we need more, and we > certainly need them running and passing in the e10s case. We have a tun of mochitests covering all of this. But it is not functioning under e10s AFAIK... We can have xpc-shell tests as the permission manager can be driven from the child process now. Yay! > * Explanations of what the #if 0'd code is (dynamic items?), and what we should > do about it Yes, dynamic items. It was previously in the spec but later removed. Now we support it only as a 'moz' extension. It needs a lot of sync operations to have it again. I believe we can postpone the work to make this function again to a non-blocking followup or remove the support completely as it is just a moz extension. I may post to WAHTWG forum about it to see a reaction.

(In reply to comment #17) > >+ POfflineCacheUpdateParent* result = update.get(); > >+ update.forget(); > >+ return result; > > return update.forget(); > Did you try to compile it?

I think he meant update.forget().get().

This is a WIP patch of update from the first review. It doesn't seem to produce any regressions on the single process build - the offline cache tests are locally passing. (I didn't push to try yet). Now I have problems in fennec with running the initial update after the offline permission is given to a page, simply nothing happens. I'll investigate this and if it is related only to the fennec chrome I'll open a new bug for it. (In reply to comment #17) > Comment on attachment 453875 [details] [diff] [review] > v1 - merged > The permission manager works in the child now, so you should be able to do > this. Uncommented, but not tested (see above). > >- appCacheChannel->SetChooseApplicationCache(ShouldCheckAppCache(newURI)); > >+ // Permission will be checked in the parent process. > >+ appCacheChannel->SetChooseApplicationCache(PR_TRUE); > > This can happen in the child now, but if you'd rather do it in the parent then > that's fine too. > It's cheaper to leave it this way. > >+ POfflineCacheUpdateParent* result = update.get(); > >+ update.forget(); > >+ return result; > > return update.forget(); > None of return update.forget(); or return update.forget().get(); worked. The template is unable to do an implicit typecast. I'll leave it in this form. > >diff --git a/dom/src/offline/nsDOMOfflineResourceList.cpp b/dom/src/offline/nsDOMOfflineResourceList.cpp > > > NS_IMETHODIMP > > nsDOMOfflineResourceList::GetMozItems(nsIDOMDOMStringList **aItems) > > { > >+#if 0 // Currently unimplemented > > What should we do about this? If it's nonessential for this to work, OK. If it > regresses non-e10s behavior (probably will, right? Who uses it?) then we should > fix. Either way, followup bug? > So, as I wrote already, I would re-introduce (if we want it) in a followup bug. Currently I leave the implementation intact for single process (w/o MOZ_IPC). > > nsresult > > nsDOMOfflineResourceList::CacheKeys() > > { > >+#if 0 > > What's this about? Different to the previous cases? Used only for dynamic items. > >diff --git a/netwerk/base/public/nsIApplicationCache.idl b/netwerk/base/public/nsIApplicationCache.idl > Need to rev the uuid here. Ups, thanks. > > >diff --git a/netwerk/protocol/http/HttpBaseChannel.h b/netwerk/protocol/http/HttpBaseChannel.h > > >+ PRUint8 mInheritApplicationCache : 1; > >+ PRUint8 mChooseApplicationCache : 1; > >+ PRUint8 mLoadedFromApplicationCache : 1; > > It looks like you left mChooseApplicationCache in nsHttpChannel (and you don't > init it in the HttpBaseChannel constructor!), and it's not touched in > HttpChannelChild, so you should pick which one you want. gcc should warn about > member shadowing. :( Has changed already, these changes were removed from my patch. > > >diff --git a/netwerk/protocol/http/HttpChannelParent.cpp b/netwerk/protocol/http/HttpChannelParent.cpp > > >@@ -164,24 +168,78 @@ HttpChannelParent::RecvAsyncOpen(const I > >+ // TODO Check the permission manager here!!!!!! > > Yes! :) Hmm.. the comment was a leftover. It is alrady done bellow through offlineUpdateService->OfflineAppAllowedForURI. > >diff --git a/netwerk/protocol/http/PHttpChannel.ipdl b/netwerk/protocol/http/PHttpChannel.ipdl > > >+ // This might have to be sync. If this fails we must fail the document load > >+ // to avoid endless loop. > >+ MarkOfflineCacheEntryAsForeign(); > > Hmm. Can you explain this? Under what conditions would it fail? Added a comment to IDL file. > >+ NS_ENSURE_SUCCESS(rv, rv); > >+ > >+ return NS_OK; > > 'return rv' should be OK here. > Please no. I really don't like it. And I need the log of a possible failure here. > >+ // Make sure the service has been initialized > >+ nsOfflineCacheUpdateService* service = > >+ nsOfflineCacheUpdateService::EnsureService(); > >+ if (!service) > >+ return NS_ERROR_FAILURE; > > Hmm. Why do this in the constructor and here? Can probably kill off the > constructor one, right? > Needed for logging and then the 'service' local is used. > >+ > >+ aUpdate->RemoveObserver(this); > > Do we need to unconditionally remove the observer? We unconditionally add it in > Schedule()... Right, we don't. > * A bug filed on the missing notification after the doc has finished loading, > if that's still an issue This apparently works.

Comment on attachment 484050 [details] [diff] [review] v2 wip1 Tagging bsmedberg for sr here. The story is: this patch contains major interface and behavior changes, and the interface changes cannot be avoided given the rearchitecting. See comment 5, comment 6, and on. As I understand it, per stuart and beltzner, we're considering this a feature. Which means it's past freeze already. I don't know if we're making an exception for this, but I for one could use clarification.

(It's a huge patch, you probably just want to look at the .idl changes and the explanations in this bug about the behavior changes.)

CC'ing stuart and beltzner; please see comment 23.

Beta 7 hasn't finished yet, so there's still time if: - we know that this patch is safe (passes tests, doesn't regress perf) - we know that this patch is ready to land in the next 24 hours

(In reply to comment #26) > Beta 7 hasn't finished yet, so there's still time if: > > - we know that this patch is safe (passes tests, doesn't regress perf) Going to push this to the try server. So far, local test shows no failures, not sure of a perf regressions, but should be none. > - we know that this patch is ready to land in the next 24 hours The patch as is doesn't break anything on the single process Firefox 4 (Gecko 2.0) build. However, it needs manual testing in the Fennec multiprocess build. And there are no automated tests (mochitests still doesn't work on Fennec).

Comment on attachment 484052 [details] [diff] [review] v1 -> v2 wip1 idiff >diff --git a/dom/src/offline/nsDOMOfflineResourceList.cpp b/dom/src/offline/nsDOMOfflineResourceList.cpp >@@ -209,17 +234,17 @@ nsDOMOfflineResourceList::Disconnect() > NS_IMETHODIMP > nsDOMOfflineResourceList::GetMozItems(nsIDOMDOMStringList **aItems) > { >-#if 0 // Currently unimplemented >+#if !defined(MOZ_IPC) // Currently unimplemented for multi-process Gecko for multi-process Gecko Can you put #else clauses wrapping the 'return NS_ERROR_NOT_IMPLEMENTED;' statements? Otherwise your consecutive return statements look a little... odd. Same for other instances. >diff --git a/uriloader/prefetch/OfflineCacheUpdateChild.cpp b/uriloader/prefetch/OfflineCacheUpdateChild.cpp >@@ -397,49 +380,60 @@ OfflineCacheUpdateChild::Schedule() > nsCOMPtr<nsITabChild> tabchild = do_GetInterface(owner); >- if (!tabchild) >+ if (!tabchild) { >+ NS_WARNING("doc shell tree owner is null"); "tabchild is null"? >+ // mDocument is non-null if both: >+ // 1. this update was initiated by a document that referred a manifest >+ // 2. the document has not already been loaded from the application cache >+ // This tells the update to cache this document even in case the manifest >+ // has not been changed since the last fetch. >+ // See also nsOfflineCacheUpdate::ScheduleImplicit. >+ PRBool stickDocument = mDocument != nsnull; Can just use 'bool' here (and in the .ipdl declaration), which I'd slightly prefer. >+bool >+OfflineCacheUpdateChild::RecvNotifyStateEvent(const PRUint32 &event) >+{ >+ LOG(("OfflineCacheUpdateChild::RecvNotifyStateEvent [%p]", this)); >+ >+ // Convert the public observer state to our internal state >+ switch (event) { >+ case nsIOfflineCacheUpdateObserver::STATE_CHECKING: >+ mState = STATE_CHECKING; >+ break; >+ >+ case nsIOfflineCacheUpdateObserver::STATE_DOWNLOADING: >+ mState = STATE_DOWNLOADING; >+ break; >+ } Need a 'default' clause here. (gcc will warn if you don't specify cases that cover each value in an enumeral.) >diff --git a/uriloader/prefetch/OfflineCacheUpdateGlue.h b/uriloader/prefetch/OfflineCacheUpdateGlue.h >+// Like FORWARD_SAFE but methods: >+// Schedule >+// Init >+#define NS_ADJUSTED_FORWARD_NSIOFFLINECACHEUPDATE(_to) \ I think the comment's missing an 'except'. >diff --git a/uriloader/prefetch/nsOfflineCacheUpdate.cpp b/uriloader/prefetch/nsOfflineCacheUpdate.cpp > nsresult >-nsOfflineCacheUpdate::Finish() >+nsOfflineCacheUpdate::FinishNoNotify() > { > LOG(("nsOfflineCacheUpdate::Finish [%p]", this)); > > mState = STATE_FINISHED; > > if (!mPartialUpdate) { > if (mSucceeded) { > nsIArray *namespaces = mManifestItem->GetNamespaces(); > nsresult rv = mApplicationCache->AddNamespaces(namespaces); > if (NS_FAILED(rv)) { >- NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); >+ NotifyState(this, nsIOfflineCacheUpdateObserver::STATE_ERROR); > mSucceeded = PR_FALSE; > } > > rv = mApplicationCache->Activate(); > if (NS_FAILED(rv)) { >- NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR); >+ NotifyState(this, nsIOfflineCacheUpdateObserver::STATE_ERROR); > mSucceeded = PR_FALSE; > } So if AddNamespaces() fails, we notify and then continue to Activate(), and possibly report another error? Is that intended? r=dwitte.

(In reply to comment #27) > The patch as is doesn't break anything on the single process Firefox 4 (Gecko > 2.0) build. However, it needs manual testing in the Fennec multiprocess build. > And there are no automated tests (mochitests still doesn't work on Fennec). Do we have mochitests that cover (sufficiently!) the functionality for Firefox? If that's the case, then I think we're OK. The risk would be that we discover problems for Fennec that require further changes to API or behavior that will affect Firefox. I think we need as much testing as possible on Fennec before we ship beta2. Can we make that known (stuart?) so we make sure it happens? Talked to bsmedberg about superreview; he indicated he can get to it but he's very much unfamiliar with this code. According to hg, the only people who have worked or reviewed this code previously are * Honza * dcamp * biesi * bz * jst I'm not sure how familiar biesi and jst would consider themselves. I'd pick bz but he's probably insanely busy. In any case, cc'ing folk to see what they think.

(He also indicated he'd deputize someone to do the interface review if they're familiar enough with the code. Anyone?)

(In reply to comment #28) > >-#if 0 // Currently unimplemented > >+#if !defined(MOZ_IPC) // Currently unimplemented for multi-process Gecko for multi-process Gecko > > Can you put #else clauses ... > Yes, for sure. I forgot to do that before remerging to the final patch. > So if AddNamespaces() fails, we notify and then continue to Activate(), and > possibly report another error? Is that intended? This might be a different bug. I'll check this and file it if needed. > > r=dwitte. I'll address the rest. Are you aware, that this patch is absolutely untested under Fennec? The first version worked, but this update changed a lot of things and I need to test at least manually again. But thanks some issue I cannot start the update process (I want to take a look at that right now).

(In reply to comment #29) > Do we have mochitests that cover (sufficiently!) the functionality for Firefox? We do, the patch was pushed to the try server (3cd715fa1715), locally the tests are passing.

test_offlineNotification.html seems to be broken. I'll check that ASAP tomorrow.

I think that if there's any way to avoid reviewing a 300KB megapatch, I will. :(

Comment on attachment 484050 [details] [diff] [review] v2 wip1 biesi said he could look at this. (See comment 23.) Thanks, btw. :)

Comment on attachment 484050 [details] [diff] [review] v2 wip1 I'll have an update soon. - first, I was building platform for Firefox w/o IPC (w/o MOZ_IPC defined) - so I need to update it to work with MOZ_IPC and on the chrome process only - all test failures found on the try server are fixed (the second try run was broken because of the MOZ_IPC misuse) - done some updates to make it work also for Fennec with chrome necko stack

Updated, now pushing to try. - fixed failure of test_offlineNotification.html, missing third parameter at call to nsIOfflineCacheUpdateService.scheduleUpdate - fixed failure of test_bug413310.html, we were always choosing an offline cache, for a particular URI that test is loading we were failing nsHttpChannel::AsyncOpen (failed to create nsIURI instance by nsDiskCacheDeviceSQL) - most of #ifdef MOZ_IPC changed to test of the process type ( != default ) - nsOfflineCacheUpdate::NotifyState always notify 'this' as an update (notifying the nested update caused leak - could not remove observer) - fennec: SendAssociateApplicationCache moved from HttpChannelParent::AsyncOpen to parent HttpChannelParent::OnStartRequest, needed as we open cache entry asynchronously

Probably should get another review, though only small changes, outlined in the previous comment.

So far, so green. Also it seems that Fennec works well with this patch (and some update to its chrome, in another patch).

- moves schedule of an update after a page is allowed to store data for offline to the content process - w/o this patch (just adding null as a 3rd arg to scheduleUpdate) would work as well, but the content window won't get state change notifications and the cache associated, what is against the spec Mark, BTW, thanks for you advises on IRC today.

Comment on attachment 484505 [details] [diff] [review] v1 (mobile) [Check in comment 60] Looks OK to me. We'd probably rename the new message to "Browser:MozApplicationCache:Fetch", but otherwise fine.

Comment on attachment 484435 [details] [diff] [review] v3 sr=biesi on the interface changes +++ b/netwerk/protocol/http/nsHttpChannel.cpp + rv = mApplicationCache->MarkEntry(cacheKey, + nsIApplicationCache::ITEM_FOREIGN); wrong indentation

Comment on attachment 484437 [details] [diff] [review] v2 -> v3 idiff >diff --git a/dom/src/offline/nsDOMOfflineResourceList.cpp b/dom/src/offline/nsDOMOfflineResourceList.cpp >@@ -168,38 +172,41 @@ nsDOMOfflineResourceList::Init() > #if !defined(MOZ_IPC) >- mApplicationCacheService = >- do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID, &rv); >- NS_ENSURE_SUCCESS(rv, rv); >- >- // Check for in-progress cache updates >- nsCOMPtr<nsIOfflineCacheUpdateService> cacheUpdateService = >- do_GetService(NS_OFFLINECACHEUPDATESERVICE_CONTRACTID, &rv); >- NS_ENSURE_SUCCESS(rv, rv); >- >- PRUint32 numUpdates; >- rv = cacheUpdateService->GetNumUpdates(&numUpdates); >- NS_ENSURE_SUCCESS(rv, rv); >- >- for (PRUint32 i = 0; i < numUpdates; i++) { >- nsCOMPtr<nsIOfflineCacheUpdate> cacheUpdate; >- rv = cacheUpdateService->GetUpdate(i, getter_AddRefs(cacheUpdate)); >+ if (GeckoProcessType_Default == XRE_GetProcessType()) >+#endif This looks backwards. You want the process type check to occur when MOZ_IPC is defined, right? > NS_IMETHODIMP > nsDOMOfflineResourceList::GetMozItems(nsIDOMDOMStringList **aItems) > { >-#if !defined(MOZ_IPC) // Currently unimplemented for multi-process Gecko for multi-process Gecko >+#if !defined(MOZ_IPC) >+ if (GeckoProcessType_Default != XRE_GetProcessType()) >+ return NS_ERROR_NOT_IMPLEMENTED; >+#endif Same here, and other places. FWIW, I think it would make things more readable if you defined a little static function: bool IsChildProcess() { return GeckoProcessType_Default != XRE_GetProcessType(); } and then used that around the place. r=dwitte with that. Will also need another push to try since the above are some fairly significant changes. ;)

If this goes through try clean, it can also land on the beta7 relbranch; please mark the target milestone appropriately.

(In reply to comment #43) > This looks backwards. You want the process type check to occur when MOZ_IPC is > defined, right? Working late and tired is not healthy... Updated to the last comments. Looks green on the try server.

Comment on attachment 484672 [details] [diff] [review] v4 (mozilla-central) [Check in m-c comment 52] [Check in b7 branch comment 57] Try server is green.

Comment on attachment 484672 [details] [diff] [review] v4 (mozilla-central) [Check in m-c comment 52] [Check in b7 branch comment 57] No need for approval; you've got 2.0b2+ for landing on m-c and approval for the b7 relbranch from comment 44. I'd land on m-c first, make sure we're good, then push to relbranch today. Good to see this landing :)

Dumb question: why would we land this on the FF4 beta7 branch, when this is e10s-specific code that I'd have expected is irrelevant for FF4? Do out-of-process plugins use this somehow?

(In reply to comment #48) > Dumb question: why would we land this on the FF4 beta7 branch, when this is > e10s-specific code that I'd have expected is irrelevant for FF4? Do > out-of-process plugins use this somehow? Changes APIs, AIUI.

This seems to have busted all platforms on comm-central. E.g. vor OS X (same for Linux, similar for Win): /builds/slave/macosx-comm-central-bloat/build/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp:1948: error: 'GeckoProcessType_Default' was not declared in this scope /builds/slave/macosx-comm-central-bloat/build/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp:1948: error: 'XRE_GetProcessType' was not declared in this scope /builds/slave/macosx-comm-central-bloat/build/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp: At global scope: /builds/slave/macosx-comm-central-bloat/build/mozilla/uriloader/prefetch/nsOfflineCacheUpdate.cpp:75: warning: 'gOfflineCacheUpdateService' defined but not used NEXT ERROR make[6]: *** [nsOfflineCacheUpdate.o] Error 1 make[6]: *** Waiting for unfinished jobs....

(In reply to comment #50) > This seems to have busted all platforms on comm-central Just fixing it.

Comment on attachment 484672 [details] [diff] [review] v4 (mozilla-central) [Check in m-c comment 52] [Check in b7 branch comment 57] http://hg.mozilla.org/mozilla-central/rev/c73c0da830fe

Bustage fix: http://hg.mozilla.org/mozilla-central/rev/4fce6753afea

Bustage fix 2: http://hg.mozilla.org/mozilla-central/rev/f134b9c36a4a

This bug's checkin removed the last usage of "gOfflineCacheUpdateService" in nsOfflineCacheUpdate.cpp, but left the variable-declaration. Consequently, GCC spams a "unused variable" warning for that line now. > uriloader/prefetch/nsOfflineCacheUpdate.cpp: At global scope: > uriloader/prefetch/nsOfflineCacheUpdate.cpp:75: warning: 'gOfflineCacheUpdateService' defined but not used http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287595046.1287602100.13270.gz Mind removing that variable entirely, as a one-liner followup? (rs=me)

(In reply to comment #55) > Mind removing that variable entirely, as a one-liner followup? (rs=me) After the tree turns to green again I can do that. cl.exe is quit benevolent, no warnings about that...

Comment on attachment 484672 [details] [diff] [review] v4 (mozilla-central) [Check in m-c comment 52] [Check in b7 branch comment 57] http://hg.mozilla.org/mozilla-central/rev/12f2f0fcb484

Anything else to do here, or is this FIXED?

Looks like the mobile-browser patch needs to land.

pushed: http://hg.mozilla.org/mobile-browser/rev/5444d0d530ff

Still getting warning-spam from the unused variable mentioned in Comment 55 / 56 -- honza, mind nixing that as a trivial followup push?

(In reply to comment #62) > Still getting warning-spam from the unused variable mentioned in Comment 55 / > 56 -- honza, mind nixing that as a trivial followup push? There is bug 607012 on that.