Last Comment Bug 674728 - Allow web apps to be "pinned", so that they're guaranteed to be fully cached locally
: Allow web apps to be "pinned", so that they're guaranteed to be fully cached ...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: Thinker Li [:sinker]
: Jason Smith [:jsmith]
: Patrick McManus [:mcmanus]
Mentors:
: 383014 (view as bug list)
Depends on: 727685 780878
Blocks: webapi 702369 b2g-demo-phone b2g-app-updates 736193 739868
  Show dependency treegraph
 
Reported: 2011-07-27 15:09 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-10-11 00:37 PDT (History)
35 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Database schema and commands (WIP) (4.48 KB, patch)
2012-02-12 08:21 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Add pinned attribute to nsIApplicationCache (WIP) (4.65 KB, patch)
2012-02-12 08:21 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Implement pinned attribute for SQL cache device (WIP) (2.38 KB, patch)
2012-02-12 08:21 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 4: Special works for pinned app during cache updating (WIP) (8.38 KB, patch)
2012-02-12 08:21 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v2 (4.65 KB, patch)
2012-02-12 21:45 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Implement pinned attribute for SQL cache device (WIP), v2 (3.06 KB, patch)
2012-02-12 21:45 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 4: Special works for pinned app during cache updating (WIP), v2 (8.38 KB, patch)
2012-02-12 21:45 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Database schema and commands (WIP) (7.09 KB, patch)
2012-02-23 16:59 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v2 (4.66 KB, patch)
2012-02-23 16:59 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Implement pinned attribute for SQL cache device (WIP), v2 (5.13 KB, patch)
2012-02-23 16:59 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 4: Special works for pinned app during cache updating (WIP), v2 (11.24 KB, patch)
2012-02-23 16:59 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP) (20.47 KB, patch)
2012-02-23 17:00 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP) (3.94 KB, patch)
2012-02-23 17:00 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 7: Testcase for pinned application caches (6.37 KB, patch)
2012-02-23 17:00 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 8: Fix testcases using nsIOfflineCacheUpdate (6.77 KB, patch)
2012-02-23 17:00 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Database schema and commands (WIP), v2 (8.95 KB, patch)
2012-03-03 10:34 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Implement pinned attribute for SQL cache device (WIP), v3 (5.18 KB, patch)
2012-03-03 10:34 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 4: Special works for pinned app during cache updating (WIP), v3 (10.50 KB, patch)
2012-03-03 10:34 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v2 (27.32 KB, patch)
2012-03-03 10:34 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v2 (4.04 KB, patch)
2012-03-03 10:35 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 7: Testcase for pinned application caches, v2 (7.53 KB, patch)
2012-03-03 10:35 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 8: Fix testcases using nsIOfflineCacheUpdate, v2 (7.29 KB, patch)
2012-03-03 10:35 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v3 (28.18 KB, patch)
2012-03-04 06:42 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 7: Testcase for pinned application caches, v3 (7.95 KB, patch)
2012-03-04 06:43 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 8: Fix testcases using nsIOfflineCacheUpdate, v3 (6.87 KB, patch)
2012-03-04 06:43 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v3 (4.65 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Database schema and commands (WIP), v3 (8.94 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Implement pinned attribute for SQL cache device (WIP), v4 (5.16 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 4: Special works for pinned app during cache updating (WIP), v4 (10.49 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v3 (4.04 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v4 (28.17 KB, patch)
2012-03-04 06:55 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 7: Testcase for pinned application caches, v4 (10.01 KB, patch)
2012-03-04 23:35 PST, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Use pin-app permission at permission manager (49.86 KB, patch)
2012-03-21 05:59 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Pinned apps get higher priority for cache. (14.96 KB, patch)
2012-03-22 00:51 PDT, Thinker Li [:sinker]
honzab.moz: review-
Details | Diff | Splinter Review
Part 2: Testcase for pinned application caches, v5 (10.25 KB, patch)
2012-03-22 00:53 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Pinned apps get higher priority for cache., v2 (23.59 KB, patch)
2012-03-23 04:00 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Testcase for pinned application caches, v6 (8.33 KB, patch)
2012-03-24 06:28 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Evict cache asynchronized (WIP) (15.59 KB, patch)
2012-03-25 19:38 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 3: Evict cache asynchronized (WIP) (23.59 KB, patch)
2012-03-26 04:17 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Pinned apps get higher priority for cache, v3 (20.45 KB, patch)
2012-03-27 01:07 PDT, Thinker Li [:sinker]
honzab.moz: review+
Details | Diff | Splinter Review
Part 2: Testcase for pinned application caches, v7 (9.97 KB, patch)
2012-03-27 01:07 PDT, Thinker Li [:sinker]
honzab.moz: review+
Details | Diff | Splinter Review
Part 3: Evict cache asynchronized, v2 (23.92 KB, patch)
2012-03-27 01:07 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Pinned apps get higher priority for cache, v4 (26.32 KB, patch)
2012-03-28 02:57 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 2: Testcase for pinned application caches, v8 (9.65 KB, patch)
2012-03-28 02:57 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review
Part 1: Pinned apps get higher priority for cache, v5 (26.35 KB, patch)
2012-03-29 03:14 PDT, Thinker Li [:sinker]
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-27 15:09:51 PDT
Goal
 - I visit https://apps.com/phone.html, a sweet phone app.  Me want.
 - I press the right buttons in the UI to make this replace my current phone app.
 - When I reboot my phone, my phone app had better be loadable there even if I don't have a data Internet connection.

This seems to require: (i) knowing what "fully loaded" means for an app.  HTML manifest?; (ii) ensuring all those files are in the app cache; (iii) ensuring those files are never evicted.

I suspect this would be pretty easy to implement in current Gecko, depending on (i).
Comment 1 Matthew Phillips 2011-08-23 09:54:10 PDT
Chris, I understand you wanting this for a phone app, but how about for a game?

If what you need is a way to guarantee cache for critical applications (like a phone app) then why not just place that exact restriction on those critical apps?  For example your phone app needs to request to become the default.  Maybe it calls

requestAnswerPermission() which prompts the user to accept or decline.  We could simple ignore this function call if the page doesn't contain a cache.manifest file that minimally includes the current page html and executing js as being available offline.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-23 17:49:39 PDT
Yes, this feature is intended to be available to any page with the right manifest.  Apps that are "installed", using the new APIs being hacking on, should probably be pinned as part of the installation process.

For other apps, that the user navigates to through links/URL-bar/etc., I think I lean more towards pinning being an intentional user action instead of a response to a request made by the page itself.  The user action could be as simple as "bookmarking" a page is in FF today, press a little button near the URL identifier.  In fact we could reuse that bit of UX entirely, make bookmark => pin for apps with manifests.  In general, UAs already try to cache pages for performance anyway, and it's not clear that users would be able to intelligently respond to a "Pin: Allow/Deny?" request.  It's a somewhat complex concept to communicate concisely, too.
Comment 3 Thinker Li [:sinker] 2012-02-04 05:28:53 PST
We can make a page and its relative pages never being purged (with maximum expired time).  But, how to purge/uninstall an app is important, too.  We need a database for pinned apps, more than bookmarking them.  The database records URLs of apps and their pages, so we can uninstall any of them later.  We also need to consider how to update apps.  Either allow users to trigger a refreshing, and allow apps to refresh them-self.
Comment 4 Thinker Li [:sinker] 2012-02-08 23:40:33 PST
It looks like what is mentioned at https://developer.mozilla.org/en/Offline_resources_in_Firefox .  Do I miss any thing?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-08 23:47:26 PST
That's the mechanism we want to use.  The additional bit we need is to ensure that resources added to the appcache by installed web apps never get purged from the cache, i.e. are "pinned".
Comment 6 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-02-09 00:13:26 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> That's the mechanism we want to use.  The additional bit we need is to
> ensure that resources added to the appcache by installed web apps never get
> purged from the cache, i.e. are "pinned".

Not sure what your "additional bit" means, coz the behavior described follows is already what the AppCache do.

For example, if you go to Firefox -> Pref -> Advanced -> Network and click [Clean Now] under "Cached Web Content" section, the cache in Offline AppCache (i.e., website listed in "Offline Web Content and User Data") will not be deleted.

I think the thing you want here is just to introduce the Offline AppCache prompt in B2G and Gaia. Anything else than that would involve working with new features similar to the ability to view cached content in "Offline Mode" of Netscape Navigator.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-09 00:24:41 PST
Users being able to forceably clear the cache is one thing.  (That I don't particularly care about.)

The cache is fixed size (see about:cache --- "Maximum storage size: 	512000 KiB").  If the cache fills up, and the user browses to a page P that has an appcache manifest, then gecko has two choices
 1. evict old entries from the appcache, install P (likely what happens)
 2. fail to install P to appcache

We don't want (2) for installed apps (installed through navigator.mozApps).  If the user tries to install an app, we want older non-installed offline resources to be evicted to make room for the new app.  So that's approach (1).

BUT, in (1), what we *don't* want to happen is evicting apps that were installed through navigator.mozApps.  The user has explicitly asked that the apps be installed.  Removing them should be done by the user, by uninstalling them.

If the cache fills up with *installed* apps, so that nothing can be evicted, then we need to fail to install new apps until older ones are uninstalled by the user.

Does that make sense?
Comment 8 Thinker Li [:sinker] 2012-02-09 08:49:37 PST
Should we remove an app if the manifest was removed from origin?
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-09 13:44:09 PST
That's a good question.  That would be something like the vendor dropping support for the app.

For now, I think we should *not* do that.  If I pay $5 for a game, and then the vendor goes out of business, I don't want the incidental fact that their website shuts down to remove from all my devices the game that I payed for.
Comment 10 Thinker Li [:sinker] 2012-02-10 07:12:59 PST
I will take this bug if there is no one to stop me.
Comment 11 Tim Guan-tin Chien [:timdream] (please needinfo) 2012-02-11 09:55:00 PST
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> That's a good question.  That would be something like the vendor dropping
> support for the app.
> 
> For now, I think we should *not* do that.  If I pay $5 for a game, and then
> the vendor goes out of business, I don't want the incidental fact that their
> website shuts down to remove from all my devices the game that I payed for.

That will break the Offline AppCache standard

http://www.whatwg.org/specs/web-apps/current-work/multipage/offline.html#event-appcache-obsolete

If I understand it correctly, the spec states if the browser received 404 or 410 response when attempted to check the manifest, it should remove the application cache.

I do agree that Chris's case is a valid use case though.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-11 15:45:45 PST
We're trying to reuse appcache as machinery to support installed applications.  Installing an app is slightly different from appcache though, hence the new work here and in the dependent bugs.

To take another extreme example, if gaia.org 404's for whatever reason, you don't want your dialer application to be deleted and no longer be able to make emergency calls.
Comment 13 Thinker Li [:sinker] 2012-02-11 20:04:08 PST
I think we need to find out solutions for potential issues.  Maybe to file more bugs.  For example,

 - Provide an option that users can stop updating the applications
 - Rollback to previous version.

For an buggy proxy server or network, we may get a broken manifest and flush out all cached entries.  Or, manifest may became empty for some reason.  There is no message digest for application manifest files.  We need to help people for these non-meaningful or intent occasions.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-11 22:27:48 PST
This bug covers pinning resources to app cache.  We can worry about other issues in followup work.
Comment 15 Thinker Li [:sinker] 2012-02-12 08:21:37 PST
Created attachment 596482 [details] [diff] [review]
Part 1: Database schema and commands (WIP)
Comment 16 Thinker Li [:sinker] 2012-02-12 08:21:43 PST
Created attachment 596483 [details] [diff] [review]
Part 2: Add pinned attribute to nsIApplicationCache (WIP)
Comment 17 Thinker Li [:sinker] 2012-02-12 08:21:49 PST
Created attachment 596484 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP)
Comment 18 Thinker Li [:sinker] 2012-02-12 08:21:56 PST
Created attachment 596485 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP)
Comment 19 Thinker Li [:sinker] 2012-02-12 21:45:05 PST
Created attachment 596568 [details] [diff] [review]
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v2

--
Attachment #596483 [details] [diff] - Attachment is obsolete: true
Comment 20 Thinker Li [:sinker] 2012-02-12 21:45:10 PST
Created attachment 596569 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP), v2

--
Attachment #596484 [details] [diff] - Attachment is obsolete: true
Comment 21 Thinker Li [:sinker] 2012-02-12 21:45:15 PST
Created attachment 596570 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP), v2

--
Attachment #596485 [details] [diff] - Attachment is obsolete: true
Comment 22 Thinker Li [:sinker] 2012-02-23 16:59:36 PST
Created attachment 600243 [details] [diff] [review]
Part 1: Database schema and commands (WIP)

--
Attachment #596482 [details] [diff] - Attachment is obsolete: true
Comment 23 Thinker Li [:sinker] 2012-02-23 16:59:43 PST
Created attachment 600244 [details] [diff] [review]
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v2

--
Attachment #596568 [details] [diff] - Attachment is obsolete: true
Comment 24 Thinker Li [:sinker] 2012-02-23 16:59:50 PST
Created attachment 600245 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP), v2

--
Attachment #596569 [details] [diff] - Attachment is obsolete: true
Comment 25 Thinker Li [:sinker] 2012-02-23 16:59:56 PST
Created attachment 600246 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP), v2

