Closed Bug 913807 Opened 6 years ago Closed 6 years ago

Land HTTP cache v2 pref'ed off

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla27

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 3 open bugs, )

Details

(Whiteboard: [cache2])

Attachments

(15 files, 1 obsolete file)

225.93 KB, patch
michal
: review+
Details | Diff | Splinter Review
46.55 KB, patch
michal
: review+
ehsan
: review-
mfinkle
: review+
fabrice
: review+
Details | Diff | Splinter Review
203.16 KB, patch
michal
: review+
ehsan
: review+
markh
: review+
Details | Diff | Splinter Review
180.08 KB, patch
Details | Diff | Splinter Review
95.52 KB, patch
Details | Diff | Splinter Review
44.58 KB, patch
michal
: review-
Details | Diff | Splinter Review
8.93 KB, patch
michal
: review+
ehsan
: feedback+
Details | Diff | Splinter Review
1.72 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
4.06 KB, patch
michal
: review+
Details | Diff | Splinter Review
6.85 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
4.01 KB, patch
Details | Diff | Splinter Review
3.75 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
6.02 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
631.90 KB, patch
Details | Diff | Splinter Review
180.37 KB, patch
Details | Diff | Splinter Review
No description provided.
Depends on: 913815
Depends on: 913873
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
Attachment #802442 - Flags: review?(michal.novotny)
Need to find respective reviewers yet.
Attachment #802446 - Flags: review?(honzab.moz)
Attachment #802447 - Flags: review?(michal.novotny)
Attached patch part6: new testsSplinter Review
Attachment #802448 - Flags: review?(michal.novotny)
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.
Attachment #802446 - Flags: review?(honzab.moz)
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.
Attachment #802447 - Flags: review?(michal.novotny)
Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

Michal, can you please take a look at the uriloader/ changes?
Attachment #802443 - Flags: review?(michal.novotny)
Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Michal, and as well here, please check just the /network changes.
Attachment #802444 - Flags: review?(michal.novotny)
Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

Michal, and please also /image, /.../all.js, /toolkit/components, /toolkit/forgetaboutsite ?
Comment on attachment 802443 [details] [diff] [review]
part2: chrome changes

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

Thanks!
Attachment #802443 - Flags: review?(ehsan)
Comment on attachment 802444 [details] [diff] [review]
part3: existing test changes

Ehsan, here please check on /content and /dom changes.
Attachment #802444 - Flags: review?(ehsan)
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
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.
Attachment #802443 - Flags: review?(ehsan) → review-
(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 on attachment 802444 [details] [diff] [review]
part3: existing test changes

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

r=me on dom/ and content/ changes.
Attachment #802444 - Flags: review?(ehsan) → review+
(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.  :-)
(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.
- 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
Attachment #804343 - Flags: review?(michal.novotny)
Attachment #804343 - Flags: feedback?(ehsan)
Attachment #802443 - Flags: review?(mark.finkle)
Attachment #802443 - Flags: review?(fabrice)
Attachment #802444 - Flags: review?(mark.finkle)
Attachment #802444 - Flags: review?(fabrice)
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.
Attachment #802444 - Flags: review?(mark.finkle)
Attachment #802444 - Flags: review?(fabrice)
Fabrice, please look over changes in /b2g.
Mark, please look over changes in /mobile.

Thanks!
(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?)
Flags: needinfo?(ehsan)
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.
Attachment #802443 - Flags: review?(fabrice) → review+
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.
Attachment #804343 - Flags: feedback?(ehsan) → feedback+
(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.)
Flags: needinfo?(ehsan)
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?
Attachment #802443 - Flags: review?(mark.finkle) → review+
(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.
Blocks: 916484
Blocks: 916485
Blocks: 916750
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.
Attachment #802444 - Flags: review?(mhammond)
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
Attachment #802442 - Flags: review?(michal.novotny) → review+
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)
Attachment #802444 - Flags: review?(mhammond) → review+
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 on attachment 802443 [details] [diff] [review]
part2: chrome changes

r+ for changes in uriloader/, image, .../all.js, toolkit/components, toolkit/forgetaboutsite
Attachment #802443 - Flags: review?(michal.novotny) → review+
(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
- 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
Attachment #805945 - Flags: review?(michal.novotny)
(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.
(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.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Depends on: 917275
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
Attachment #805974 - Attachment description: review fix 5, v1, (fix Gavin Sharp's comment 33) → review fix 5, v1 (fix Gavin Sharp's comment 33)
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.
Attachment #802444 - Flags: review?(michal.novotny) → review+
Attachment #805945 - Flags: review?(michal.novotny) → review+
Attachment #804343 - Flags: review?(michal.novotny) → review+
(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.
Depends on: 917393
- 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
Attachment #806171 - Flags: review+
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.
Attachment #802448 - Flags: review?(michal.novotny) → review-
(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.
(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.
- 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+
Attachment #806629 - Flags: review?(michal.novotny)
Attachment #806629 - Flags: checkin-
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;
Attachment #806629 - Flags: review?(michal.novotny) → review+
Attachment #806629 - Attachment description: review fix 7, v1 (fix Michal's comment 46) → review fix 7, v2 (fix Michal's comment 46)
Attachment #806728 - Attachment is obsolete: true
Attachment #806629 - Attachment is obsolete: true
Attachment #806629 - Flags: checkin-
Attachment #806728 - Attachment description: review fix 7, v1 (fix Michal's comment 46+50) → review fix 7, v2 (fix Michal's comment 46+50)
Attachment #806728 - Attachment is obsolete: false
Attachment #806629 - Attachment description: review fix 7, v2 (fix Michal's comment 46) → review fix 7, v1 (fix Michal's comment 46)
Blocks: 895390
Try run from on the inbound base change set:
https://tbpl.mozilla.org/?tree=Try&rev=defcd6d2e9aa
Fixed accidental all.js bad merge change that slipped in through review (nothing serious):
https://hg.mozilla.org/integration/mozilla-inbound/rev/44952b2191fc
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
Blocks: 919087
http://mxr.mozilla.org/mozilla-central/source/netwerk/cache2/CacheStorageService.cpp#1043 make the build fail at line 1070, when MOZ_LOGGING not defiend.
Depends on: 919468
Depends on: 919723
No longer blocks: 919087
Depends on: 924112
Depends on: 920802
No longer depends on: 924112
Depends on: 925352
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.
Status: RESOLVED → VERIFIED
Depends on: 931383
Blocks: 975362
Depends on: 984994
Blocks: 1022154
Blocks: 1035411
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?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
Duplicate of this bug: 446876
blocking-b2g: 2.6? → ---
Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.