917393 - HTTP cache v2: make disabled net xpchshell tests run on the old cache backend

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Some tests has been completely skip-if = true'ed.  They should however run on the old backend.  We have a runtime function that checks the new backend use that can be used.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

- skip-if = true where applicable changed to a run-time check inside respective tests

- test_bug812167.js:
  - runs on both new and old, since this one shows up to be important
  - old test check reverted to exactly the old testing code
  - new test check changed to reflect the new api behavior

- fixed SetPersistToDisk to throw after the entry has been used
  - here I'm thinking of not even allowing to set the flag back when ones dropped (by either call to SetPersistToDisk(false) or opening the entry using memoryCacheStorage)

Michal, what do you think? (not needed to decide now, this is for the new cache only..)

- fixed EvictionRunnable release of mCallback on the main thread


https://tbpl.mozilla.org/?tree=Try&rev=7007b4265212

Comment 2

[Honza Bambas (:mayhemer)]

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug651100.js | test failed (with xpcshell return code: 0), see following log:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug651100.js | 1024 == 0 - See following stack:

Going to investigate.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> TEST-UNEXPECTED-FAIL |
> /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/
> test_bug651100.js | test failed (with xpcshell return code: 0), see
> following log:
> TEST-UNEXPECTED-FAIL |
> /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/
> test_bug651100.js | 1024 == 0 - See following stack:
> 
> Going to investigate.

The problem is that I switched from .doom() to .asyncDoom() at one place of the test and the last entry is not moved out from the storage before checking the storage occupation using VisitDevice.  AsyncDoom behaves differently then Doom.  The problem is not in missing synchronization, there is sync with IO thread.  

Following lines in the NSPR log should be in good case (when using Doom) before visiting:

2013-09-18 15:38:02.588000 UTC - 0[36d758]: Deactivating entry 3c96188
2013-09-18 15:38:02.588000 UTC - 0[36d758]: CACHE: disk DeactivateEntry [3c96188 46f717d]
2013-09-18 15:38:02.588000 UTC - 5116[2c015b0]: nsDiskCacheDeviceDeactivateEntryEvent[3d57c50]
2013-09-18 15:38:02.588000 UTC - 5116[2c015b0]: CACHE: DeleteStorage [46f717d 0]
2013-09-18 15:38:02.588000 UTC - 5116[2c015b0]: CACHE: DeleteStorage [46f717d 1]

But when using AsyncDoom, these are happening after visiting.

It seems like AsyncDoom causes chain of dispatches to cache->main->cache->main thread and the finial check happens before the dooming is done.  Even a crazy code like:

  os.close();
  dump("doom\n");
  entry.asyncDoom({onCacheEntryDoomed: function() {
    dump("ondoom\n");
    do_execute_soon(function() {
      dump("exec\n");
      syncWithCacheIOThread(run_test_3);
    });
  }});
  dump("close\n");
  entry.close();  

doesn't help...

I'll just change the check for the size to <= 1024...

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1.2

Sorry to change the file under your hands.

- updated only to test_bug812167.js, I realized the code for the new cache can also be used for the old one ; so, the test is simplified

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 807123 [details] [diff] [review]
v1.2

https://tbpl.mozilla.org/?tree=Try&rev=0765ae234ffe

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 807123 [details] [diff] [review]
v1.2

(In reply to Honza Bambas (:mayhemer) from comment #3)
> It seems like AsyncDoom causes chain of dispatches to
> cache->main->cache->main thread and the finial check happens before the
> dooming is done.  Even a crazy code like:
> 
>   os.close();
>   dump("doom\n");
>   entry.asyncDoom({onCacheEntryDoomed: function() {
>     dump("ondoom\n");
>     do_execute_soon(function() {
>       dump("exec\n");
>       syncWithCacheIOThread(run_test_3);
>     });
>   }});
>   dump("close\n");
>   entry.close();  
> 
> doesn't help...
> 
> I'll just change the check for the size to <= 1024...

But something like this should work, right?

  entry.asyncDoom({onCacheEntryDoomed: function() {
    syncWithCacheIOThread(function() {
      syncWithCacheIOThread(run_test_3);
    });
  }});


> - fixed SetPersistToDisk to throw after the entry has been used
>   - here I'm thinking of not even allowing to set the flag back when ones
> dropped (by either call to SetPersistToDisk(false) or opening the entry
> using memoryCacheStorage)
> 
> Michal, what do you think? (not needed to decide now, this is for the new
> cache only..)

Makes sense. Is there any real scenario where we need to call SetPersistToDisk(true) on an entry created by memoryCacheStorage?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #7)

Thanks.

> But something like this should work, right?
> 
>   entry.asyncDoom({onCacheEntryDoomed: function() {
>     syncWithCacheIOThread(function() {
>       syncWithCacheIOThread(run_test_3);
>     });
>   }});

No, even that code won't help.  I even tried to iterate syncWCIOT 100 times before run_test_3 call...  It's strange, I don't know how the old cache work that in detail.

But I don't want to spend time on one limits-checking test that is badly written and will be fixed when we enable the new backend.

> Makes sense. Is there any real scenario where we need to call
> SetPersistToDisk(true) on an entry created by memoryCacheStorage?

Let's do it as a followup.  I don't think we should ever go from memory to disk.  Only some code could presume store to memory and later find out it may store to disk.

Worth a separate bug and more people involvement.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 807123 [details] [diff] [review]
v1.2

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