--
Attachment #596570 [details] [diff] - Attachment is obsolete: true
Comment 26 Thinker Li [:sinker] 2012-02-23 17:00:37 PST
Created attachment 600247 [details] [diff] [review]
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP)
Comment 27 Thinker Li [:sinker] 2012-02-23 17:00:44 PST
Created attachment 600248 [details] [diff] [review]
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP)
Comment 28 Thinker Li [:sinker] 2012-02-23 17:00:51 PST
Created attachment 600249 [details] [diff] [review]
Part 7: Testcase for pinned application caches
Comment 29 Thinker Li [:sinker] 2012-02-23 17:00:57 PST
Created attachment 600250 [details] [diff] [review]
Part 8: Fix testcases using nsIOfflineCacheUpdate
Comment 30 Thinker Li [:sinker] 2012-03-03 10:34:31 PST
Created attachment 602630 [details] [diff] [review]
Part 1: Database schema and commands (WIP), v2

--
Attachment #600243 [details] [diff] - Attachment is obsolete: true
Comment 31 Thinker Li [:sinker] 2012-03-03 10:34:39 PST
Created attachment 602631 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP), v3

--
Attachment #600245 [details] [diff] - Attachment is obsolete: true
Comment 32 Thinker Li [:sinker] 2012-03-03 10:34:47 PST
Created attachment 602632 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP), v3

--
Attachment #600246 [details] [diff] - Attachment is obsolete: true
Comment 33 Thinker Li [:sinker] 2012-03-03 10:34:55 PST
Created attachment 602633 [details] [diff] [review]
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v2

--
Attachment #600247 [details] [diff] - Attachment is obsolete: true
Comment 34 Thinker Li [:sinker] 2012-03-03 10:35:04 PST
Created attachment 602634 [details] [diff] [review]
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v2

