Closed Bug 753990 Opened 12 years ago Closed 12 years ago

Allow appcache to work with a custom cache (profile) folder within a single application

Categories

(Core :: Networking: Cache, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 1 obsolete file)

Open Web Apps support needs to let the main browser process do preload of OWA cache content.

We need to enhance nsApplicationCacheService API to create application caches with custom cache folder (not the default running application profile directory).

Technically: 
- add nsIApplicationCacheService.createCustomApplicationCache(manifestURL, directoryPath, quota)
- let nsCacheService keep a hash of nsOfflineCacheDevice's bound each to a custom directory
- let nsApplicationCache class expose the custom cache dir and nsApplicationCacheService class use that exposed dir to map the correct device
No longer blocks: 752705
I don't think this is the approach we decided to try... OWA each have their own profile, so we were going to try to launch a subprocess and have that download into the new profile. Right?
Just make up your mind :)  Both ways are doable.
Long term we'll need support for remotely triggering an app update (i.e. app cache update) whether the app itself is running or not. Seems if we go down the road of writing to the apps cache directory from Firefox (or some other process that is not the app itself) then we'll need to solve this problem again for the case where the app itself is actually running (and deal with race conditions between the app itself and whatever process triggers updates etc).

So, it seems to me that building a system that uses the app process itself for doing the app cache populating/updating is a more future proof approach.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #3)
> Long term we'll need support for remotely triggering an app update (i.e. app
> cache update) whether the app itself is running or not.
That is a requirement I haven't been aware of.

> So, it seems to me that building a system that uses the app process itself
> for doing the app cache populating/updating is a more future proof approach.
Sure, but we need proper IPC for that if we want the progress updates.
Right now our IPC code can't handle that in a sane way.

I thought bug 752705 was somewhat urgent to get the app installation work better.
For that downloading the appcache data to the app profile directory even before the
app itself has been installed should work well, and should be reasonable easy thing to implement.
(In reply to Olli Pettay [:smaug] from comment #4)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #3)
> > Long term we'll need support for remotely triggering an app update (i.e. app
> > cache update) whether the app itself is running or not.
> That is a requirement I haven't been aware of.

Sorry for not communicating that :(

> > So, it seems to me that building a system that uses the app process itself
> > for doing the app cache populating/updating is a more future proof approach.
> Sure, but we need proper IPC for that if we want the progress updates.
> Right now our IPC code can't handle that in a sane way.
> 
> I thought bug 752705 was somewhat urgent to get the app installation work
> better.
> For that downloading the appcache data to the app profile directory even
> before the
> app itself has been installed should work well, and should be reasonable
> easy thing to implement.

Yes, if it's significantly easier to do this in the Firefox process then I don't have a problem doing that to get the ball rolling here. But we should keep in mind that we'll eventually need a way to trigger updates from a different process as well.
Is there a requirement to get progress updates to the process which triggers the update?
If not, the code to do the update should be there already, I think. Just some small script to
call the right things should be needed; start the webapprt with some parameter which ends up
calling that update script.
If OOP progress updates are needed, then we need IPC in some form, and I wish we don't add
any hacks but use proper IPDL.
I'm not entirely following what is being propose here.

What has been definitely been decided on for desktop:

* Each application will run as a separate XULRunner process
* Each such process will have its own profile directory
* The profile directory used by a app-xulrunner-process is created when the
  application is first run.

Additionally, here are a couple of my guesses:

* We will eventually move to a solution where applications are always kept
  up-to-date. Even if the isn't running the app *or* firefox. However this is a
  long-term goal and not something we need to worry about too much about right now.
* We want it to be possible to keep running webapps even if you uninstall firefox.


So I think we could certainly allow the main firefox process take care of downloading the resources for a application. However if we put the downloaded resources in the firefox profile, that means that:

* The xulrunner processes will have to be able to read those files, as well as
  update them.
* If the user uninstalls firefox, the files will have to remain.
* When the user uninstalls the app, the files should be deleted.

If we download the files into a special folder outside of the firefox profile directories, that might would work, but we'd have to solve the exact same issues enumerated above.


If we can make the firefox process download the files into the xulrunner profile directory, that would be awesome, but I'm not sure that this is possible given that the directory isn't created until at first launch.


Honza: Which of these solutions were you proposing?
(In reply to Jonas Sicking (:sicking) from comment #7)
> However if we put the
> downloaded resources in the firefox profile,
No one has proposed that.


> If we can make the firefox process download the files into the xulrunner
> profile directory, that would be awesome, but I'm not sure that this is
> possible given that the directory isn't created until at first launch.
Which is why I'm going to change profileservice a bit so that we can
create the profile directory during the installation
Cool, so if we want to download directly into the xulrunner profile then we'll have the following requirements:

A) We'll have to write not just the resources, but also the meta-data into the
   xulrunner profile such that the xulrunner process can pick them up.
B) We have to prevent file corruption if a user tries to start the application while
   it is still being downloaded by the firefox process.
