The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
Networking: HTTP
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla9
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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.
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.
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.
>  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.
Summary: content/base/test/test_bug482935.html is broken due to e10s HTTP interactions with the cache → Remove content/base/test/test_bug482935.html assumption that cancel causes cache invalidation.
Created attachment 556721 [details] [diff] [review]
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?
Attachment #556721 - Flags: review?(josh)
(Reporter)

Comment 5

6 years ago
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.
(Reporter)

Comment 6

6 years ago
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.
Attachment #556721 - Flags: review?(josh) → review+
> 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...
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.
(Reporter)

Comment 9

6 years ago
My patch already makes the relevant modifications to this test, so removing this hunk should be all that's necessary.
Filed bug 683817.
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba720787775f
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/ba720787775f
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.