--
Attachment #600248 [details] [diff] - Attachment is obsolete: true
Comment 35 Thinker Li [:sinker] 2012-03-03 10:35:11 PST
Created attachment 602635 [details] [diff] [review]
Part 7: Testcase for pinned application caches, v2

--
Attachment #600249 [details] [diff] - Attachment is obsolete: true
Comment 36 Thinker Li [:sinker] 2012-03-03 10:35:18 PST
Created attachment 602636 [details] [diff] [review]
Part 8: Fix testcases using nsIOfflineCacheUpdate, v2

--
Attachment #600250 [details] [diff] - Attachment is obsolete: true
Comment 37 Thinker Li [:sinker] 2012-03-04 06:42:54 PST
Created attachment 602715 [details] [diff] [review]
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v3

--
Attachment #602633 [details] [diff] - Attachment is obsolete: true
Comment 38 Thinker Li [:sinker] 2012-03-04 06:43:02 PST
Created attachment 602716 [details] [diff] [review]
Part 7: Testcase for pinned application caches, v3

--
Attachment #602635 [details] [diff] - Attachment is obsolete: true
Comment 39 Thinker Li [:sinker] 2012-03-04 06:43:09 PST
Created attachment 602717 [details] [diff] [review]
Part 8: Fix testcases using nsIOfflineCacheUpdate, v3

--
Attachment #602636 [details] [diff] - Attachment is obsolete: true
Comment 40 Thinker Li [:sinker] 2012-03-04 06:55:10 PST
Created attachment 602718 [details] [diff] [review]
Part 2: Add pinned attribute to nsIApplicationCache (WIP), v3

--
Attachment #600244 [details] [diff] - Attachment is obsolete: true
Comment 41 Thinker Li [:sinker] 2012-03-04 06:55:18 PST
Created attachment 602719 [details] [diff] [review]
Part 1: Database schema and commands (WIP), v3

--
Attachment #602630 [details] [diff] - Attachment is obsolete: true
Comment 42 Thinker Li [:sinker] 2012-03-04 06:55:25 PST
Created attachment 602720 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP), v4

--
Attachment #602631 [details] [diff] - Attachment is obsolete: true
Comment 43 Thinker Li [:sinker] 2012-03-04 06:55:34 PST
Created attachment 602721 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP), v4

--
Attachment #602632 [details] [diff] - Attachment is obsolete: true
Comment 44 Thinker Li [:sinker] 2012-03-04 06:55:41 PST
Created attachment 602722 [details] [diff] [review]
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v3

--
Attachment #602634 [details] [diff] - Attachment is obsolete: true
Comment 45 Thinker Li [:sinker] 2012-03-04 06:55:49 PST
Created attachment 602723 [details] [diff] [review]
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v4

--
Attachment #602715 [details] [diff] - Attachment is obsolete: true
Comment 46 Mozilla RelEng Bot 2012-03-04 11:00:24 PST
Try run for 868d98d8ae14 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=868d98d8ae14
Results (out of 217 total builds):
    exception: 2
    success: 178
    warnings: 37
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-868d98d8ae14
Comment 47 Thinker Li [:sinker] 2012-03-04 23:35:03 PST
Created attachment 602820 [details] [diff] [review]
Part 7: Testcase for pinned application caches, v4

--
Attachment #602716 [details] [diff] - Attachment is obsolete: true
Comment 48 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-05 13:28:18 PST
I can't review this code so I'm going to unset myself.  Apologies in advance for the bugspam.

Thinker, there are a lot of patches here and the problem that's being solved may not be clear to Boris and Jason.  Can you write a comment describing what these patches do, and then maybe a sentence or two describing each patch individually?  Thanks!
Comment 49 Jason Duell [:jduell] (needinfo me) 2012-03-05 14:05:08 PST
Comment on attachment 602717 [details] [diff] [review]
Part 8: Fix testcases using nsIOfflineCacheUpdate, v3

Moving reviews for this bug to Honza Bambas, who knows appcache the best on the necko team.  Honza, feel free to reassign/delegate if you need help with these.
Comment 50 Jason Duell [:jduell] (needinfo me) 2012-03-05 14:10:01 PST
Moving this to Core:Networking:Cache both because that's the logical place for it, and because I suspect a lot of folks on the necko team will want to follow it.
Comment 51 Thinker Li [:sinker] 2012-03-05 17:55:33 PST
Comment on attachment 602720 [details] [diff] [review]
Part 3: Implement pinned attribute for SQL cache device (WIP), v4

This patch implements accessors of |nsIApplicationCache::pinned|.  It controls whether a is pinned app cache at storage level.  It persists the value of |pinned| as one of fields of |moz_cache_groups| table.
Comment 52 Thinker Li [:sinker] 2012-03-05 18:05:33 PST
Comment on attachment 602721 [details] [diff] [review]
Part 4: Special works for pinned app during cache updating (WIP), v4

This patch add the code to deal with pinned app caching for |nsOfflineCacheUpdate|.  It does not provide any way to start a pinned app cache here.  It assumes callers change the value of |mPinned| to start pinned ones ( although it is impossible here).
Comment 53 Thinker Li [:sinker] 2012-03-05 18:30:53 PST
Comment on attachment 602723 [details] [diff] [review]
Part 5: nsIOfflineCacheUpdateService supports pinned (WIP), v4

This patch changes interface nsIOfflineCacheUpdateService that introduce a lot of minor changes for IPC protocols.  The major task is to let callers starting pinned app caches through |nsIOfflineCacheUpdateService|.

This patch also adds |ALLOW_PINNED| as one of values of "offline-app" perm type.  |nsOfflineCachePendingUpdate| checks for this value to decide whether it is a pinned or non-pinned.  The applications (browser) are supposed to ask user whether to pin an WEB app and set |ALLOW_PINNED| for pinned WEB apps.
Comment 54 Thinker Li [:sinker] 2012-03-05 20:46:27 PST
Comment on attachment 602722 [details] [diff] [review]
Part 6: Fix JS codes using nsIOfflineUpdateCache (WIP), v3

Fix relative JS code for changes of |nsIOfflineCacheUpdateService|.  They start new caches a non-pinned app cache.  These code blocks are related to implementation of applications (browsers).  They are supposed to revise their code to pass purpose value to start pinned app caching.
Comment 55 Boris Zbarsky [:bz] (still a bit busy) 2012-03-06 07:56:19 PST
those "sentence or two" bits should be in the checkin comments in the patches, no?  That's exactly what checkin comments are for!  Compare "nsIOfflineCacheUpdateService supports pinned" and comment 53 as checkin comments....

