758358 - Cache doesn't call listener for non-blocking async requests (LOAD_BYPASS_LOCAL_CACHE_IF_BUSY) off the main thread when the entry is busy (NS_ERROR_CACHE_WAIT_FOR_VALIDATION)

Status: RESOLVED DUPLICATE of bug 722034

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 615947 [details] [diff] [review] [diff] [review]
Cache changes necessary to build the previous patch

Bug 722034 comment 21:
> 1. Fixes NS_ERROR_CACHE_WAIT_FOR_VALIDATION processing for
> AsyncOpenCacheEntry when it is called on the cache I/O thread. In
> particular, we cannot have AsyncOpenCacheEntry return NS_OK unless it will
> later call the listener's OnCacheEntryAvailable method.

When exactly happen that we return NS_OK from AsyncOpenCacheEntry() and then don't call OnCacheEntryAvailable()? I'm not aware of this behavior. IMO your patch causes that we return NS_ERROR_CACHE_WAIT_FOR_VALIDATION in case of async non-blocking request that can't be satisfied and then we call the listener with the same result.


> +    static void AssertIsCacheIOThread()
> +    {
> +      MOZ_ASSERT(IsCacheIOThread());
> +    }

AFAICS this method is used only by HttpCacheQuery that lives outside cache directory where you should probably access cache service only via interface methods.

Bug 722034 comment 39:
> (In reply to Brian Smith (:bsmith) from comment #21)
> > 1. Fixes NS_ERROR_CACHE_WAIT_FOR_VALIDATION processing for
> > AsyncOpenCacheEntry when it is called on the cache I/O thread. In
> > particular, we cannot have AsyncOpenCacheEntry return NS_OK unless it will
> > later call the listener's OnCacheEntryAvailable method.
> 
> When exactly happen that we return NS_OK from AsyncOpenCacheEntry() and then
> don't call OnCacheEntryAvailable()? I'm not aware of this behavior. IMO your
> patch causes that we return NS_ERROR_CACHE_WAIT_FOR_VALIDATION in case of
> async non-blocking request that can't be satisfied and then we call the
> listener with the same result.

First, I am very open to the possibility that my patch isn't good here, because I don't understand the cache code well.

If I apply this entire patch series except for NS_ERROR_CACHE_WAIT_FOR_VALIDATION change (which is in Part 1, ctcc-1-v2), then the test netwerk\test\unit\test_reentrancy.js hangs. 

Bug 722034 comment 43:
> Comment on attachment 624123 [details] [diff] [review]
> Part 1 - Make AsyncOpenCacheEntry return NS_ERROR_CACHE_WAIT_FOR_VALIDATION
> 
> (In reply to Brian Smith (:bsmith) from comment #39)
> > > When exactly happen that we return NS_OK from AsyncOpenCacheEntry() and then
> > > don't call OnCacheEntryAvailable()? I'm not aware of this behavior. IMO your
> > > patch causes that we return NS_ERROR_CACHE_WAIT_FOR_VALIDATION in case of
> > > async non-blocking request that can't be satisfied and then we call the
> > > listener with the same result.
> > 
> > First, I am very open to the possibility that my patch isn't good here,
> > because I don't understand the cache code well.
> > 
> > If I apply this entire patch series except for
> > NS_ERROR_CACHE_WAIT_FOR_VALIDATION change (which is in Part 1, ctcc-1-v2),
> > then the test netwerk\test\unit\test_reentrancy.js hangs. 
> 
> There is really a problem that we don't call the listener in case of
> non-blocking async request called on a background thread when the cache
> entry is in use. But the correct solution would be to call the listener
> instead of returning NS_ERROR_CACHE_WAIT_FOR_VALIDATION from
> AsyncOpenCacheEntry(). At least this is the behavior described in
> nsICacheSession.idl. So the change should be IMO in
> nsCacheService::ProcessRequest()
> 
> -        if (NS_FAILED(rv) && calledFromOpenCacheEntry)
> +        if (NS_FAILED(rv) && calledFromOpenCacheEntry &&
> request->IsBlocking())

Replacing my patch with this one, several tests fail:

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit\test_nojsredir.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit\test_redirect-caching_failure.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit\test_redirect_failure.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit\test_XHR_redirects.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit_ipc\test_redirect-caching_failure_wrap.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit_ipc\test_redirect_failure_wrap.js | test failed (with xpcshell return code: 0)

TEST-UNEXPECTED-FAIL | c:\p\threading\ff-dbg\_tests\xpcshell\netwerk\test\unit_ipc\test_XHR_redirects.js | test failed (with xpcshell return code: 0)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Never mind, those errors seem to be caused by another mistake I made, not Michal's suggested patch.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This is fixed in patch 1 of bug 722034. But, see bug 759805 and bug 722034 comment 62.