C) We shouldn't affect how the non-webapp appcache for the domain works. I.e. if we
   have a "normal" appcache for facebook.com in the firefox profile, and the user
   downloads an app from facebook.com, this should not affect the appcache for
   facebook in the user's firefox profile.


B might be as simple as not creating the icon on the desktop until the profile directory is created and we're done downloading and creating the resources and  metadata files.

Does all of this sound doable? If so I think it's a *great* solution.
(In reply to Jonas Sicking (:sicking) from comment #10)
> B) We have to prevent file corruption if a user tries to start the
> application while
>    it is still being downloaded by the firefox process.
The application isn't there before the data has been loaded.
(In reply to Jonas Sicking (:sicking) from comment #10)
> Cool, so if we want to download directly into the xulrunner profile then
> we'll have the following requirements:
> 
> A) We'll have to write not just the resources, but also the meta-data into
> the
>    xulrunner profile such that the xulrunner process can pick them up.

That would be automatic when ApplicationCacheService would be used for the update process.

> B) We have to prevent file corruption if a user tries to start the
> application while
>    it is still being downloaded by the firefox process.

Good point.

> C) We shouldn't affect how the non-webapp appcache for the domain works.
> I.e. if we
>    have a "normal" appcache for facebook.com in the firefox profile, and the
> user
>    downloads an app from facebook.com, this should not affect the appcache
> for
>    facebook in the user's firefox profile.

That is also automatically covered by this bug.

> 
> 
> B might be as simple as not creating the icon on the desktop until the
> profile directory is created and we're done downloading and creating the
> resources and  metadata files.

It could potentially be a bad user experience to not let them start the app before it's fully downloaded, but it is question if that would even be possible to run it before it gets cached...

> 
> Does all of this sound doable? If so I think it's a *great* solution.
It's totally ok that you can't start an application until it's been downloaded. That's how Window, OSX, Linux, iOS and Android all work.

There was discussion about allowing users to start applications before they were downloaded by simply using resources off the website until the appcached resources are available. However that can lead to a very bad first impression for users since the application could be significantly slower. It could also lead to applications not working properly since they are exposed to timing issues when the application has significantly different performance characteristics.

So we should not worry about that for now.