And yes, Honza is a better reviewer for the code here than I am.
Comment 56 Thinker Li [:sinker] 2012-03-06 08:30:38 PST
(In reply to Boris Zbarsky (:bz) from comment #55)
> those "sentence or two" bits should be in the checkin comments in the
> patches, no?  That's exactly what checkin comments are for!  Compare
> "nsIOfflineCacheUpdateService supports pinned" and comment 53 as checkin
> comments....
It is my misunderstanding about "comment".  Update these patches later.
Comment 57 Honza Bambas (:mayhemer) 2012-03-06 13:57:44 PST
Thinker, I started to look at the patches, so please don't update them or you destroy my draft review comments in splinter.

Can you please add an outline of how the process of "pinning" an app will be done from top of the UI?  I mean, what the order of operations can be, when what happens etc.

Thanks!
Comment 58 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-06 15:40:07 PST
Is there a description anywhere what changes are being made to the appcache here.

It's awesome you're working on the App cache, and I think we can make it work for the use cases mentioned in this bug. But we should we have for a while been planning some pretty major changes to the app cache and I want to make sure that these changes fit in that.

Also, if you'd be available to implement those changes, in addition to the ones planned here, that would be super awesome :-)
Comment 59 Honza Bambas (:mayhemer) 2012-03-06 15:50:44 PST
(In reply to Jonas Sicking (:sicking) from comment #58)
> we have for a
> while been planning some pretty major changes to the app cache

Bug # ?
Comment 60 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-06 18:53:26 PST
Well, we don't have a bug filed yet since we don't know exactly what changes to make.

But at the very least:

* Make AppCache an actual cache, i.e. don't have a prompt but instead toss out data once we've filled up X MB of total AppCache data (pinned sites would be excluded from this quota).

* Ensure that installed OWA apps get "pinned" (i.e. that there's no prompt but that they don't count towards quota).

* Figure out a solution for the current "user only sees updated site on second visit" problem. Lots of different strategies we can use here. We'll probably want to use different strategies for pinned sites.

I didn't actually realize that anyone else was working on the appcache. The discussion so far has mostly been between me and Bent.
Comment 61 Honza Bambas (:mayhemer) 2012-03-06 18:57:56 PST
Thanks, I just wanted to be sure there is nothing I wouldn't know about.

FYI: I'm a maintainer of that code, or maybe these days more just a reviewer.
Comment 62 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-06 19:33:53 PST
(In reply to Jonas Sicking (:sicking) from comment #60)
> * Make AppCache an actual cache, i.e. don't have a prompt but instead toss
> out data once we've filled up X MB of total AppCache data (pinned sites
> would be excluded from this quota).
> 
> * Ensure that installed OWA apps get "pinned" (i.e. that there's no prompt
> but that they don't count towards quota).
> 

Yep, this is the goal here.

> * Figure out a solution for the current "user only sees updated site on
> second visit" problem. Lots of different strategies we can use here. We'll
> probably want to use different strategies for pinned sites.
> 

Note sure I understand this.
Comment 63 Thinker Li [:sinker] 2012-03-06 21:12:42 PST
(In reply to Jonas Sicking (:sicking) from comment #60)
> * Make AppCache an actual cache, i.e. don't have a prompt but instead toss
> out data once we've filled up X MB of total AppCache data (pinned sites
> would be excluded from this quota).

This patch set picks and evicts an non-pinned app from cache randomly (most old one actually) for caching a pinned app while we have reached the maximum size of the storage. The rules of picking for evicting can be refined later. (another bug?)

> 
> * Figure out a solution for the current "user only sees updated site on
> second visit" problem. Lots of different strategies we can use here. We'll
> probably want to use different strategies for pinned sites.

I am not sure I understand this, too.
Do you mean versioning app caches?  User can rollback its apps to elder versions.
Since users bought an app from a store, the may not happy to see a broken app caused by an incorrect update of manifest.  They should be able to rollback their apps to elder versions.
Comment 64 Thinker Li [:sinker] 2012-03-06 22:34:49 PST
(In reply to Honza Bambas (:mayhemer) from comment #57)
> Can you please add an outline of how the process of "pinning" an app will be
> done from top of the UI?  I mean, what the order of operations can be, when
> what happens etc.

For a new app, the browser would receive an "MozApplicationManifest"
chrome event from nsDocument.
  - nsDocument would check root node of the loaded document for
    "manifest" attribute, and send a chrome event
    "MozApplicationManifest" if "manifest" is existing.
  - The browser calls nsContentUtils::OfflineAppAllowed() to check if
    the app has previously allowed for caching, once the browser have
    received a "MozApplicationManifest". (see "offline-app" permission.
  - If the app has allowed, the browser does nothing.
  - Otherwise, the browser prompts the user for whether to cache/or
    pin the app.
  - Once the user agree to cache/or pin an app,
    - the browser calls nsIOfflineCacheUpdate::scheduleUpdate() to
      cache/or pin the app.
    - add permission "offline-app" for the app (URI) with value
      - ALLOW_ACTION, for a normal caching, or
      - ALLOW_PINNED, for a pinned app.

For an app already in an app cache, the browser would also receive an
"MozApplicationManifest" chrome event.  But, it does nothing since its
URL has an "offline-app" permission (ALLOW_ACTION or ALLOW_PINNED).
Intead, nsContentSink will call
nsIOfflineCacheUpdate::ScheduleOnDocumentStop() to start an updating.
 - nsContentSink::ProcessOfflineManifest() checks root node of the
   loaded document for "manifest" attribute.
 - If there is an "manifest" attribute, it
   - calls nsContentUtils::OfflineAppAllowed() for checking if the app
     have been allowed for caching.
   - call nsIOfflineCacheUpdate::ScheduleOnDocumentStop() if the app
     have been allowed.
     - ScheduleOnDocumentStop() calls
       nsIOfflineCacheUpdate::offlineAppPinned() to check if the app
       is pinned.

Is it clear?
Comment 65 Honza Bambas (:mayhemer) 2012-03-07 09:37:41 PST
(In reply to Thinker Li [:sinker] from comment #64)
> (In reply to Honza Bambas (:mayhemer) from comment #57)
> Is it clear?

Yes, thanks.  I actually know all of these, I was more interested in what new you were adding to this.  I was confused with "pinning" what here is different from what "pinning" means on desktop Firefox (to have a small persistent app tab: http://imagecdn.maketecheasier.com/2010/08/firefox4-app-tab.png)

As I understand, the only changes are:
- for apps the prompt will change to "do you want to pin" instead of saying just "do you want to cache offline"
- domains with PINNED permission will rule with different cache limit priority, i.e. PINNED domain can make ALLOWED domains evict


I play with an idea of having a whole new database and cache folder for the PINNED cache instead of messing the ALLOWED and PINNED databases together.  This is not thought through very much, though, raising it more to hear your opinion since you've spent a lot of time on these changes already.



(In reply to Jonas Sicking (:sicking) from comment #60)
> * Figure out a solution for the current "user only sees updated site on
> second visit" problem. Lots of different strategies we can use here. We'll
> probably want to use different strategies for pinned sites.

To explain:

an already cached page performs an update check (nsIOfflineCacheUpdate::ScheduleOnDocumentStop()).  When an update is found, it is not used until the page is reloaded by the user.  We would like to have a way to "update" - what actually means reload - the page immediately.  I solved this in my web app with a prompt saying "Update ready, apply now?" [Yes] [No].  On clicking yes I just do location.reload().
Comment 66 Thinker Li [:sinker] 2012-03-07 18:09:32 PST
(In reply to Honza Bambas (:mayhemer) from comment #65)
> (In reply to Thinker Li [:sinker] from comment #64)
> > (In reply to Honza Bambas (:mayhemer) from comment #57)
> > Is it clear?
> 
> Yes, thanks.  I actually know all of these, I was more interested in what
> new you were adding to this.  I was confused with "pinning" what here is
> different from what "pinning" means on desktop Firefox (to have a small
> persistent app tab:
> http://imagecdn.maketecheasier.com/2010/08/firefox4-app-tab.png)

"Pinning" means making an app being cached and never being evicted for the offline cache storage's reaching its maximum size.  It is nothing about "Pin as App Tab" of firefox.  Do you have a better name?

> 
> As I understand, the only changes are:
> - for apps the prompt will change to "do you want to pin" instead of saying
> just "do you want to cache offline"
> - domains with PINNED permission will rule with different cache limit
> priority, i.e. PINNED domain can make ALLOWED domains evict

Correct!

> 
> 
> I play with an idea of having a whole new database and cache folder for the
> PINNED cache instead of messing the ALLOWED and PINNED databases together. 
> This is not thought through very much, though, raising it more to hear your
> opinion since you've spent a lot of time on these changes already.

Does "a whole new database and cache folder" means to instantiate both nsOfflineCacheUpdateService and nsDiskCacheDevice two times, one for pinned and another for non-pinned?
If I am correct, the current nsOfflineCacheUpdate in m-c never evicts old caches.  It just stops caching new apps and entries while it is filled.  The idea of two instances makes no different between PINNED and ALLOWED except where they are and separated quota.  But, it is a fail of current implementation, we can fix it up with evicting entries randomly for ALLOWED apps for reaching maximum size.
It is more easy to understand separated devices for PINNED/installed apps and ALLOWED/cached apps for users.
Actually, I think users would like to have a unlimited quota for PINNED apps XD
Comment 67 Thinker Li [:sinker] 2012-03-15 22:55:42 PDT
@Honza, is there any update or comment?
Comment 68 Honza Bambas (:mayhemer) 2012-03-16 07:15:41 PDT
(In reply to Thinker Li [:sinker] from comment #67)
> @Honza, is there any update or comment?

I'll get to this on Monday.  Just to note that I probably have left the idea of two databases.  It would mean more complication then simplification.
Comment 69 Honza Bambas (:mayhemer) 2012-03-20 14:10:23 PDT
(In reply to Thinker Li [:sinker] from comment #66)
> > - for apps the prompt will change to "do you want to pin" instead of saying
> > just "do you want to cache offline"
> > - domains with PINNED permission will rule with different cache limit
> > priority, i.e. PINNED domain can make ALLOWED domains evict
> 
> Correct!


Hmm... according this you don't need any of these large patches you've made to implement this feature.

To explain how the current (just "offline") implementation works:

- ability to cache offline is indicated by "offline-app" permission (ALLOW/DENY/UNKNOWN/ALLOW_NO_WARNING) for the whole DOMAIN
- when manifest= attribute is found in the document then the permission is checked for the document's domain
- on UNKNOWN
  - pop up the prompt to allow
  - on "Allow" click
    - set the "offline-app" permission to ALLOW
    - schedule the (first) update to fetch the cache content
  - on "Don't allow" click obviously do nothing
- on ALLOWED
  - update is scheduled (this may even be the first update, 
    since there may be more offline apps on a single domain)

Since you just want some details to change for the special "pinned" case, you may just go the way of permissions and not flags in the database with carrying and setting the pinned flags here and there.

If the operation of "Allow" in the prompt is actually to "pin", then just set beside the "offline-app" permission also a new permission "pin-app" to ALLOW for the domain.

Updates will read the permission it self using the permission manager.

When the permission is set, then just don't obsolete the cache on 40X or do any specific actions you need.  The "pinned" information doesn't need to be in the database at all.
Comment 70 Thinker Li [:sinker] 2012-03-20 18:56:20 PDT
(In reply to Honza Bambas (:mayhemer) from comment #69)
> (In reply to Thinker Li [:sinker] from comment #66)
> > > - for apps the prompt will change to "do you want to pin" instead of saying
> > > just "do you want to cache offline"
> > > - domains with PINNED permission will rule with different cache limit
> > > priority, i.e. PINNED domain can make ALLOWED domains evict
> > 
> > Correct!
> 
> 
> Hmm... according this you don't need any of these large patches you've made
> to implement this feature.
> 
> To explain how the current (just "offline") implementation works:
> 
> - ability to cache offline is indicated by "offline-app" permission
> (ALLOW/DENY/UNKNOWN/ALLOW_NO_WARNING) for the whole DOMAIN
> - when manifest= attribute is found in the document then the permission is
> checked for the document's domain
> - on UNKNOWN
>   - pop up the prompt to allow
>   - on "Allow" click
>     - set the "offline-app" permission to ALLOW
>     - schedule the (first) update to fetch the cache content
>   - on "Don't allow" click obviously do nothing
> - on ALLOWED
>   - update is scheduled (this may even be the first update, 
>     since there may be more offline apps on a single domain)
> 
> Since you just want some details to change for the special "pinned" case,
> you may just go the way of permissions and not flags in the database with
> carrying and setting the pinned flags here and there.
> 
> If the operation of "Allow" in the prompt is actually to "pin", then just
> set beside the "offline-app" permission also a new permission "pin-app" to
> ALLOW for the domain.
> 
> Updates will read the permission it self using the permission manager.
> 
> When the permission is set, then just don't obsolete the cache on 40X or do
> any specific actions you need.  The "pinned" information doesn't need to be
> in the database at all.

It means to move all code in the patches from C++ to JS and implemented by applications(browsers) them-self.  Right?!  It seems not to make thing easier.  It just moves the same code around.
The benefits are we don't need to change nsIOfflineCacheUpdateService and all things in JS.
The disadvantage is every application is responsible for the implementation of "pin-app".
Comment 71 Thinker Li [:sinker] 2012-03-20 19:11:43 PDT
(In reply to Thinker Li [:sinker] from comment #70)
> It means to move all code in the patches from C++ to JS and implemented by
> applications(browsers) them-self.  Right?!  It seems not to make thing
> easier.  It just moves the same code around.
> The benefits are we don't need to change nsIOfflineCacheUpdateService and
> all things in JS.
> The disadvantage is every application is responsible for the implementation
> of "pin-app".

I think I had mis-undertood your point.  After the explanation of fabrice, I think you like to remove all flag from db and check permission manager instead of "pinned" flag in DB.
Comment 72 Thinker Li [:sinker] 2012-03-20 22:48:59 PDT
I am working on removing pinned flag from DB and querying permission manager instead.  But, I think there are two disadvantages on this solution.

 1. We should query permission manager everywhere that is not so efficient.
 2. Offline cache will be tightly coupled with a global state at permission manager.

It means to query permission manager for every cached group, one by one, to pick a group for eviction.  It is inefficient to scan all cache groups from head to tail for a non-pinned while most apps in the cache are pinned.
Comment 73 Thinker Li [:sinker] 2012-03-21 05:59:44 PDT
Created attachment 607921 [details] [diff] [review]
Use pin-app permission at permission manager
Comment 74 Thinker Li [:sinker] 2012-03-21 08:16:40 PDT
(In reply to Thinker Li [:sinker] from comment #73)
> Created attachment 607921 [details] [diff] [review]
> Use pin-app permission at permission manager

This patch remove "pinned" flag from DB, and ask permission manager for "pin-app" permission.  The testcase test_pinned_app_cache.js have passed.  I will re-organize all patches later.
Comment 75 Thinker Li [:sinker] 2012-03-22 00:51:39 PDT
Created attachment 608253 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache.
Comment 76 Thinker Li [:sinker] 2012-03-22 00:53:17 PDT
Created attachment 608254 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v5
Comment 77 Honza Bambas (:mayhemer) 2012-03-22 12:19:41 PDT
Thinker, I'm looking at the patches now.  Thanks for the update.  It is much simpler.

I will have few comments to change few things, but not anything major (this time).

One of them is that I can see a major problem with discarding an existing cache group.  Doing it on the main thread will kill (unexpectedly, means hard to track) the main thread.  We cannot do that.

Also, I don't see much reason to do that, as you say your self there will be just few non-pinned offline apps, so the affect may be just little.


Based on that I think the solution here is to allow larger space for the whole offline cache capacity.  I don't see that done in the patch.  We may change the pref for b2g app as a workaround (or already done in a different bug?).

Thinking of a more correct solution, this carries me back to introducing a new database.  We would have a new storage policy STORE_OFFLINE_PINNED and a new instance of nsOfflineCacheDevice working with a different physical directory on the device disk and having (mainly!) a different mCacheCapacity setting.  I have to think about this more deeply, but probably having this in a followup is sufficient and finally may not be that hard to implement.

(Please don't update the patch yet, there will be more comments.)
Comment 78 Honza Bambas (:mayhemer) 2012-03-22 15:32:56 PDT
Comment on attachment 608253 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache.

Review of attachment 608253 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1368,5 @@
>              mObsolete = true;
>              if (mPreviousApplicationCache) {
> +                if (mPinned) {
> +                    // Do not obsolete a pinned application.
> +                    NotifyState(nsIOfflineCacheUpdateObserver::STATE_NOUPDATE);

If you don't want to obsolete the cache, then don't set mObsolete flag.  Notification is OK but not enough.  (mObsolete = !mPinned)

@@ +1431,5 @@
>      rv = item->GetRequestSucceeded(&succeeded);
>  
> +    PRUint32 dummy_cache_type;
> +    rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type);
> +    bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed

We should have an IsContained method for this.  Followup is OK for that.

@@ +1435,5 @@
> +    bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed
> +
> +    while (mPinned && item_doomed &&
> +           (item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT |
> +                               nsIApplicationCache::ITEM_FALLBACK))) {

why are you checking for item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT | nsIApplicationCache::ITEM_FALLBACK) ?

@@ +1442,5 @@
> +
> +        rv = EvictOneNonPinned();
> +        if (NS_FAILED(rv)) break;
> +        rv = RescheduleUpdate();
> +        if (NS_FAILED(rv)) break;

You don't need to reschedule the whole update, just re-fetch the current item again.  You need to introduce a new counter or, check there actually has been some data removed.

@@ +1843,5 @@
>      return rv;
>  }
>  
> +nsresult
> +nsOfflineCacheUpdate::EvictOneNonPinned() {

Style is:
funcName()
{
}

@@ +1863,5 @@
> +
> +    for (i = 0; i < count; i++) {
> +        bool pinned;
> +        nsDependentCString group_name(groups[i]);
> +        nsCOMPtr<nsIURI> uri;

We're used to declare stuff on/close to its first use.

@@ +1870,5 @@
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = cacheService->GetActiveCache(group_name, getter_AddRefs(cache));
> +        if (NS_FAILED(rv)) {
> +            break;

continue; ?

@@ +1877,5 @@
> +        rv = nsOfflineCacheUpdateService::OfflineAppPinned(uri, &pinned);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        if (!pinned) {
> +            rv = cache->Discard();

This is a big problem: on the main thread we may cause a large unexpected and not to any current user action bound (means hard to track) hang.

This must be done asynchronously or must not be done at all.  It would mean to make the space for pinned apps just larger.

::: uriloader/prefetch/nsOfflineCacheUpdate.h
@@ +310,5 @@
>      PRUint32 mRescheduleCount;
>  
>      nsRefPtr<nsOfflineCacheUpdate> mImplicitUpdate;
> +
> +    bool                           mPinned;

Initialize this in the constructor.

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +591,5 @@
>  }
> +
> +nsresult
> +nsOfflineCacheUpdateService::OfflineAppPinned(nsIURI *aDocumentURI,
> +                                              bool *aPinned)

This method should share the code with nsOfflineCacheUpdateService::OfflineAppAllowedForURI and the name should be nsOfflineCacheUpdateService::OfflineAppPinnedForURI
Comment 79 Honza Bambas (:mayhemer) 2012-03-22 16:13:37 PDT
Comment on attachment 608253 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache.

Review of attachment 608253 [details] [diff] [review]:
-----------------------------------------------------------------

You should also discard the cache with the oldest fetch time.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1866,5 @@
> +        nsDependentCString group_name(groups[i]);
> +        nsCOMPtr<nsIURI> uri;
> +
> +        rv = ios->NewURI(group_name, NULL, NULL, getter_AddRefs(uri));
> +        NS_ENSURE_SUCCESS(rv, rv);

And you have NS_NewURI for this.
Comment 80 Thinker Li [:sinker] 2012-03-22 23:08:28 PDT
(In reply to Honza Bambas (:mayhemer) from comment #77)
> Thinking of a more correct solution, this carries me back to introducing a
> new database.  We would have a new storage policy STORE_OFFLINE_PINNED and a
> new instance of nsOfflineCacheDevice working with a different physical
> directory on the device disk and having (mainly!) a different mCacheCapacity
> setting.  I have to think about this more deeply, but probably having this
> in a followup is sufficient and finally may not be that hard to implement.
> 

I agree with you.
It would be better to have a separated database for pinned apps in a follow up.
And, I also think a separated database is a more correct solution.
Comment 81 Thinker Li [:sinker] 2012-03-23 00:28:12 PDT
(In reply to Honza Bambas (:mayhemer) from comment #78)
> Comment on attachment 608253 [details] [diff] [review]
> Part 1: Pinned apps get higher priority for cache.
> 
> Review of attachment 608253 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
> @@ +1368,5 @@
> >              mObsolete = true;
> >              if (mPreviousApplicationCache) {
> > +                if (mPinned) {
> > +                    // Do not obsolete a pinned application.
> > +                    NotifyState(nsIOfflineCacheUpdateObserver::STATE_NOUPDATE);
> 
> If you don't want to obsolete the cache, then don't set mObsolete flag. 
> Notification is OK but not enough.  (mObsolete = !mPinned)

You are right!  It will obsolete the cache for conditions other than mPreviousApplicationcache && mPinned in next update.

> 
> @@ +1431,5 @@
> >      rv = item->GetRequestSucceeded(&succeeded);
> >  
> > +    PRUint32 dummy_cache_type;
> > +    rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type);
> > +    bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed
> 
> We should have an IsContained method for this.  Followup is OK for that.

I will file a followup bug.

> 
> @@ +1435,5 @@
> > +    bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed
> > +
> > +    while (mPinned && item_doomed &&
> > +           (item->mItemType & (nsIApplicationCache::ITEM_EXPLICIT |
> > +                               nsIApplicationCache::ITEM_FALLBACK))) {
> 
> why are you checking for item->mItemType &
> (nsIApplicationCache::ITEM_EXPLICIT | nsIApplicationCache::ITEM_FALLBACK) ?

Should an update fail for URIs that are not specified in a manifest explicitly?
This checking make sure an update does not fail for the error of loading an URI that is not specified in the manifest explicitly.
Comment 82 Thinker Li [:sinker] 2012-03-23 04:00:17 PDT
Created attachment 608653 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache., v2

--
Attachment #608253 [details] [diff] - Attachment is obsolete: true
Comment 83 Thinker Li [:sinker] 2012-03-23 08:06:10 PDT
For pinned apps, we may be able to rollback to earlier versions while latest version is broken.  People would not like to stay at broken version for an accident on manifest or server.  So, it would be better to give user a chance to rollback an app to an earlier revision.
Comment 84 Thinker Li [:sinker] 2012-03-24 06:28:54 PDT
Created attachment 608989 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v6

--
Attachment #608254 [details] [diff] - Attachment is obsolete: true
Comment 85 Thinker Li [:sinker] 2012-03-25 19:38:36 PDT
Created attachment 609193 [details] [diff] [review]
Part 3: Evict cache asynchronized (WIP)
Comment 86 Thinker Li [:sinker] 2012-03-26 04:17:19 PDT
Created attachment 609280 [details] [diff] [review]
Part 3: Evict cache asynchronized (WIP)

--
Attachment #609193 [details] [diff] - Attachment is obsolete: true
Comment 87 Honza Bambas (:mayhemer) 2012-03-26 14:46:35 PDT
Comment on attachment 608653 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache., v2

Review of attachment 608653 [details] [diff] [review]:
-----------------------------------------------------------------

r=0, please let me check the final version ones more.  But this looks good.

::: netwerk/base/public/nsIApplicationCacheService.idl
@@ +97,5 @@
> +    /**
> +     * Get the list of application cache groups in the order of
> +     * activating time.
> +     */
> +    void getGroupsTimeOrder([optional] out unsigned long count,

Ordered ?

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1302,5 @@
>      if (!succeeded || !mManifestItem->ParseSucceeded()) {
>          return NS_ERROR_FAILURE;
>      }
>  
> +    if (mRescheduleCount == 0 && !mManifestItem->NeedsUpdate()) {

Probably don't need mRescheduleCount == 0 in the cond anymore?

@@ +1451,5 @@
> +
> +        rv = item->Cancel();
> +        if (NS_FAILED(rv)) break;
> +
> +        mPinnedEntryRetriesCount++;

Reset this on successful load.

@@ +1452,5 @@
> +        rv = item->Cancel();
> +        if (NS_FAILED(rv)) break;
> +
> +        mPinnedEntryRetriesCount++;
> +        mCurrentItem--;

Maybe just ++ this after this block and add a comment here that ProcessNextURI will process the current one since we didn't move the counter?

@@ +1456,5 @@
> +        mCurrentItem--;
> +        ProcessNextURI();
> +
> +        return;
> +    }

BTW, I really don't like this while cycle.  I understand it and sometimes miss a break from if my self, but please move this to a separate method returning true when eviction succeeded or something like that.

@@ +1507,5 @@
>          NotifyState(nsIOfflineCacheUpdateObserver::STATE_ERROR);
>      }
>  
> +    if (NS_FAILED(aStatus)) {
> +        nsresult rv = RescheduleUpdate();

Personally, I think this change (new method) is not need anymore.  Please remove it to keep the patch and reviewing it as simple as possible.

@@ +1878,5 @@
> +                                                                 &pinned);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        if (!pinned) {
> +            rv = cache->Discard();

This needs a followup fixed before any (pre)production public release.  We must not do this extremely expensive operation on the main thread.

::: uriloader/prefetch/nsOfflineCacheUpdate.h
@@ +263,5 @@
> +     * Restart this update.
> +     *
> +     * Return error for retring for a maximum times.
> +     */
> +    nsresult RescheduleUpdate();

Probably remove this.

::: uriloader/prefetch/nsOfflineCacheUpdateService.cpp
@@ +573,3 @@
>  
>      if (perm == nsIPermissionManager::UNKNOWN_ACTION) {
>          static const char kPrefName[] = "offline-apps.allow_by_default";

This means offline-apps.allow_by_default = true will allow also pinning automatically, I trust you know what you're doing.  But I would personally don't do that...
Comment 88 Honza Bambas (:mayhemer) 2012-03-26 14:58:54 PDT
Comment on attachment 608989 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v6

Can you please write a summary (also to the test file directly) of what the test is doing?  It makes understanding it easier (also for you).  Thanks.
Comment 89 Thinker Li [:sinker] 2012-03-27 01:07:18 PDT
Created attachment 609646 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v3

--
Attachment #608653 [details] [diff] - Attachment is obsolete: true
Comment 90 Thinker Li [:sinker] 2012-03-27 01:07:34 PDT
Created attachment 609647 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v7

--
Attachment #608989 [details] [diff] - Attachment is obsolete: true
Comment 91 Thinker Li [:sinker] 2012-03-27 01:07:49 PDT
Created attachment 609648 [details] [diff] [review]
Part 3: Evict cache asynchronized, v2

--
Attachment #609280 [details] [diff] - Attachment is obsolete: true
Comment 92 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-03-27 03:25:15 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> (iii) ensuring those files are never evicted.
> 
> I suspect this would be pretty easy to implement in current Gecko, depending
> on (i).

Although it seems simple to implement, there is a huge limitation: when Gecko crashes, the cache is reset. This makes the cache totally unusable as an app storage mechanism for mobile devices, AFAICT.

The Necko cache is optimized for avoiding disk I/O (avoiding fsyncs and the like), and is not optimized for persistence. This fundamental design choice is going to cause all kinds of problems if we try to rely on our cache as an app storage mechanism. There is not even a plan yet as to how to rectify it--i.e. it would be extremely optimistic to expect a solution in Q2 if things go according to current plans.
Comment 93 Honza Bambas (:mayhemer) 2012-03-27 12:22:23 PDT
(In reply to Brian Smith (:bsmith) from comment #92)
> Although it seems simple to implement, there is a huge limitation: when
> Gecko crashes, the cache is reset...

Brian, you are talking about HTTP "normal" cache.  This bug is all about using offline app cache.  There it is different.
Comment 94 Honza Bambas (:mayhemer) 2012-03-27 14:10:51 PDT
Comment on attachment 609646 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v3

Review of attachment 609646 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab with some last comments addressed.

I think we may land this now to start playing with this new feature.  To solve some other requirements like capacity and also eviction of some lower priority stuff, we have to introduce the secondary instance of the database with a different quota limit.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1439,5 @@
>      rv = item->GetRequestSucceeded(&succeeded);
>  
> +    PRUint32 dummy_cache_type;
> +    rv = mApplicationCache->GetTypes(item->mCacheKey, &dummy_cache_type);
> +    bool item_doomed = NS_FAILED(rv); // can not find it? -> doomed

You should actually do this under if (mPinned) {} since this may be expensive and you don't need it for otherwise.

This could regress on main thread perf for Fx mobile and desktop.

@@ +1462,5 @@
> +            return;
> +        }
> +
> +        mPinnedEntryRetriesCount++;
> +        // Do a retrying for current item, so mCurrentItem is not advanced.

Just s/Do a retrying for/Retry/  ?

@@ +1902,5 @@
> +        nsCOMPtr<nsIApplicationCache> cache;
> +        rv = cacheService->GetActiveCache(group_name, getter_AddRefs(cache));
> +        // Maybe someone in another thread or process have deleted it.
> +        if (NS_FAILED(rv))
> +            continue;

Err... this check is not enough.  The result may be NS_OK + cache = null.  Add also "|| !cache" to the condition.
Comment 95 Honza Bambas (:mayhemer) 2012-03-27 17:58:36 PDT
Comment on attachment 609646 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v3

Review of attachment 609646 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1888,5 @@
>  }
>  
> +static nsresult
> +EvictOneOfCacheGroups(nsIApplicationCacheService *cacheService,
> +            PRUint32 count, const char * const *groups) {

One more comment - the style is:

funcName()
{
}

Braces on new lines.
Comment 96 Honza Bambas (:mayhemer) 2012-03-27 18:05:52 PDT
Comment on attachment 609647 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v7

Review of attachment 609647 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab but address the following comments please:

The test takes 26 seconds (debug build) on a very fast i7 machine with ssd disk and a tun of memory.  It is unacceptable.  Please lower the limits by a magnitude.  I think the limit for 10k and each page having 1k will work well too.

Then, I've seen this in the test log:

TEST-PASS | undefined | [undefined : undefined] true == true

Just before "Start pinned App2 for success" TEST-INFO output.  Looks like there is something wrong with do_check_true on line 218.

::: netwerk/test/unit/test_pinned_app_cache.js
@@ +4,5 @@
> + * are
> + *
> + *  - start_cache_nonpinned_app1()
> + *
> + *    - Request nsOfflineCacheService to skep pages (4) of app1 on

to "skep" ?  maybe just a gap in my english..

@@ +58,5 @@
> +const kHttpLocation_ip = "http://127.0.0.1:4444/";
> +
> +function manifest1_handler(metadata, response) {
> +  do_print("manifest1\n");
> +  response.setHeader("content-type", "text/cache-manifest");

"text/cache-manifest" is no longer needed.

@@ +82,5 @@
> +  do_print("datafile_handler\n");
> +  let data = "";
> +
> +  while(data.length < kDataFileSize) {
> +    data = data + Math.random().toString(36).substring(0, 8);

Nice :), maybe just 2, 15 for substring.

@@ +124,5 @@
> +    getService(Ci.nsICacheService);
> +  cache_service.evictEntries(Ci.nsICache.STORE_OFFLINE);
> +}
> +
> +function do_app_cache(manifestURL, pageURL, pinned) {

maybe call this start_app_cache_update

@@ +156,5 @@
> +
> +  return update;
> +}
> +
> +function start_n_watch_app_cache(manifestURL,

s/_n_/_and_/ ?

@@ +191,5 @@
> +
> +                            case STATE_ERROR:
> +                              do_throw_todo("App1 cache state = " + state);
> +                              break;
> +                            }

NOUPDATE and OBSOLETE should be considered unexpected here and you should do_throw for them.

@@ +215,5 @@
> +                          function (update, state) {
> +                            switch(state) {
> +                            case STATE_FINISHED:
> +                              do_check_true(error_count[0] > 0,
> +                                            "This request should run out of the cache storage");

Do you think we can do error_count[0] == 1 ?

@@ +222,5 @@
> +
> +                            case STATE_ERROR:
> +                              error_count[0]++;
> +                              break;
> +                            }

Same for NOUPDATE and OBSOLETE here.
Comment 97 Honza Bambas (:mayhemer) 2012-03-27 18:07:21 PDT
Comment on attachment 609648 [details] [diff] [review]
Part 3: Evict cache asynchronized, v2

I'll review this later.  I also think we may not need this patch at all, since discarding may be removed when we have a second space-unlimited database.
Comment 98 Thinker Li [:sinker] 2012-03-28 02:57:14 PDT
Created attachment 610054 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v4

--
Attachment #609646 [details] [diff] - Attachment is obsolete: true
Comment 99 Thinker Li [:sinker] 2012-03-28 02:57:23 PDT
Created attachment 610055 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v8

--
Attachment #609647 [details] [diff] - Attachment is obsolete: true
Comment 100 Honza Bambas (:mayhemer) 2012-03-28 04:50:13 PDT
Comment on attachment 610054 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v4

Review of attachment 610054 [details] [diff] [review]:
-----------------------------------------------------------------

You've added a lot of white space only changes, please remove them before landing.
Comment 101 Honza Bambas (:mayhemer) 2012-03-28 04:51:19 PDT
Comment on attachment 610055 [details] [diff] [review]
Part 2: Testcase for pinned application caches, v8

Review of attachment 610055 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/unit/test_pinned_app_cache.js
@@ +180,5 @@
> + */
> +function start_cache_nonpinned_app() {
> +  do_print("Start non-pinned App1");
> +  start_and_watch_app_cache(kHttpLocation + "app1.appcache",
> +                          kHttpLocation + "app1",

Indention.
Comment 102 Honza Bambas (:mayhemer) 2012-03-28 04:52:57 PDT
Thinker, I think you can move the part 3 patch to a followup bug and just land these two with the last few nits and let this bug be closed.
Comment 103 Mozilla RelEng Bot 2012-03-28 07:46:40 PDT
Try run for cdbccb5c015c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cdbccb5c015c
Results (out of 221 total builds):
    exception: 2
    success: 183
    warnings: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-cdbccb5c015c
Comment 104 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-28 17:46:37 PDT
(In reply to Brian Smith (:bsmith) from comment #92)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #0)
> > (iii) ensuring those files are never evicted.
> > 
> > I suspect this would be pretty easy to implement in current Gecko, depending
> > on (i).
> 
> Although it seems simple to implement, there is a huge limitation: when
> Gecko crashes, the cache is reset. This makes the cache totally unusable as
> an app storage mechanism for mobile devices, AFAICT.
> 

Is the app cache cleared when gecko crashes?  That would be annoying, and we should fix it, but it's not a showstopper.  We can recover core cached apps from a pristine read-only package that will be stashed away on device, and we can recover web apps next time network is up.

> The Necko cache is optimized for avoiding disk I/O (avoiding fsyncs and the
> like), and is not optimized for persistence. This fundamental design choice
> is going to cause all kinds of problems if we try to rely on our cache as an
> app storage mechanism. There is not even a plan yet as to how to rectify
> it--i.e. it would be extremely optimistic to expect a solution in Q2 if
> things go according to current plans.

If this is true for app cache, then we very much need to fix it.  There is a fair amount of this kind of stuff in gecko we need to fix as well :/.
Comment 105 Mozilla RelEng Bot 2012-03-29 01:18:03 PDT
Try run for 75aa7826a464 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=75aa7826a464
Results (out of 219 total builds):
    exception: 22
    success: 151
    warnings: 30
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-75aa7826a464
Comment 106 Thinker Li [:sinker] 2012-03-29 03:14:39 PDT
Created attachment 610500 [details] [diff] [review]
Part 1: Pinned apps get higher priority for cache, v5

--
Attachment #610054 [details] [diff] - Attachment is obsolete: true
Comment 107 Mozilla RelEng Bot 2012-03-29 07:31:49 PDT
Try run for ff31dab19922 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ff31dab19922
Results (out of 221 total builds):
    exception: 3
    success: 197
    warnings: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tlee@mozilla.com-ff31dab19922
Comment 108 Honza Bambas (:mayhemer) 2012-03-29 15:52:34 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #104)

In general, Brian was describing the "normal" HTTP web content cache, not offline cache.

> Is the app cache cleared when gecko crashes?  

No.  Only the sqlite db could potentially be damaged, but I have to check the level of sync on the connection.

> If this is true for app cache, then we very much need to fix it.  There is a
> fair amount of this kind of stuff in gecko we need to fix as well :/.

I don't think it's true but some investigation should be taken to find out what may not be in favor with what we need for B2G.
Comment 110 Ryan VanderMeulen [:RyanVM] 2012-03-30 18:19:53 PDT
And a bustage fix since I didn't notice the double braces when un-bitrotting.
https://hg.mozilla.org/integration/mozilla-inbound/rev/db5dbc168429
Comment 111 Honza Bambas (:mayhemer) 2012-03-31 11:41:20 PDT
Thinker, see comment 102.

I've never R+'ed the part 3 patch.  Back it out ASAP!
Comment 112 Ryan VanderMeulen [:RyanVM] 2012-03-31 15:29:43 PDT
Part 3 backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee28936ed0ab
Comment 114 Ed Morley [:emorley] 2012-03-31 19:25:08 PDT
Let's try again. (This replaces comment 113)

Part 1:
https://hg.mozilla.org/mozilla-central/rev/cdd005fde96d

Part 2:
https://hg.mozilla.org/mozilla-central/rev/dd0291644c67

Bustage fix:
https://hg.mozilla.org/mozilla-central/rev/db5dbc168429

(And part 3 has been backed out since un-reviewed and will be moved to another bug)
Comment 115 Honza Bambas (:mayhemer) 2012-07-12 13:35:27 PDT
*** Bug 383014 has been marked as a duplicate of this bug. ***
Comment 116 Jason Smith [:jsmith] 2012-08-28 08:21:39 PDT
Is there anything testable in this feature from an end-user perspective?

Note You need to log in before you can comment on or make changes to this bug.