Last Comment Bug 678595 - Remove content/base/test/test_bug482935.html assumption that cancel causes cache invalidation.
: Remove content/base/test/test_bug482935.html assumption that cancel causes ca...
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla9
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 668283
  Show dependency treegraph
 
Reported: 2011-08-12 12:54 PDT by Josh Matthews [:jdm] (away until 9/3)
Modified: 2011-09-03 03:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Removes test that assumes cancel in XHR state 3 means no cache entry (2.35 KB, patch)
2011-08-29 17:23 PDT, Jason Duell [:jduell] (needinfo me)
josh: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] (away until 9/3) 2011-08-12 12:54:25 PDT
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 [:jduell] (needinfo me) 2011-08-24 19:03:34 PDT
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 [:bz] 2011-08-26 18:54:51 PDT
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 [:jduell] (needinfo me) 2011-08-29 17:11:34 PDT
>  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 [:jduell] (needinfo me) 2011-08-29 17:23:01 PDT
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?
Comment 5 Josh Matthews [:jdm] (away until 9/3) 2011-08-29 17:37:39 PDT
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] (away until 9/3) 2011-08-29 17:38:59 PDT
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 [:bz] 2011-08-29 19:01:08 PDT
> 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 [:jduell] (needinfo me) 2011-08-30 12:16:18 PDT
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] (away until 9/3) 2011-08-30 12:23:14 PDT
My patch already makes the relevant modifications to this test, so removing this hunk should be all that's necessary.
Comment 10 Boris Zbarsky [:bz] 2011-08-31 21:36:07 PDT
Filed bug 683817.
Comment 11 Jason Duell [:jduell] (needinfo me) 2011-09-02 13:06:13 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ba720787775f
Comment 12 Marco Bonardo [::mak] 2011-09-03 03:00:46 PDT
http://hg.mozilla.org/mozilla-central/rev/ba720787775f

Note You need to log in before you can comment on or make changes to this bug.