561313 - Cache asserts for two RW requests followed by a W request

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

Attachment: xpcshell-test triggering the issue

This is a spin-off from bug #548406, comment #282.

If the cache receives two read-write ("normal") requests followed by one force-write request (all requests for the same resource) we hit an assertion when trying to insert a new entry for the third request into the list of active entries.

Comment 1

[Bjarne (:bjarne)]

This problem comes from the fact that we call DoomEntry_Internal(|existing entry|) in ActivateEntry for the third (force-write) request. This in turn calls ProcessPendingRequests which ends up calling ActivateEntry again (a recursion, in other words) which results in a new entry added to mActiveEntries. Control eventually returns to the point where we called DoomEntry_Internal, and here we clear our |entry| and in the next step we create another new entry and try to insert it into mActiveEntries. Boom...

IMO, a fix for this could be related to a fix for bug #548406, especially if the latter includes keeping the existing entry somehow.

Comment 2

[Bjarne (:bjarne)]

Followup from bug #548406, comment #282.

> But I don't think the result must be deterministic in this case. The cache
> doesn't guarantee that you can always read back what you've written. And this
> is exactly the case. HTTP request #3 wants to bypass the cache (and the caller
> probably knows why he asked that) so a new content is expected and the cache
> should be updated. And in our case this means that HTTP request #2 won't use
> cache (which may be obsolete). This seems to me reasonable.

You're probably right. But there is still something bothering me about this which I cannot quite put my finger on...

If we allow HTTP request #3 to cancel cache-access for HTTP request #2 we swap the order of requests #2 and #3 as seen by the server. Problem?

Depending on timing between requests and speed of cache-device, request #2 will either be read from cache or fetched from the server. Problem?

> Also please note
> that your testcase is artificial test that IMO doesn't reflect real scenarios.

Sure, but doesn't that apply to many unittests? :) We establish some desired behavior for our software and create unittests to ensure isolated aspects of this behavior. (The interesting bit in this particular case, IMO, is that desired behavior is a little unclear.)

> Ah, OK. Anyway IMO the issue isn't limited to unbound entries. The test would
> assert even if the entry would be already bound.

True.

> > A thought-experiment : Three XMLHttpRequests access a resource like the test
> > does (first two requests are normal, third has a bypass-cache flag). Which
> > result-value would we expect for the second XMLHttpRequest?
> 
> I would expect the correct response from the server (i.e. options 1). If the
> server's response differs from the cache content then we return right response.
> If the response is the same then we just sent one inefficient request to the
> server.

Mmmmm...  that corresponds to changing the second channel-load in the xpcshell-test to expect "2" and not "1" . However, that would not be true if the third load was not present or was delayed. Problem (in practice)?

Comment 3

[Bjarne (:bjarne)]

Using the updated unit-test from bug #596443, comment #36 I believe this bug is fixed by the patch in bug #596443, comment #41.