Closed
Bug 758358
Opened 13 years ago
Closed 13 years ago
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)
Categories
(Core :: Networking: Cache, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 722034
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
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)
Assignee | ||
Comment 1•13 years ago
|
||
Never mind, those errors seem to be caused by another mistake I made, not Michal's suggested patch.
Assignee | ||
Comment 2•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•