Closed
Bug 758358
Opened 12 years ago
Closed 12 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•12 years ago
|
||
Never mind, those errors seem to be caused by another mistake I made, not Michal's suggested patch.
Assignee | ||
Comment 2•12 years ago
|
||
This is fixed in patch 1 of bug 722034. But, see bug 759805 and bug 722034 comment 62.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•