1203113 - Support deferred commit to HTTP cache.

Status: NEW

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

Description

[Honza Bambas (:mayhemer)]

Pinned resources should not be overwritten in the HTTP cache until said "from above".


API proposal:

- new interface nsIPinningCacheStorage derived from nsICacheStorage
- nsICacheStorageService.pinningCacheStorage (bug 1032254) will return implementation of that new inteface
- nsIPinningCacheStorage.defer() which defers write (commit) of all entries further open with this storage
- nsIPinningCacheStorage.commit() which commits the cache entries (overwrites the files)
- nsIPinningCacheStorage.rollback() which trows any pending entries away
- added nsICachingChannel.cacheStorage attribute to set a specific cache storage on a channel to open cache entries at (from)


Implementation (first though, unconfirmed how complicated it would be):

- a deferred entry would actually behave as a doomed entry (invisible to consumers of cache storage service, stored under doomed/NNN file)
- on commit an entry would be renamed to a regular one (any currently open handle would doom prior that), index would update
- on rollback we just close the handle and the automatic system we already have takes care of the 'doomed' file
- in CacheStorageService an entry will not be listed (nor registered) until committed



Problems:
1. new deferred resources cannot be streamed until committed (is it an issue actually?)
  - this is a non-issue for package resources since that coalesces access to //! resource urls
  - solution: commit/don't defer when index claims there is no entry for a URL automatically (won't work when index is not up to date, but that should rare and this is just an optimization anyway)

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> - added nsICachingChannel.cacheStorage attribute to set a specific cache
> storage on a channel to open cache entries at (from)

Would this mean that we have to know in advance if a particular URL is pinned or not? I.e. would this mean that we can't make normal browsing automatically use pinned resources if they are available?

If so that would be a problem.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #1)
> > - added nsICachingChannel.cacheStorage attribute to set a specific cache
> > storage on a channel to open cache entries at (from)
> 
> Would this mean that we have to know in advance if a particular URL is
> pinned or not? I.e. would this mean that we can't make normal browsing
> automatically use pinned resources if they are available?
> 
> If so that would be a problem.

No, we would use this _only_ during package updates for "download" channels.  Each channel will be armed the same pinning storage that for a channel is just a transparent cache storage to create a _new_ resource at.  We can then control that storage from above (defer and commit/rollback) to commit all resource in a "group" - aka "package" - atomically.

Schematically:

storage = cache.pinningStorage();
storage.defer();
channel[1].cacheStorage = storage;
channel[1].asyncOpen(..);
channel[2].cacheStorage = storage;
channel[2].asyncOpen(..);
...
// after all downloads and checks are done
storage.commit();


For normal loads we open cache entries as normal (as we do now, in the unpatched code).  Status of pinned/non-pinned is determined automatically by the cache for your convenience ;)

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> No, we would use this _only_ during package updates for "download" channels.

Ah, so we only need to set the .cacheStorage property on channels that we use to download an updated version of a previously pinned resource?

> storage = cache.pinningStorage();
> storage.defer();
> channel[1].cacheStorage = storage;
> channel[1].asyncOpen(..);
> channel[2].cacheStorage = storage;
> channel[2].asyncOpen(..);
> ...
> // after all downloads and checks are done
> storage.commit();

Does the .defer() call prevent updating for *all* pinned resources? And the .commit() commit downloaded updates to *all* resources? If so, it would be very tricky to juggle when we download and commit updates. 

We would have to download the updates to website 1. But the risk is that the user update website 1 before the download finishes. That would prevent us from starting to download updates to pinned website 2 since then we'd have to worry about website 1 *or* website 2 being open before we can commit.

Or would we be able to call `storage = cache.pinningStorage();` multiple times to get multiple separate storage areas that can be committed independently?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) from comment #3)
> > No, we would use this _only_ during package updates for "download" channels.
> 
> Ah, so we only need to set the .cacheStorage property on channels that we
> use to download an updated version of a previously pinned resource?

Exactly.

> 
> > storage = cache.pinningStorage();

This line always creates a new instance, with its own list of deferred entries.

> > storage.defer();
> > channel[1].cacheStorage = storage;
> > channel[1].asyncOpen(..);
> > channel[2].cacheStorage = storage;
> > channel[2].asyncOpen(..);
> > ...
> > // after all downloads and checks are done
> > storage.commit();
> 
> Does the .defer() call prevent updating for *all* pinned resources?

Based on the above, no.  Just those open with this cache storage.

> And the
> .commit() commit downloaded updates to *all* resources? 

As above, just those created by this cache storage.

> If so, it would be
> very tricky to juggle when we download and commit updates. 
> 
> We would have to download the updates to website 1. But the risk is that the
> user update website 1 before the download finishes. That would prevent us
> from starting to download updates to pinned website 2 since then we'd have
> to worry about website 1 *or* website 2 being open before we can commit.
> 
> Or would we be able to call `storage = cache.pinningStorage();` multiple
> times to get multiple separate storage areas that can be committed
> independently?

Exactly!

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Awesome, that sounds great!

Comment 6

[Honza Bambas (:mayhemer)]

AFAIK, Valentin is offline, so I'll start working on this.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: wip1

(backup, never even built)

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: wip2

(builds :))

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1

First patch passing network tests.  

- The defer information goes down to handles.  
- A deferred handle's file is created under the "doomed" dir so that it it's deleted after crash automatically.  
- A deferred cache entry is not put to the storage service master tables, nor registered
- Deferred entries carry a special "storage id" (aka context id) with added '!deferred:%d;' tag+value in front - the hash is specific per transaction id, allows the file be found when the handle is closed by limits
- On commit the storage id is replaced and entry added to master hashtables, hash and key on handles is updated and any original entries/handles are doomed.  
- Added a new simple test for basic exercise of this new code (maybe see it first as a demonstration of how this works).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71fbec2caa76

Comment 10

[Honza Bambas (:mayhemer)]

Attachment: v1.1

(I assume you haven't yet started with the review)

- fixed build issues plus few small fixes
- added a rollback test as well

https://treeherder.mozilla.org/#/jobs?repo=try&revision=36711ff74344

Comment 11

[Honza Bambas (:mayhemer)]

osx build fix

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81996117e59a

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1.2

few more fixes:
- key in metadata is also updated during commit of the cache file
- CacheFileHandle::mFile only swapped for a new name when the file is not yet physically present on disk
- added few more tests, also for real persistence

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a9f849cd8f5

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.3

- same as v1.3
- added the nsICachingChannel.cacheStorage attribute

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 8670230 [details] [diff] [review]
v1.3

Going to update once more.

Comment 15

[Honza Bambas (:mayhemer)]

Attachment: v1.4

same as v1.3, plus:
- AsyncVisitStorage implemented
- correctly implemented doom callback
- the hash table of deferred entries in PinningCacheStorage keeps ref directly to CacheEntry, not CacheEntryHandle (there was no need to)
- added comments to the new IDL

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8670714 [details] [diff] [review]
v1.4

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43775dbf7eda

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Honza, I'm getting an odd assertion when I create a pinned entry by calling
> mPinnedStorage->OpenTruncate(aURI, EmptyCString(),
>                       getter_AddRefs(writer->mEntry));
then open the output stream of that entry:
> rv = mEntry->OpenOutputStream(0, getter_AddRefs(mOutputStream));

This is the stack trace, with a few printfs I added for debugging:
https://pastebin.mozilla.org/8848679

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #16)
> Comment on attachment 8670714 [details] [diff] [review]
> v1.4
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=43775dbf7eda

Please be aware of:


TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_cache2-31a-deferred-commit.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | netwerk/test/unit/test_cache2-31a-deferred-commit.js | run_test/mc</mc</<.onCacheStorageCommitted - [run_test/mc</mc</<.onCacheStorageCommitted : 25] 2152857614 == 0
Return code: 1 

2152857614 = 8052000E = NS_ERROR_FILE_IS_LOCKED

I may need some advice from Michal on this (I might screwed up his code)

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #17)
> Honza, I'm getting an odd assertion when I create a pinned entry by calling
> > mPinnedStorage->OpenTruncate(aURI, EmptyCString(),
> >                       getter_AddRefs(writer->mEntry));
> then open the output stream of that entry:
> > rv = mEntry->OpenOutputStream(0, getter_AddRefs(mOutputStream));
> 
> This is the stack trace, with a few printfs I added for debugging:
> https://pastebin.mozilla.org/8848679

I need the changeset or at least what patches you have applied.  Lines are different in my local sources.

Comment 20

[Honza Bambas (:mayhemer)]

Also let you know that you need to have a profile when you run the test and want to pin in the cache (do_get_profile()).

This line proves you don't:
WARNING: Forcing memory-only entry since CacheFileIOManager doesn't have mCacheDirectory.: file /home/icecold/mozilla-central/netwerk/cache2/CacheFile.cpp, line 557"

Anyway, we probably should not assert when not.

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #20)
> Also let you know that you need to have a profile when you run the test and
> want to pin in the cache (do_get_profile()).

Ooops, I missed that. Indeed, it works if I add do_get_profile().
Thanks!

Comment 22

[Honza Bambas (:mayhemer)]

Valentin, I'm still interested in a way you could reproduce the assertion.  We should not assert when there is no profile, we should just cleanly fail.

Can you provide STR?

Comment 23

[Valentin Gosu [:valentin] (he/him)]

Apply patches:
1032254-pinning-global.patch
1203113-deferred-commit.patch
1203113-rej.patch // This is just a rebase on m-c.
bug1190290-update-apps.patch

Then run ./mach test netwerk/test/unit/test_packaged_app_signed_update.js

Comment 24

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #23)
> Apply patches:
> 1032254-pinning-global.patch
> 1203113-deferred-commit.patch
> 1203113-rej.patch // This is just a rebase on m-c.
> bug1190290-update-apps.patch
> 
> Then run ./mach test netwerk/test/unit/test_packaged_app_signed_update.js

Valentin, thanks, 'fixed' in bug 1032254.

Comment 25

[Honza Bambas (:mayhemer)]

Attachment: v1.4 -> v1.5 idiff

- only removes the mPinCacheContent from nsHttpChannel (no longer used with v3.3 patch for cache pinning)

Comment 26

[Honza Bambas (:mayhemer)]

Attachment: v1.5

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c5f677ef782

Comment 27

[Honza Bambas (:mayhemer)]

Attachment: v1.5 -> v1.6 idiff

This is a fix for the test failure.  Apparently, I'm forgetting to close the NSPR handle before rename (if it's currently open).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47bd2401bf1a

Comment 28

[Honza Bambas (:mayhemer)]

Attachment: v1.6

Comment 29

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb9d7470a4ff

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Valentin Gosu [:valentin] from comment #29)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb9d7470a4ff

Why at this bug?

Comment 31

[Valentin Gosu [:valentin] (he/him)]

(In reply to Honza Bambas, off till Nov 19 (:mayhemer) from comment #30)
> (In reply to Valentin Gosu [:valentin] from comment #29)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb9d7470a4ff
> 
> Why at this bug?

I was just experimenting with the pinning/deferred patches for packaged apps.
It seems that after I call asyncEvictStorage, the cache entry is still there, but inputStream.available() returns 0. Is this right?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dbb9c549575

Comment 32

[Valentin Gosu [:valentin] (he/him)]

(In reply to Valentin Gosu [:valentin] from comment #31)
> It seems that after I call asyncEvictStorage, the cache entry is still
> there, but inputStream.available() returns 0. Is this right?

Nevermind. There was a bug in my test.

Comment 33

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 34

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258