678595 - Remove content/base/test/test_bug482935.html assumption that cancel causes cache invalidation.

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Josh Matthews [:jdm]]

The test cancels an XHR with readyState==3, but the cancellation message only reaches the parent after OnStopRequest has already been sent, so the cache entry is not doomed as it should be.

Comment 1

[Jason Duell]

My gut sense here is that we should change the test.  There's nothing in the XHR spec that mandates that a response can't be cached even if it's canceled.

cc-ing Jonas and bz to see if they agree.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah, I think that's correct.  Of course nothing in the XHR spec mandates any caching or not caching anywhere.

And I've been thinking we should consider changing the "drop from cache if canceled before done" behavior in Necko anyway, so we shouldn't be testing for it, imo.

Comment 3

[Jason Duell]

>  we should consider changing the "drop from cache if canceled before done" behavior in Necko anyway

We did already in bug 482935, right?  This mochitest is from that bug, but was making too strong of an assumption about which states must still drop the cache entry.

Comment 4

[Jason Duell]

Attachment: Removes test that assumes cancel in XHR state 3 means no cache entry

jdm:  so the code removal seems pretty straightforward, and it runs OK in firefox, but on fennec I see both the current AND my patched version of the test failing with NS_ERROR_XPC_GS_RETURNED_FAILURE at "clearCache :: line 16). (I'd post the whole msg, but cut-and-paste doesn't seem to work in linux-fennec).

I know you've been monkeying around with e10s mochitests--is this something you've seen before?  I assume it may be because the e10s process can't simply grab Components.interfaces.nsICacheService and call evictEntries (though I don't see a function called clearCache() anywhere in the HTTP cache, so I'm not sure where that name is coming from).   

How were you testing this?  I test e10s mochitests by building a fennec build and then running them against it.

Finally, if/when we get this working, I assume it's disabled currently somehow for e10s at least?  Where's the knob to turn it on again?

Comment 5

[Josh Matthews [:jdm]]

Yeah, clearCache is a function in that file that tries to call evictEntries on the cache service. In my local series of patches I've added a clearCache function to SpecialPowers that sends a message to the parent process to do the same, and the rest of the test has been passing with that. I don't really know where the knobs are, though, I just do the work to make it so they can be twisted at some point.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 556721 [details] [diff] [review]
Removes test that assumes cancel in XHR state 3 means no cache entry

I'll give this a test locally at some point to confirm, but this looks like it should be fine.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> We did already in bug 482935, right?

No.  That bug made canceling from OnStopRequest not drop the cache entry.  Canceling from earlier can still drop it, depending.

I'm talking about a further change, to make the cache-dropping heuristic not depend on the race between the Cancel() and OnStopRequest; nothing wrong with keeping partial content...

Comment 8

[Jason Duell]

bz: file a new bug and make it block 559729?

jdm:  so does this patch need to change anything to work with your SpecialPowers patch?  We want one version of the code that clears the cache whether it's called from the child/parent.

Comment 9

[Josh Matthews [:jdm]]

My patch already makes the relevant modifications to this test, so removing this hunk should be all that's necessary.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Filed bug 683817.

Comment 11

[Jason Duell]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ba720787775f

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/ba720787775f