I'm very excited if we can make this work by the way. This is a great solution!
(In reply to Jonas Sicking (:sicking) from comment #13)
> There was discussion about allowing users to start applications before they
> were downloaded by simply using resources off the website until the
> appcached resources are available. 

I'm strongly against:
- app can load only from app cache or only from web, not both ; and allowing web only load would complicate privileges
- we would break the appcache atomicity when resources are updated on the server just when we download them (appcache update process takes care of this)
So, thanks for clearing and settling the decision here.  Going to start to work on this now!
Attached patch v1 (obsolete) — Splinter Review
- exposes a new method nsIOfflineCacheUpdate nsIOfflineCacheUpdateService.scheduleCustomUpdate(manifest,document,profileDirectory)

- the directory path is set on nsICachingChannel (a new attribute) by nsOfflineCacheUpdate
- then carried from nsHttpChannel to nsICacheSession (a new attribute)
- then from nsCacheSession to nsCacheRequest (since the request is not referring the session...)
- then in nsCacheService::ProcessRequest from nsCacheRequest to nsCacheEntry.mPrefferedDevice (already as an instance of nsCacheDevice)
- then in nsCacheService::EnsureEntryHasDevice nsCacheEntry.mPrefferedDevice is used to create binding and is set on nsCacheEntry directly (cannot just set the device in nsCacheService::ProcessRequest since binding wouldn't be processed)

- there is a hash table "directory path"->nsOfflineCacheDevice* at nsCacheService of all 'custom' offline cache devices including code to shut them down and release
- setting a custom directory on nsICacheSession is supported only for STORE_OFFLINE policy ; for any other policy the cache entry cannot be opened in a custom directory
Attachment #625186 - Flags: review?(michal.novotny)
Comment on attachment 625186 [details] [diff] [review]
v1

The patch is based on patches from bug 744710 and bug 744719 (in this order).

However, technically it doesn't depend on them (can be rebased to land w/o then).

https://tbpl.mozilla.org/?tree=Try&rev=21082512ae1f
Comment on attachment 625186 [details] [diff] [review]
v1

>      /**
> +     * Schedule a cache update for a given offline manifest and let the data
> +     * be stored to a custom profile directory.  There is no coalescing of
> +     * manifests by manifest URL.
> +     */
> +    nsIOfflineCacheUpdate scheduleCustomUpdate(in nsIURI aManifestURI,
> +                                               in nsIURI aDocumentURI,
> +                                               in nsILocalFile aProfileDir);

I think that ScheduleCustomProfileUpdate would be a better name, since it schedules an update of cache in a custom profile and not a custom update.


>      CondVar                    mCondVar;
> +    nsCOMPtr<nsILocalFile>     mCacheDir;
>  };

This is a profile directory, not a cache directory.


>      /**
> +     * When set, entries created with this session will be placed to a cache
> +     * based at this directory.  Use when storing entries to a different
> +     * profile then the active profile of the the current running application
> +     * process.
> +     */
> +    attribute nsILocalFile cacheDirectory;

The same as above, this is a profile directory. Also there is a typo in the comment, it should be "different profile than". The same typo is in nsIApplicationCache.idl.


> +NS_IMETHODIMP nsCacheSession::SetCacheDirectory(nsILocalFile *cacheDir)
> +{
> +    mCacheDir = cacheDir;
> +    return NS_OK;
> +}

We could move the check of the storage policy from nsCacheService::OpenCacheEntry() here. It makes more sense to me to fail at this place.


> +    void            SetPrefferedCacheDevice( nsCacheDevice * device )
> +                                                             { mPrefferedDevice = device; }
> +    nsCacheDevice * PrefferedCacheDevice()                   { return mPrefferedDevice; }

s/Preffered/Preferred/

But in fact, this isn't a preferred device since we don't allow to store the entry to any other device. The name should be something like mPredefinedDevice or mCustomDevice...


> + * 3. checks presence of index.sql and files in the expcted location

s/expcted/expected/


> +  if (pm.testPermission(uri, "offline-app") != 0) {
> +    dump("Previous test failed to clear offline-app permission!  Expect failures.\n");
> +  }

If possible clear the permission here, otherwise fail.


> +  dump(ps.getBoolPref("browser.cache.offline.enable"));

Tests shouldn't dump anything to console.
Attachment #625186 - Flags: review?(michal.novotny) → review-
Honza, so in the new API when installing webapp
one could call nsIOfflineCacheUpdateService.scheduleCustomUpdate(URLtoManifes, documentURL, file-to-the-profile-local-directory) ?
(scheduleCustomUpdate will be perhaps renamed)
How does one detect updates to that particular offlinecache?
nsIOfflineCacheUpdateObserver doesn't seem to get offlinecache object as a parameter.
(In reply to Olli Pettay [:smaug] from comment #19)
> Honza, so in the new API when installing webapp

See the test in the patch [1].  You can add an observer on the updated that is result of call to schedule*Update().

[1] https://hg.mozilla.org/try/file/21082512ae1f/netwerk/test/unit/test_offlinecache_custom-directory.js#l113
Ah, right. Thanks.
Since appcache is a permission-based feature, will this also set the permission on the target profile so that the webpage can actually use the appcache that has been populated?  Or does the permission only matters during population time, and then the network service always try to access what is in the appcache?
(In reply to Felipe Gomes (:felipe) from comment #22)
> Since appcache is a permission-based feature, will this also set the
> permission on the target profile so that the webpage can actually use the
> appcache that has been populated?

Enabling the webapp to use the appcache is bug 749029.
(In reply to Jonas Sicking (:sicking) from comment #24)
> What is the ETA here?

End of this week for next patch version.
(In reply to Michal Novotny (:michal) from comment #18)
> I think that ScheduleCustomProfileUpdate would be a better name, since it

Agree.

> entry to any other device. The name should be something like
> mPredefinedDevice or mCustomDevice...

I like mCustomDevice.

> > +  if (pm.testPermission(uri, "offline-app") != 0) {
> > +    dump("Previous test failed to clear offline-app permission!  Expect failures.\n");
> > +  }
> 
> If possible clear the permission here, otherwise fail.

No.  I set the perm on the next line.  This is here as an alert to help explain potential errors.

> 
> 
> > +  dump(ps.getBoolPref("browser.cache.offline.enable"));
> 
> Tests shouldn't dump anything to console.

They do and sometimes it's useful.  But I'll remove this one.
Attached patch v2Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=c462a950976a

- changed cacheDirectory to profileDirectory
- fixed leak
- fixed usage of GetTarget (using GetPath instead)
- fixed carry of the profile dir after redirect during offline cache update
Attachment #625186 - Attachment is obsolete: true
Attachment #627755 - Flags: review?(michal.novotny)
Could you clarify which profile dir the API expects? The main one or the local. 
IIRC appCache is usually stored in the local one.
(In reply to Olli Pettay [:smaug] from comment #28)
> Could you clarify which profile dir the API expects? The main one or the
> local. 
> IIRC appCache is usually stored in the local one.

This API expects the "local" profile part.  I will reflect this in the cache service idl before landing.
Comment on attachment 627755 [details] [diff] [review]
v2

> +            rv = GetCustomOfflineDevice(request->mProfileDir, -1,
> +            							getter_AddRefs(customCacheDevice));

Wrong indentation.


> +     * If set, this offline cache is placed in a different directory
> +     * then the current application profile.

You forgot to fix this typo, s/then/than/


> + * 3. checks presence of index.sql and files in the expcted location

And this one too, s/expcted/expected/
Attachment #627755 - Flags: review?(michal.novotny) → review+
Thanks, I'll land this in one or two days, I want to have a window when some issue arise with the parallel download.
Honza: I have a few questions on how to properly use this API:

- why do I need to pass both the doc uri and the manifest uri?

- I thought it was going to be part of the API to find the manifest uri. Am I supposed to manually find it before calling the API? How should I do it, load the doc url, parse it and look for the html tag?

- After a successful run I verified that the files were created inside the local profile folder, under OfflineCache. Then I tried to delete that and it said that index.sqlite couldn't be deleted because it was open in Firefox. bug?

- In your test I see that you set a number of prefs and permissions. Am I supposed to set those too during a webapp installation?
(In reply to Felipe Gomes (:felipe) from comment #32)
> - I thought it was going to be part of the API to find the manifest uri.
That was never the plan, AFAIK

> Am
> I supposed to manually find it before calling the API? How should I do it,
> load the doc url, parse it and look for the html tag?
load the doc url using XHR and check the manifest attribute of the html root element?
(In reply to Felipe Gomes (:felipe) from comment #32)
> Honza: I have a few questions on how to properly use this API:
> 
> - why do I need to pass both the doc uri and the manifest uri?

doc uri since the page that refers the manifest has, by the spec, to be cached as well, while it doesn't need to be explicitly listed in the manifest it self, and usually is not.

manifest uri from obvious reasons.

> 
> - I thought it was going to be part of the API to find the manifest uri. Am
> I supposed to manually find it before calling the API? How should I do it,
> load the doc url, parse it and look for the html tag?

This is the app cache level API, not the web apps level API, so this API is designed to take only the final information about the offline cache identifying attributes.  It has never been even thought to let offline cache service search for the manifest attribute it self and never will be.  

> 
> - After a successful run I verified that the files were created inside the
> local profile folder, under OfflineCache. Then I tried to delete that and it
> said that index.sqlite couldn't be deleted because it was open in Firefox.
> bug?

No, it's expected.  The file is held by Firefox until shutdown, it's the same when you try to delete the index.sqlite file in your Firefox profile.  

If there is a need to be able to access the index.sqlite file in "external" or "custom" profiles sooner, we can have a separate bug for it.  It probably will be, since the app can be run before Firefox is closed.

I'll report a followup on that.  Good point!

> 
> - In your test I see that you set a number of prefs and permissions. Am I
> supposed to set those too during a webapp installation?

You have to set "offline-app" permission for the web app's domain in the web app's profile to let it load from the offline cache.  But I believe in bug 749029 this is being taken care of.
Blocks: 760067
(In reply to Honza Bambas (:mayhemer) from comment #34)
> doc uri since the page that refers the manifest has, by the spec, to be
> cached as well, while it doesn't need to be explicitly listed in the
> manifest it self, and usually is not.

Ok. Some webapps specify its launch URL but that when loaded that address has a redirect to a different one. What should be the doc uri in this case, the original launch path or the final one? Or maybe it doesn't matter?

>> - I thought it was going to be part of the API to find the manifest uri.
>That was never the plan, AFAIK

Ok no problem, I was just confused.

FWIW I was testing with one of the webapps from the marketplace (Lanyrd mobile) that has appcache support, and an automatic detection on install for that app will be likely impossible. The <html manifest=""> tag is not in the top-level document, but inside an iframe in the page. And to add to the fun that iframe is dynamically created and inserted by JS after they detect the browser has offline support. 
 
> If there is a need to be able to access the index.sqlite file in "external"
> or "custom" profiles sooner, we can have a separate bug for it.  It probably
> will be, since the app can be run before Firefox is closed.
> 
> I'll report a followup on that.  Good point!

Indeed, the app might be open before Firefox closes and if it tries to update its appcache then it would fail to write to the database. Thanks for filing the followup.

> You have to set "offline-app" permission for the web app's domain in the web
> app's profile to let it load from the offline cache.  But I believe in bug
> 749029 this is being taken care of.

Yeah I knew about that one, I was thinking about browser.cache.offline.enable and browser.cache.offline.parent_directory. Should we make sure these prefs are true in the webapp runtime? Or is true their default values?
(In reply to Felipe Gomes (:felipe) from comment #35)
> Ok. Some webapps specify its launch URL but that when loaded that address
> has a redirect to a different one. What should be the doc uri in this case,
> the original launch path or the final one? Or maybe it doesn't matter?

We don't allow redirects during cache update, so it has to be the final URL.

However, it's simple to make docuri optional and force web developers to add the main page to the manifest as an explicit resource.

> FWIW I was testing with one of the webapps from the marketplace (Lanyrd
> mobile) that has appcache support, and an automatic detection on install for
> that app will be likely impossible. The <html manifest=""> tag is not in the
> top-level document, but inside an iframe in the page. And to add to the fun
> that iframe is dynamically created and inserted by JS after they detect the
> browser has offline support. 

We may either:
- don't support preload for such apps
- let apps specific explicitly in the web app manifest where the app cache manifest is ; I believe that could be very helpful not just for this bug

> Yeah I knew about that one, I was thinking about
> browser.cache.offline.enable and browser.cache.offline.parent_directory.
> Should we make sure these prefs are true in the webapp runtime? Or is true
> their default values?

browser.cache.offline.enable is true by def.
browser.cache.offline.parent_directory is NULL (not defined) by default and I cannot find any place we set it to any value in mxr
(In reply to Honza Bambas (:mayhemer) from comment #36)
> (In reply to Felipe Gomes (:felipe) from comment #35)
> > FWIW I was testing with one of the webapps from the marketplace (Lanyrd
> > mobile) that has appcache support, and an automatic detection on install for
> > that app will be likely impossible. The <html manifest=""> tag is not in the
> > top-level document, but inside an iframe in the page. And to add to the fun
> > that iframe is dynamically created and inserted by JS after they detect the
> > browser has offline support. 
> 
> We may either:
> - don't support preload for such apps
> - let apps specific explicitly in the web app manifest where the app cache
> manifest is ; I believe that could be very helpful not just for this bug

I think we're going to want to do both these things, as trying to accommodate all possible ways that an app might dynamically specify its appcache manifest will be painful (and also seems unnecessary).

So in this initial implementation the manifest will have to be specified via the attribute on the <html> element; and in a subsequent followup we'll add a field to the app manifest for specifying it.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 761946
Depends on: 762115
Blocks: 766402
Blocks: 772919
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: