913807 - Land HTTP cache v2 pref'ed off

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Honza Bambas (:mayhemer)]

I'm going to attach the patches to start reviews.

There are 6 patches, those are nothing more then split of the one large overall patch:
1. changes to /netwerk, core changes and additions needed to land the new code preffed off
2. changes to chrome (browser, mobile, b2g code)
3. changes to existing tests
4. new code written by michal
5. new code written by honza (modulo already added in part 1)
6. new tests

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: part1: changes to /network

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: part2: chrome changes

Need to find respective reviewers yet.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: part3: existing test changes

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: part4: new code (by Michal)

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: part5: new code (by Honza)

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: part6: new tests

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 802446 [details] [diff] [review]
part4: new code (by Michal)

Actually, there is no need to review this.  These are files that will not be triggered when the new cache is off.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 802447 [details] [diff] [review]
part5: new code (by Honza)

Actually, there is no need to review this.  These are files that will not be triggered when the new cache is off.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

Michal, can you please take a look at the uriloader/ changes?

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Michal, and as well here, please check just the /network changes.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

Michal, and please also /image, /.../all.js, /toolkit/components, /toolkit/forgetaboutsite ?

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

Ehsan, please check on /browser, /dom and /layout changes.

Thanks!

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Ehsan, here please check on /content and /dom changes.

Comment 15

[(no longer active)]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

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

r- because of the typo, mostly to make sure that this is properly tested (only looked at browser/, dom/ and layout/).

Also, please note that I did not actually review the usage of the new cache API, I just assumed that it is used correctly.  Please let me know if you want me to review that as well.

::: browser/components/preferences/advanced.js
@@ +321,5 @@
> +      },
> +      onCacheStorageInfo: function(num, consumption)
> +      {
> +        this.sum += consumption;
> +        if (!--this.expect)

This looks like a typo, did you mean this.expected?  How have you tested this code?

::: browser/components/preferences/in-content/advanced.js
@@ +303,5 @@
> +      },
> +      onCacheStorageInfo: function(num, consumption)
> +      {
> +        this.sum += consumption;
> +        if (!--this.expect)

Same here.

::: browser/installer/removed-files.in
@@ +1270,5 @@
>    components/mozfind.xpt
>    components/necko.xpt
>    components/necko_about.xpt
>    components/necko_cache.xpt
> +  components/necko_cache2.xpt

IIRC removed-files.in is not used any more, please check with rstrong.

::: layout/build/nsLayoutStatics.cpp
@@ +274,5 @@
>    nsCookieService::AppClearDataObserverInit();
>    nsApplicationCacheService::AppClearDataObserverInit();
>  
>    InitializeDateCacheCleaner();
> +  

Nit: trailing space.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > +        this.sum += consumption;
> > +        if (!--this.expect)
> 
> This looks like a typo, did you mean this.expected?  How have you tested
> this code?

Thanks Ehsan.  Yes, it's a typo.  I'll recheck the code ones again, I might be looking at a wrong place in the dialog that persuaded me it worked.

Comment 17

[(no longer active)]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

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

r=me on dom/ and content/ changes.

Comment 18

[(no longer active)]

(In reply to comment #16)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #15)
> > > +        this.sum += consumption;
> > > +        if (!--this.expect)
> > 
> > This looks like a typo, did you mean this.expected?  How have you tested
> > this code?
> 
> Thanks Ehsan.  Yes, it's a typo.  I'll recheck the code ones again, I might be
> looking at a wrong place in the dialog that persuaded me it worked.

Thanks!  If all you need to do to make this actually work is to fix the typo, you can consider my r- to be r+ with that fix.  Otherwise, I'll be happy to do another quick round.  :-)

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #18)
> If all you need to do to make this actually work is to fix the
> typo, you can consider my r- to be r+ with that fix.  Otherwise, I'll be
> happy to do another quick round.  :-)

Thanks you.  I'll retest this part more carefully this time.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: review fix 1, v1 (fix Ehsan's comments + fix old cache anonymous storage visit behavior)

- fix for :ehsan's comments (comment 17: typo, but also found an issue!)
- fixed behavior of old cache 'anonymous' disk storage: visiting 'anon' storage when the old back-end is used have to give 0 items since anon and non-anon items are stored all in the disk non-anon storage distinguished with the "anon&uri=" prefix

The main question here is whether we insist on the same behavior of the new API for the old and for the new cache exactly.  If so, we should then use a different session name for anonymous entries, but that means to actually break backward compatibility with the old cache.

Landed on gum as https://hg.mozilla.org/projects/gum/rev/41d977558e3d

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

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

Forgot there are no test changes for b2g and mobile.  Dropping r? flag.

Comment 22

[Honza Bambas (:mayhemer)]

Fabrice, please look over changes in /b2g.
Mark, please look over changes in /mobile.

Thanks!

Comment 23

[Honza Bambas (:mayhemer)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> Comment on attachment 802444 [details] [diff] [review]
> part3: existing test changes
> 
> Review of attachment 802444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on dom/ and content/ changes.

Ehsan, should I find someone else to take a look at the browser test changes?  (Who should that be?)

Comment 24

[[:fabrice] Fabrice Desré]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

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

r=me for b2g/* . I would just like to be able to tweak this value for devices that have more memory available.

Comment 25

[(no longer active)]

Comment on attachment 804343 [details] [diff] [review]
review fix 1, v1 (fix Ehsan's comments + fix old cache anonymous storage visit behavior)

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

The test changes look fine.

Comment 26

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #17)
> > Comment on attachment 802444 [details] [diff] [review]
> > part3: existing test changes
> > 
> > Review of attachment 802444 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me on dom/ and content/ changes.
> 
> Ehsan, should I find someone else to take a look at the browser test
> changes?  (Who should that be?)

See https://wiki.mozilla.org/Modules/Firefox.  I'd be happy to review those parts next week if you want (please set the review flag to me in that case.)

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

>diff --git a/mobile/android/app/mobile.js b/mobile/android/app/mobile.js

>+pref("browser.cache.memory_limit", 5120); // 5 MB
>+
>+

One blank line is enough. How did you pick 5MB? Just wondering what influences the choice since we have Android devices that have varying amounts of memory.

>diff --git a/mobile/android/modules/Sanitizer.jsm b/mobile/android/modules/Sanitizer.jsm

>     offlineApps: {
>       clear: function ()
>       {
>+        // TODO mayhemer: migrate to new cache api when implemete for appcache
>         var cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
>         try {
>           cacheService.evictEntries(Ci.nsICache.STORE_OFFLINE);
>         } catch(er) {}

You use appCacheStorage.asyncEvictStorage(null); in metro and browser. Why not here?

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Mark Finkle (:mfinkle) from comment #27)
> One blank line is enough. How did you pick 5MB? 

Absolutely from scratch :)  There is no science, this number was chosen just from top of my head.

> Just wondering what
> influences the choice since we have Android devices that have varying
> amounts of memory.

This is a completely new ground to explore.  Just a note that there is also hook to "memory-pressure" notification that completely purges the memory intermediate cache but leaves the limit intact.

If there is an API able to tell me how many bytes on the heap are free and how it changes at runtime, we can hook it.

Fabrice asked a more or less same thing.  I'll file bugs.

> You use appCacheStorage.asyncEvictStorage(null); in metro and browser. Why
> not here?

I'll fix it here too.

Thanks Mark!


(In reply to Fabrice Desré [:fabrice] from comment #24)
> I would just like to be able to tweak this value for
> devices that have more memory available.

As mentioned above, I'll file bugs for adjusting this at runtime.

Thanks for the reviews.

Comment 29

[Honza Bambas (:mayhemer)]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Mark, I'd like to ask for review of the /browser parts here.  Feel free to redirect to anyone else.  Thanks.

Comment 30

[Honza Bambas (:mayhemer)]

Attachment: review fix 2, v1 (fix Mark's comment)

Carrying r+.

Landed on gum as https://hg.mozilla.org/projects/gum/rev/ec07bfc3e301

Comment 31

[Michal Novotny [:michal]]

Comment on attachment 802442 [details] [diff] [review]
part1: changes to /network

The patch looks good to me with few nits:

> -    if (!startBuffering) {
> +    if (1) {
>          // We do not connect the stream to the stream transport service if we
>          // have to validate the entry with the server. If we did, we would get
>          // into a race condition between the stream transport service reading
>          // the existing contents and the opening of the cache entry's output
>          // stream to write the new contents in the case where we get a non-304
>          // response.

IIUC, we still need to keep this logic. Why did you skip the check?


> -    // Don't bother to validate items that are read-only,
> -    // unless they are read-only because of INHIBIT_CACHING or because
> +    // Don't bother to validate items that are
> +    // or because

fix the comment


> +        // We started to read cached data sooner then its write has been done.
> +        // But the concurrent write has not finished completely, so we had
> +        // do a range request.  Now let the content coming from the network

then -> than
had do -> had to do


>     AssertOnCacheThread();
> +    //AssertOnCacheThread();

remove this line

Comment 32

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

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

The /browser parts LGTM.  I'm not familiar enough with the other patches to know exactly how the "old" cache keeps test coverage while this is pref'ed off, but I'm happy to trust that this is being appropriately managed (and it's as much of a concern for the non /browser parts), so r=me.

::: browser/base/content/test/browser_save_private_link_perwindowpb.js
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +let tempScope = {};
> +Cu.import("resource://gre/modules/LoadContextInfo.jsm", tempScope);

Another way to write this is:

let {LoadContextInfo} = Cu.import("...", null);

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js
@@ +31,5 @@
>  const URL = "ftp://localhost/clearHistoryOnShutdown/";
>  
> +function createURI(urispec)
> +{
> +  var ioServ = Components.classes["@mozilla.org/network/io-service;1"]

Use Services.io.newURI

@@ +140,5 @@
>    return [ar[i] for (i in ar) if (ar.slice(0, i).indexOf(ar[i]) == -1)];
>  }
>  
>  function storeCache(aURL, aContent) {
> +  let cache = Cc["@mozilla.org/netwerk/cache-storage-service;1"].

Services.cache2

@@ +148,5 @@
>    var storeCacheListener = {
> +    onCacheEntryCheck: function (entry, appcache) {
> +      return nsICacheEntryOpenCallback.ENTRY_VALID;
> +    },
> +    

nit: trailing whitespace

@@ +167,5 @@
>        do_execute_soon(run_test_continue);
>      }
>    };
>  
> +  storage.asyncOpenURI(createURI(aURL), "", 

nit: trailing space

@@ +175,5 @@
>  
>  
>  function checkCache(aURL) {
> +  let cache = Cc["@mozilla.org/netwerk/cache-storage-service;1"].
> +              getService(Ci.nsICacheStorageService);

Services.cache2

@@ +185,5 @@
>        do_test_finished();
>      }
>    };
>  
> +  storage.asyncOpenURI(createURI(aURL), "", 

nit: trailing space

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cache.js
@@ +6,5 @@
>  // This test covers MozTrap test 6047
>  // bug 880621
>  
> +let tempScope = {};
> +Cu.import("resource://gre/modules/LoadContextInfo.jsm", tempScope);

I think this should either use the existing "tmp" scope var, or:
let {LoadContextInfo} = Cu.import("resource://gre/modules/LoadContextInfo.jsm", null);

@@ +24,5 @@
>  
>    sanitizeCache();
>  
> +  let nrEntriesR1 = getStorageEntryCount("regular", function(nrEntriesR1) {
> +    is (nrEntriesR1, 0, "Disk cache reports 0KB and has no entries");

might as well kill the space between "is" and the parens while we are touching this.

@@ +74,2 @@
>    var cs = get_cache_service();
> +  

trailing whitespace

@@ +91,4 @@
>      },
> +    onCacheEntryInfo: function(entry)
> +    {
> +      dump(device + ":" + entry.key + "\n");

the dump should probably go (or use info() if you feel it has value)

Comment 33

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

>diff --git a/browser/base/content/pageinfo/pageInfo.js b/browser/base/content/pageinfo/pageInfo.js

>+function createURI(urispec)
>+{
>+  var ioServ = Components.classes["@mozilla.org/network/io-service;1"]
>+                         .getService(Components.interfaces.nsIIOService);
>+  return ioServ.newURI(urispec, null, null);

Don't add this, use Services.io.newURI directly (you'll need to import Services.jsm).

>diff --git a/browser/components/preferences/advanced.js b/browser/components/preferences/advanced.js

>+    // TODO - migrate to the new cache API (see updateActualCacheSize above)

Followup bug filed? Is it really OK to land with this broken?

>diff --git a/browser/installer/removed-files.in b/browser/installer/removed-files.in

>+  components/necko_cache2.xpt

You don't need to add this (removed-files.in is for files that were removed from the appdir, some xpts are listed here because we used to ship individual XPTs but now ship a single combined one).

Comment 34

[Michal Novotny [:michal]]

Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

r+ for changes in uriloader/, image, .../all.js, toolkit/components, toolkit/forgetaboutsite

Comment 35

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #31)
> Comment on attachment 802442 [details] [diff] [review]
> part1: changes to /network
> 
> The patch looks good to me with few nits:

Thanks for the quick review!

> 
> > -    if (!startBuffering) {
> > +    if (1) {
> >          // We do not connect the stream to the stream transport service if we
> >          // have to validate the entry with the server. If we did, we would get
> >          // into a race condition between the stream transport service reading
> >          // the existing contents and the opening of the cache entry's output
> >          // stream to write the new contents in the case where we get a non-304
> >          // response.
> 
> IIUC, we still need to keep this logic. Why did you skip the check?

Good catch, this needs a slight fix.  This change is needed for the new cache, however.  The reason is following:

- input stream of the new cache is non-blocking (which is correct and different from the current cache)
- netwerk/test/unit/test_bug596443.js is an interesting test that very well exercises concurrent writes and reads
- it hits the code that is disabled by that change
- the cache input stream returns correctly WOULD_BLOCK when the stream transport tries to (pre-)read from it
- however, the stream given to nsIStreamTransportService.createInputTransport must be blocking [1], it's clear the stream transport really doesn't handle WOULD_BLOCK, [2]

I forgot to add a comment to the code and finalize the change correctly.

[1] http://hg.mozilla.org/mozilla-central/annotate/1d27c4c9871f/netwerk/base/public/nsIStreamTransportService.idl#l24
[2] http://hg.mozilla.org/mozilla-central/annotate/1d27c4c9871f/xpcom/io/nsStreamUtils.cpp#l530

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: review fix 3, v1 (fix Michal's comment 31, +35)

- fixed condition to not start pre-buffering of cached data when the new cache is used

Michal please check on this patch quickly.

Landed on gum as https://hg.mozilla.org/projects/gum/rev/e02f432e948e

Comment 37

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #31)
> >     AssertOnCacheThread();
> > +    //AssertOnCacheThread();
> 
> remove this line

I have some local patches that remove it.  Got confused that this was cleaned up already.  Will remove it in a separate patch.

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Mark Hammond (:markh) from comment #32)
> Comment on attachment 802444 [details] [diff] [review]
> part3: existing test changes
> 
> Review of attachment 802444 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The /browser parts LGTM.  I'm not familiar enough with the other patches to
> know exactly how the "old" cache keeps test coverage while this is pref'ed
> off, but I'm happy to trust that this is being appropriately managed (and
> it's as much of a concern for the non /browser parts), so r=me.

Thanks.  

The old API is still available, this patch should move all tests to the new API that in case of prefs set to use the old cache just wraps the old API.  It means we are still fully testing the old cache code with these changes.

Newly accidentally added trailing spaces will be removed by my cleanup script before landing.  These got there just by omitting to run it on the one individual patch.

Comment 39

[Honza Bambas (:mayhemer)]

Attachment: review fix 4, v1 (fix Mark Hammond's comment 32)

carrying r+, landed on gum as https://hg.mozilla.org/projects/gum/rev/434a5d4303d7

Comment 40

[Honza Bambas (:mayhemer)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #33)
> Comment on attachment 802443 [details] [diff] [review]
> part2: chrome changes

Thanks for the feedback, patch coming.

> >+    // TODO - migrate to the new cache API (see updateActualCacheSize above)
> 
> Followup bug filed? 

Actually, this should be taken care of as part of bug 913825.  There is absolutely no difference in using the new or old API to manage offline cache.  There is no new implementation for the offline cache as part of this work.  New API just wraps the old.

> Is it really OK to land with this broken?

It's not actually broken, it's just saving of resources ;)  No need to migrate this right now.  It's not even blocking turning the new cache on by default.

The comment may probably me removed, I bat there are more places that need to change when we move offline cache code to something else.  Marking one spot won't save us since overall audit will be needed anyway.

Comment 41

[Honza Bambas (:mayhemer)]

Comment on attachment 805956 [details] [diff] [review]
review fix 4, v1 (fix Mark Hammond's comment 32)

fixed use of newURI() at test_clearHistory_shutdown.js:
https://hg.mozilla.org/projects/gum/rev/fc374bf6a17e

Comment 42

[Honza Bambas (:mayhemer)]

Attachment: review fix 5, v1 (fix Gavin Sharp's comment 33)

Landed on gum as https://hg.mozilla.org/projects/gum/rev/08490bb7921f

Comment 43

[Michal Novotny [:michal]]

Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Some of the tests check specific bugs in the old cache code and there is no reason to run them with the new backend. E.g. test_bug651100.js, test_bug654926.js and probably others. It would be good to find all such tests and don't run them with the new backend. Those tests would be removed with the old cache in the future. Of course, this can be done in a follow up bug.


> +const CC = Components.Constructor;
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
> -const Cr = Components.results;
> -const Cu = Components.utils;
> -const CC = Components.Constructor;

remove also Cc and Ci


> -  [kCacheA2, kTestContent, kPrivate,       false],
> -  [kCacheB,  kTestContent, kDiskDevice,    true],
> -  [kCacheC,  kTestContent, kOfflineDevice, true]
> +  [kCacheA2, kTestContent, kDiskDevice,    false],
> +  [kCacheB,  kTestContent, kDiskDevice,    true]
> +  // TODO [kCacheC,  kTestContent, kOfflineDevice, true]

Why is kCacheC commented out? r+ with this fixed or with a better comment.

Comment 44

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #43)
> Comment on attachment 802444 [details] [diff] [review]
> part3: existing test changes
> 

Thanks.

> Some of the tests check specific bugs in the old cache code and there is no
> reason to run them with the new backend. E.g. test_bug651100.js,
> test_bug654926.js and probably others. It would be good to find all such
> tests and don't run them with the new backend. Those tests would be removed
> with the old cache in the future. Of course, this can be done in a follow up
> bug.

Personally I don't much agree.  Every test is good!  And I think some of them have helped to find some issue with the new cache.  At least help to be in parity.

> > +  // TODO [kCacheC,  kTestContent, kOfflineDevice, true]
> 
> Why is kCacheC commented out? r+ with this fixed or with a better comment.

I'll do one or other.

Comment 45

[Honza Bambas (:mayhemer)]

Attachment: review fix 6, v1 (fix Michal's comment 43)

- kCacheC | OFFLINE is re-enabled
- Ci,Cc not removed since this file doesn't include head_cache2

Landed on gum as https://hg.mozilla.org/projects/gum/rev/e601d4358434

Comment 46

[Michal Novotny [:michal]]

Comment on attachment 802448 [details] [diff] [review]
part6: new tests

> +    if (this.behavior & NOTVALID) {
> +      LOG_C2(this, "onCacheEntryCheck DONE, return ENTRY_WANTED");
> +      return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED;
> +    }

I think you want to return ENTRY_NOT_VALID here.


> +  asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> +    new OpenCallback(NEW|WAITFORWRITE, "a1m", "a1d", function(entry) {
> +      // Open for read and check
> +      do_check_eq(entry.dataSize, 3);
> +      asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> +        new OpenCallback(NORMAL|WAITFORWRITE, "a1m", "a1d", function(entry) {
> +          // Open for rewrite (truncate), write different meta and data
> +          do_check_eq(entry.dataSize, 3);
> +          asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_TRUNCATE, null,
> +            new OpenCallback(NEW|WAITFORWRITE, "a2m", "a2d", function(entry) {
> +              // Open for read and check
> +              do_check_eq(entry.dataSize, 3);
> +              asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> +                new OpenCallback(NORMAL|WAITFORWRITE, "a2m", "a2d", function(entry) {
> +                  do_check_eq(entry.dataSize, 3);
> +                  finish_cache2_test();

Is the WAITFORWRITE flag needed when openning the entry for reading?


> diff --git a/netwerk/test/unit/test_cache2-20-range-200.js b/netwerk/test/unit/test_cache2-20-range-200.js
> new file mode 100644
> --- /dev/null
> +++ b/netwerk/test/unit/test_cache2-20-range-200.js

This test doesn't test response 200 on partial entries. This is just an exact copy of test_cache2-16-conditional-200.js.

Comment 47

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #46)
> Comment on attachment 802448 [details] [diff] [review]
> part6: new tests
> 
> > +    if (this.behavior & NOTVALID) {
> > +      LOG_C2(this, "onCacheEntryCheck DONE, return ENTRY_WANTED");
> > +      return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED;
> > +    }
> 
> I think you want to return ENTRY_NOT_VALID here.

No.  First, there is no ENTRY_NOT_VALID any more.  Result just tells the cache back end what the consumer is going to do with the entry based on onCacheEntryCheck examination:

  const unsigned long ENTRY_WANTED = 0;
  const unsigned long ENTRY_NEEDS_REVALIDATION = 1;
  const unsigned long ENTRY_NOT_WANTED = 2;

Second, in the testing code NOTVALID just means to bypass the data checks since data provided to the callback are intended to rewrite a new entry.  However, after checking on the usage, this flag is now obsolete (the logic has changed during development) and can be complemented with the RECREATE flag.

I'll update the tests.

> 
> 
> > +  asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> > +    new OpenCallback(NEW|WAITFORWRITE, "a1m", "a1d", function(entry) {
> > +      // Open for read and check
> > +      do_check_eq(entry.dataSize, 3);
> > +      asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> > +        new OpenCallback(NORMAL|WAITFORWRITE, "a1m", "a1d", function(entry) {
> > +          // Open for rewrite (truncate), write different meta and data
> > +          do_check_eq(entry.dataSize, 3);
> > +          asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_TRUNCATE, null,
> > +            new OpenCallback(NEW|WAITFORWRITE, "a2m", "a2d", function(entry) {
> > +              // Open for read and check
> > +              do_check_eq(entry.dataSize, 3);
> > +              asyncOpenCacheEntry("http://a/", "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,
> > +                new OpenCallback(NORMAL|WAITFORWRITE, "a2m", "a2d", function(entry) {
> > +                  do_check_eq(entry.dataSize, 3);
> > +                  finish_cache2_test();
> 
> Is the WAITFORWRITE flag needed when openning the entry for reading?

No, probably a left over.  It makes sense only when NEW or RECREATE flags are specified.

> 
> 
> > diff --git a/netwerk/test/unit/test_cache2-20-range-200.js b/netwerk/test/unit/test_cache2-20-range-200.js
> > new file mode 100644
> > --- /dev/null
> > +++ b/netwerk/test/unit/test_cache2-20-range-200.js
> 
> This test doesn't test response 200 on partial entries. This is just an
> exact copy of test_cache2-16-conditional-200.js.

Ops...  OK, now I don't remember what I wanted this test to check on exactly...  I'll fix this.


Thanks for the review.

Comment 48

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #47)
> Second, in the testing code NOTVALID just means to bypass the data checks
> since data provided to the callback are intended to rewrite a new entry. 
> However, after checking on the usage, this flag is now obsolete (the logic
> has changed during development) and can be complemented with the RECREATE
> flag.

Taking back.  The flag is needed to distinguish when onCacheEntryCheck has to be called.


Otherwise 

  selfCheck: function()
  {
    ...
    do_check_true(this.onCheckPassed);


fails in some cases.

Comment 49

[Honza Bambas (:mayhemer)]

Attachment: review fix 7, v1 (fix Michal's comment 46)

- removed WAITFORWRITE where not needed
- test_cache2-20-range-200.js finished as it should be (similar to 19-range-206, just simulates 200 from the server)
- NOTVALID flag remains, it's actually needed

I'll land this after r+

Comment 50

[Michal Novotny [:michal]]

Comment on attachment 806629 [details] [diff] [review]
review fix 7, v1 (fix Michal's comment 46)

(In reply to Honza Bambas (:mayhemer) from comment #47)
> No.  First, there is no ENTRY_NOT_VALID any more.

OK, so remove/change the comment below.

> +// Return ENTRY_NOT_VALID from onCacheEntryCheck
> +const NOTVALID =        1 << 1;

Comment 51

[Honza Bambas (:mayhemer)]

Attachment: review fix 7, v2 (fix Michal's comment 46+50)

carrying r+, comment fixed.

Landed on gum as https://hg.mozilla.org/projects/gum/rev/695594c571e9

Comment 52

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=d712c9f1bda0 for https://hg.mozilla.org/projects/gum/rev/1c8b5fbb330f ("fix about_networking.js test after the merge")

Comment 53

[Honza Bambas (:mayhemer)]

Attachment: [as landed] part 1 of 2: API, top service, integration, tests

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c91d9aa9476

Comment 54

[Honza Bambas (:mayhemer)]

Attachment: [as landed] part 2 of 2: file I/O (by Michal Novotny)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d66cd80b6be

Comment 55

[Honza Bambas (:mayhemer)]

Try run from on the inbound base change set:
https://tbpl.mozilla.org/?tree=Try&rev=defcd6d2e9aa

Comment 56

[Honza Bambas (:mayhemer)]

Fixed accidental all.js bad merge change that slipped in through review (nothing serious):
https://hg.mozilla.org/integration/mozilla-inbound/rev/44952b2191fc

Comment 57

[Ed Morley [:emorley]]

Got merge conflicts with things that had landed on fx-team meanwhile, manually resolved:
https://hg.mozilla.org/integration/mozilla-inbound/diff/9ba8efe588db/netwerk/test/unit/test_about_networking.js

Then a few more changes needed post merge:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae09c5402cbe

Comment 58

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c91d9aa9476
https://hg.mozilla.org/mozilla-central/rev/4d66cd80b6be
https://hg.mozilla.org/mozilla-central/rev/44952b2191fc
https://hg.mozilla.org/mozilla-central/rev/ae09c5402cbe

Comment 59

[[e-mail]]

http://mxr.mozilla.org/mozilla-central/source/netwerk/cache2/CacheStorageService.cpp#1043 make the build fail at line 1070, when MOZ_LOGGING not defiend.

Comment 60

[Mihaela Velimiroviciu (:mihaelav)]

Build ID: 20131017030201

Verified that the new cache back end is pref'ed off by default (browser.cache.use_new_backend is 0) on Firefox 27.0a1 (current Nightly) builds across main platforms (Ubuntu 13.04, Win 7, Mac 10.8.4).
Marking verified.

Comment 61

[:Gijs (he/him)]

Is there a bug on file on fixing and re-enabling the FTP tests that were disabled here, and/or any example on how to fix them?

Comment 62

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #61)
> Is there a bug on file on fixing and re-enabling the FTP tests that were
> disabled here, and/or any example on how to fix them?

Bug 913827.

Comment 64

[alexjarabek]

[Tracking Requested - why for this release]: