Last Comment Bug 482935 - Cancel() from OnStopRequest should not cause cache entry to be doomed
: Cancel() from OnStopRequest should not cause cache entry to be doomed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla5
Assigned To: Bjarne (:bjarne)
:
Mentors:
: 683817 (view as bug list)
Depends on: 552651 648485
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-12 04:03 PDT by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2016-02-09 12:41 PST (History)
8 users (show)
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix


Attachments
V1.0 with mochitest (6.82 KB, patch)
2009-03-25 05:13 PDT, Bjarne (:bjarne)
no flags Details | Diff | Review
Includes proper heading, otherwise unchanged (6.94 KB, patch)
2009-03-25 05:18 PDT, Bjarne (:bjarne)
no flags Details | Diff | Review
Unbitrot + test moved [Backout: Comment 15] (6.63 KB, patch)
2009-08-01 09:34 PDT, Bjarne (:bjarne)
jduell.mcbugs: review+
Details | Diff | Review
Minimal testcase demonstrating leak (1.98 KB, patch)
2009-08-22 13:11 PDT, Bjarne (:bjarne)
no flags Details | Diff | Review
Unbitrotted and slightly cleaned up minimal test which causes leak (2.02 KB, patch)
2010-03-11 04:46 PST, Bjarne (:bjarne)
no flags Details | Diff | Review
Code-fix and test which was comitted and backed out (6.66 KB, patch)
2010-03-16 06:18 PDT, Bjarne (:bjarne)
no flags Details | Diff | Review
Updated according to comments from reviewer (6.60 KB, patch)
2010-03-19 08:14 PDT, Bjarne (:bjarne)
dwitte: review+
Details | Diff | Review
(Av1e) Allow content to be cached if it was loaded successfully [Backed out: Comment 47] (6.61 KB, patch)
2010-11-08 17:34 PST, Serge Gautherie (:sgautherie)
mbeltzner: approval2.0-
Details | Diff | Review
(Av1f) Removed chrome:// urls from test-header - otherwise unchanged [Backed out: Comment 53] (6.54 KB, patch)
2010-11-10 15:41 PST, Bjarne (:bjarne)
no flags Details | Diff | Review
Unit-test not calling Cancel() but still failing intermittently... (5.04 KB, patch)
2010-11-12 07:56 PST, Bjarne (:bjarne)
no flags Details | Diff | Review
Modified test slightly, unchanged otherwise [Checked in: Comment 67] (6.23 KB, patch)
2010-11-13 04:06 PST, Bjarne (:bjarne)
jduell.mcbugs: review+
Details | Diff | Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2009-03-12 04:03:47 PDT
Calling cancel() from inside OnStopRequest currently causes the cache entry to be doomed.

This is unfortunate because we actually have all the data. I'm not sure what the best fix is, but the current behaviour is unfortunate.

In particular this is a problem when an onreadystatechange callback of an XMLHttpRequest calls cancel() on the XHR.
Comment 1 Bjarne (:bjarne) 2009-03-12 15:12:11 PDT
(In reply to comment #0)
> In particular this is a problem when an onreadystatechange callback of an
> XMLHttpRequest calls cancel() on the XHR.

I guess you mean abort()?

So, if I load a resource using XHR and call abort() in an onreadystatechange callback when state is > 1, the response should be cached? I.e. if I load the resource again, I should get it from cache?
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2009-03-13 07:32:50 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > In particular this is a problem when an onreadystatechange callback of an
> > XMLHttpRequest calls cancel() on the XHR.
> 
> I guess you mean abort()?

Yeah, sorry. That API is too high-level for me ;)

> So, if I load a resource using XHR and call abort() in an onreadystatechange
> callback when state is > 1, the response should be cached? I.e. if I load the
> resource again, I should get it from cache?

I meant when state=4 (I think, whatever the "complete" state is).
Comment 3 Bjarne (:bjarne) 2009-03-25 05:13:48 PDT
Created attachment 369254 [details] [diff] [review]
V1.0 with mochitest

This is a simple approach to avoid dooming a cache-entry if the request finished loading before it was canceled.

The test also verifies that the response is NOT cached when request is canceled before reaching state=4 (complete).
Comment 4 Bjarne (:bjarne) 2009-03-25 05:18:21 PDT
Created attachment 369257 [details] [diff] [review]
Includes proper heading, otherwise unchanged
Comment 5 Bjarne (:bjarne) 2009-04-12 11:52:15 PDT
Any indication of situations not handled by the patch would be appreciated...
Comment 6 Bjarne (:bjarne) 2009-05-06 14:28:11 PDT
Comment on attachment 369257 [details] [diff] [review]
Includes proper heading, otherwise unchanged

Trying new reviewer...
Comment 7 Jason Duell [:jduell] (needinfo? me) 2009-07-22 16:39:55 PDT
Comment on attachment 369257 [details] [diff] [review]
Includes proper heading, otherwise unchanged

Bjarne,

The actual code fix looks simple and straightforward.  I'm less clear about what's going on with the changes for testing.  Is this a mochitest?  We currently use only xpcshell unit tests in necko, and I'm just wondering why you need so many changes to the test Makefile (most of which I don't understand :)
Comment 8 Bjarne (:bjarne) 2009-07-23 00:56:43 PDT
It's a mochitest, yes. Makefile is changed to get the mochitest installed for necko because, as you point out, only xpcshell-tests are used currently :)

It's been some time since I did this and I'm not 100% sure why I took on the trouble of changing the Makefile in order to use a mochitest, but I'm pretty sure it was for a reason. If you are uncomfortable with the setup I can try to re-do the test in xpcshell (during which I'll probably remember the reason for doing the mochitest in the first place).
Comment 9 Jason Duell [:jduell] (needinfo? me) 2009-07-23 11:39:48 PDT
Necko is supposed to be buildable without the rest of the browser, so we can't currently add mochitests to the test Makefile.in without at least disabling them when necko is built standalone (I don't know how to detect a standalone build:  hopefully there's a way to tell within the Makefile).

So:  you can either look into whether your test could be written as an xpcshell test (I suspect it could: it doesn't look like it needs the rest of the browser), or you can figure out how to disable mochitests when necko is build standalone.

If you choose the latter, please make sure to make it clear how to add additional mochitests.  From my quick parse of the Makefile.in, it looks like any additional mochitests could be added by adding them to _TEST_FILES.  If that's right, we should at least add a comment there saying "# Mochitests" or something like that.

Sorry to make this harder than it ideally ought to be...
Comment 10 Jason Duell [:jduell] (needinfo? me) 2009-07-24 16:29:04 PDT
Update:  I talked to biesi about this.  He'd suspects that you may need a mochitest after all, since XMLHttpReq is not a necko feature.  But don't put the mochitest in /netwerk:  stick it in content/base somewhere (where we've already got existing mochitests and don't need to worry about creating a necko dependency).
Comment 11 Bjarne (:bjarne) 2009-08-01 09:34:33 PDT
Created attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]

Test moved to content/base, otherwise unchanged.
Comment 12 Bjarne (:bjarne) 2009-08-19 14:26:04 PDT
Requesting check-in..
Comment 13 Serge Gautherie (:sgautherie) 2009-08-20 09:01:26 PDT
Comment on attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]


http://hg.mozilla.org/mozilla-central/rev/3a829715fd39
Comment 14 Serge Gautherie (:sgautherie) 2009-08-20 09:17:06 PDT
Comment on attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]

(In reply to comment #13)
> http://hg.mozilla.org/mozilla-central/rev/3a829715fd39

After fixing context for
{
patching file content/base/test/Makefile.in
Hunk #1 FAILED at 316
1 out of 1 hunks FAILED
}
and reordering the 2 lines.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-08-20 12:25:36 PDT
I backed this out because there were reliable leaks on the mochitest-plain test, and this seemed like the most likely reason for them:
http://hg.mozilla.org/mozilla-central/rev/36d3597620c7
http://hg.mozilla.org/mozilla-central/rev/83c2882b42f2

We'll seen in a few hours if it was in fact the cause, although I suspect it was.
Comment 16 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-08-20 13:21:09 PDT
Backing this out did fix the leaks.
Comment 17 Serge Gautherie (:sgautherie) 2009-08-20 15:01:55 PDT
Bjarne, I don't know how you tested you patch ... Anyway, next time, have it run through TryServer.
Comment 18 Bjarne (:bjarne) 2009-08-21 05:58:18 PDT
Weird...  truly sorry for the confusion!

I spent some time trying to identify the leaking mochitest locally, but with no particular success...  just need to figure out what happened to my LDAP access, then I'll push it to the try-server.

(If there are usable logs from the failing runs, I'd be grateful for a link...)
Comment 19 Serge Gautherie (:sgautherie) 2009-08-21 06:16:03 PDT
(In reply to comment #18)

> I spent some time trying to identify the leaking mochitest locally, but with no
> particular success...

Does your fix+test leak?
Does the test suite leak with your fix but without your test?
...

> just need to figure out what happened to my LDAP access,
> then I'll push it to the try-server.

Try is to check before checkin, but won't help finding/fixing this leak.

> (If there are usable logs from the failing runs, I'd be grateful for a link...)

See http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
Comment 20 Serge Gautherie (:sgautherie) 2009-08-21 07:20:15 PDT
While helping Bjarne on irc, I noticed:
{
Leaked URLs:
  [...]
  http://localhost:8888/tests/content/base/test/test_bug482935.html
  http://localhost:8888/tests/content/base/test/bug482935.sjs
  http://localhost:8888/tests/content/base/test/test_bug482935.html
  http://localhost:8888/tests/content/base/test/bug482935.sjs
}

Yet he said he doesn't reproduce the leak locally :-/
Comment 21 Bjarne (:bjarne) 2009-08-21 08:42:13 PDT
Ahh - thanks. I haven't been paying attention to the console-output since I'm normally running with full debug and lots of output. But you're right - the leak can be reproduced locally.
Comment 22 Serge Gautherie (:sgautherie) 2009-08-21 09:23:43 PDT
Comment on attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]

>+    netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");

Nit: I don't think this is needed, is it?
Comment 23 Bjarne (:bjarne) 2009-08-22 04:23:39 PDT
(In reply to comment #22)
> (From update of attachment 392097 [details] [diff] [review])
> >+    netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");
> 
> Nit: I don't think this is needed, is it?

Probably not...

A quick test indicates that it is in fact the test which leaks. I.e. the test both fails and leaks with code-fix disabled.
Comment 24 Serge Gautherie (:sgautherie) 2009-08-22 08:47:45 PDT
(In reply to comment #23)
> A quick test indicates that it is in fact the test which leaks. I.e. the test
> both fails and leaks with code-fix disabled.

The test fails without the fix: "good, it works".
The test leaks with/without the fix: not good, but cause should be "easier" to find and fix...
Comment 25 Bjarne (:bjarne) 2009-08-22 13:02:17 PDT
The test wouldn't be much of a test if it passed without the fix. :) It is the leak which is interesting - I'll attach a minimal testcase showing the leak...
Comment 26 Bjarne (:bjarne) 2009-08-22 13:11:36 PDT
Created attachment 396080 [details] [diff] [review]
Minimal testcase demonstrating leak

This is a simple mochitest which leaks. Note that it only leaks when the request is aborted in state 3...
Comment 27 Serge Gautherie (:sgautherie) 2009-08-22 14:59:26 PDT
Comment on attachment 396080 [details] [diff] [review]
Minimal testcase demonstrating leak

>+  var xhr = new XMLHttpRequest();
>+  xhr.addEventListener("readystatechange", function(e) {
>+    if (xhr.readyState == 3) // NOTE : only leak for state == 3
>+      xhr.abort();
>+  }, false);
>+
>+  xhr.open("GET", "cancelXHR.sjs", true);

Fwiw,

https://developer.mozilla.org/En/XMLHttpRequest
"onreadystatechange: You need to call open() before setting this attribute."

https://developer.mozilla.org/En/Using_XMLHttpRequest
"req.addEventListener(): You need to add the event listeners before calling open() on the request."
https://developer.mozilla.org/En/nsIXMLHttpRequestEventTarget
"abort, error, load, loadstart, progress"

http://www.mail-archive.com/public-webapi@w3.org/msg00492.html
"XMLHttpRequest readystatechange events"
makes me wonder if mdc is not up-to-date or if this code is mixing two different things?
(or is it documented somewhere else?)

Ftr, there is at least 2 other content tests doing this:
http://mxr.mozilla.org/mozilla-central/search?string=addEventListener%28%22readystatechange%22&case=on

***

Other than that, my (first) idea was to wonder if something like executeSoon(xhr.abort) would help?
Comment 28 Bjarne (:bjarne) 2010-03-11 04:46:30 PST
Created attachment 431838 [details] [diff] [review]
Unbitrotted and slightly cleaned up minimal test which causes leak

Addressed sgautheries comments. Canceling a XHR in state 3 (in progress) still leaks, apparently...
Comment 29 Serge Gautherie (:sgautherie) 2010-03-11 09:08:41 PST
(In reply to comment #28)
> Addressed sgautheries comments.

You moved open() before addEventListener()...
But the documentation still seems contradictory: could that be answered/fixed first?

> Canceling a XHR in state 3 (in progress) still leaks, apparently...

You mean this test is triggering a backend bug?
Is it filed yet? We probably want to fix it first!
Comment 30 Bjarne (:bjarne) 2010-03-12 01:44:47 PST
(In reply to comment #29)
> But the documentation still seems contradictory: could that be answered/fixed
> first?

I'm not the right guy to deal with such matters, sorry...

> You mean this test is triggering a backend bug?

Not sure what you mean by "backend" but : Yes, this test (which does nothing, really) triggers a reproducible leak on trunk.

> Is it filed yet?

I have not filed anything, no. I can do that if you find it appropriate.

> We probably want to fix it first!

Agreed.
Comment 31 Bjarne (:bjarne) 2010-03-12 02:53:55 PST
FWIW, it looks like it is the .sjs which causes the leak. If I change the test to load some other resource, e.g. http://www,mozilla.org/, there is no leak.
Comment 32 Bjarne (:bjarne) 2010-03-12 13:40:25 PST
(In reply to comment #31)
> FWIW, it looks like it is the .sjs which causes the leak. If I change the test
> to load some other resource, e.g. http://www,mozilla.org/, there is no leak.

I was wrong there. Setting up a simple python http-server returning the same text as the sjs also makes the browser leak (not even using the sjs).
Comment 33 Bjarne (:bjarne) 2010-03-15 03:17:46 PDT
In fact, it also leaks when not using the test-framework (just invoking the browser from cmd-line and point it to a python http-server).
Comment 34 Bjarne (:bjarne) 2010-03-16 05:52:00 PDT
Gotcha! It turns out that nsXMLHttpRequest::OnStopRequest() didn't forward the call to the parser when it was aborted, causing the leaks. I have a code-fix which works locally - pushed to tryserver for verification...

The problem now is that we are handling potentially three issues : the documentation, the leak, and the original request from comment #0. I've filed bug #552651 for the leak. Not sure how to deal with the docs...
Comment 35 Bjarne (:bjarne) 2010-03-16 06:18:20 PDT
Created attachment 432790 [details] [diff] [review]
Code-fix and test which was comitted and backed out

Pulling this patch up from the hat again and requesting new round of review...
Comment 36 Serge Gautherie (:sgautherie) 2010-03-16 06:24:56 PDT
Comment on attachment 432790 [details] [diff] [review]
Code-fix and test which was comitted and backed out

>diff -r 84ff3bb3dbed netwerk/protocol/http/src/nsHttpChannel.cpp
>@@ -5212,16 +5212,21 @@ nsHttpChannel::OnStartRequest(nsIRequest
>+     // allow content to be cached if it was loaded successfully (bug #482935)
>+     PRBool contentComplete = PR_FALSE;
>+     if (NS_SUCCEEDED(status))
>+         contentComplete = PR_TRUE;
>+    

Nit:
PRBool contentComplete = NS_SUCCEEDED(status);
Comment 37 Bjarne (:bjarne) 2010-03-16 15:13:50 PDT
Tryserver is happy with this fix combined with the fix for bug #552651

I'll address nit asap...
Comment 38 Jonas Sicking (:sicking) 2010-03-16 15:36:27 PDT
(In reply to comment #27)
> Fwiw,
> 
> https://developer.mozilla.org/En/XMLHttpRequest
> "onreadystatechange: You need to call open() before setting this attribute."

This is not true. I've removed this from the docs.
Comment 39 Bjarne (:bjarne) 2010-03-19 08:14:13 PDT
Created attachment 433566 [details] [diff] [review]
Updated according to comments from reviewer

Unbitrot and fixed nit. Nothing has changed in the logic so I won't send it to tryserver again.
Comment 40 Serge Gautherie (:sgautherie) 2010-11-06 08:03:51 PDT
Comment on attachment 433566 [details] [diff] [review]
Updated according to comments from reviewer

Ping for review.
Comment 41 dwitte@gmail.com 2010-11-08 13:23:06 PST
Comment on attachment 433566 [details] [diff] [review]
Updated according to comments from reviewer

r=dwitte since this is materially the same patch that previously had r=jduell, and the leak fix has been committed.

I'd note that this doesn't apply to trunk. I'll assume the rebasing is trivial, but if not, please request a new round of review.
Comment 42 Serge Gautherie (:sgautherie) 2010-11-08 17:34:34 PST
Created attachment 489050 [details] [diff] [review]
(Av1e) Allow content to be cached if it was loaded successfully
[Backed out: Comment 47]

Previous patch, (context) unbitrotted.

"approval2.0=?":
Stop discarding good data. Should be no risk.
Comment 43 Benjamin Smedberg [:bsmedberg] 2010-11-10 12:20:57 PST
Not "no risk", but acceptable. Let's get it landed soon please.
Comment 44 Bjarne (:bjarne) 2010-11-10 13:16:36 PST
Requesting check-in...
Comment 45 Serge Gautherie (:sgautherie) 2010-11-10 13:38:45 PST
Comment on attachment 489050 [details] [diff] [review]
(Av1e) Allow content to be cached if it was loaded successfully
[Backed out: Comment 47]

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

***

"approval1.9.2.13=?":
"approval1.9.1.16=?":
Stop discarding good data. Should be low risk.
Comment 46 Bjarne (:bjarne) 2010-11-10 14:49:18 PST
Backed out because test caused crash in mochitest-1
Comment 47 :Ehsan Akhgari (busy, don't ask for review please) 2010-11-10 14:50:46 PST
Backed out because of mochitest-1 oranges:

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

Sample log:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289427186.1289428202.25328.gz
Comment 48 Bjarne (:bjarne) 2010-11-10 15:41:22 PST
Created attachment 489661 [details] [diff] [review]
(Av1f) Removed chrome:// urls from test-header - otherwise unchanged
[Backed out: Comment 53]

The test died because it included css and js-libs using chrome:// and apparently this is not allowed anymore. Fixed and tested locally.
Comment 49 Bjarne (:bjarne) 2010-11-10 15:42:26 PST
Comment on attachment 489661 [details] [diff] [review]
(Av1f) Removed chrome:// urls from test-header - otherwise unchanged
[Backed out: Comment 53]

Carrying over approval-requests for branches.
Comment 50 Bjarne (:bjarne) 2010-11-10 15:47:03 PST
Requesting checkin
Comment 51 Serge Gautherie (:sgautherie) 2010-11-10 16:13:58 PST
Comment on attachment 489661 [details] [diff] [review]
(Av1f) Removed chrome:// urls from test-header - otherwise unchanged
[Backed out: Comment 53]

http://hg.mozilla.org/mozilla-central/rev/090046fc6891
with extra tab+whitespace removed.
Comment 52 Serge Gautherie (:sgautherie) 2010-11-10 19:46:56 PST
The test fails intermittently:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289438444.1289440689.24597.gz
Rev3 Fedora 12 mozilla-central debug test mochitests-1/5 on 2010/11/10 17:20:44
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289439332.1289443179.5276.gz
WINNT 5.2 mozilla-central debug test mochitests-1/5 on 2010/11/10 17:35:32
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289439553.1289441518.28656.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitests-1/5 on 2010/11/10 17:39:13
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289442099.1289444391.11266.gz
Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-1/5 on 2010/11/10 18:21:39
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1289442917.1289444929.13937.gz
Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central debug test mochitests-1/5 on 2010/11/10 18:35:17
{
25602 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug482935.html | Received fresh value for second request - got "1", expected "0"
}
Comment 53 Michael Wu [:mwu] 2010-11-10 19:52:12 PST
Backed out due to orange. http://hg.mozilla.org/mozilla-central/rev/f37a8ce6a103
Comment 54 Bjarne (:bjarne) 2010-11-12 07:56:05 PST
Created attachment 490098 [details] [diff] [review]
Unit-test not calling Cancel() but still failing intermittently...

I modified the test for this patch to NOT call Cancel() in OnStopRequest and it still fails intermittently. I'll try to describe the problem more accurately and file a separate bug for it.

Question: Should we land the patch without the test?
Comment 55 Daniel Veditz [:dveditz] 2010-11-12 10:43:38 PST
Comment on attachment 489661 [details] [diff] [review]
(Av1f) Removed chrome:// urls from test-header - otherwise unchanged
[Backed out: Comment 53]

If you can't get it to stick on the trunk it doesn't belong on the branches -- minused for this round.
Comment 56 Jason Duell [:jduell] (needinfo? me) 2010-11-12 11:00:43 PST
Comment on attachment 490098 [details] [diff] [review]
Unit-test not calling Cancel() but still failing intermittently...

> I modified the test for this patch to NOT call Cancel() in OnStopRequest 

Clarification: this patch changes the test to take out the abort() in readyState == 4.   But it *also* does not include the nsHttpChannel.cpp changes, i.e. the actual bug fix.

If you're testing the abort-free test without the bug fix, i.e with

   CloseCacheEntry(PR_TRUE)     // the original code

than this does look like a separate bug.  (It would mean xhr requests are intermittently not getting cached.)  Or did the nsHttpChannel.cpp change somehow not make it into your patch?

> Question: Should we land the patch without the test?

If your abort-free test fails against trunk, maybe.  If it fails only when you change to 

   CloseCacheEntry(!contentComplete)

definitely not.

I'm not sure how important this fix is (we've lived with this behavior so far; it's an optimization, not a correctness issue; and I'm unclear on how often XHR gets abort() in state >= 4, yet we care about caching the data).  So even if we suspect that the orange here is an unrelated failure, I'm not sure it's worth landing the patch this late in the cycle.  My gut tells me no, but others may disagree.
Comment 57 Bjarne (:bjarne) 2010-11-12 13:07:05 PST
(In reply to comment #56)
> Clarification: this patch changes the test to take out the abort() in
> readyState == 4.   But it *also* does not include the nsHttpChannel.cpp
> changes, i.e. the actual bug fix.

Yes - the test (minus the call to Cancel()) fails intermittent, with no changes to the rest of the code. It is a separate bug.

I'll leave it to others to decide if we want the fix to nsHttpChannel.cpp , but that fix does not have anything to do with the intermittent failure we see in the test.
Comment 58 Bjarne (:bjarne) 2010-11-13 04:06:20 PST
Created attachment 490362 [details] [diff] [review]
Modified test slightly, unchanged otherwise
[Checked in: Comment 67]

Changed test to be more predictable with respect to when the second load is triggered (i.e. making sure to trigger second load after OnStopRequest() for first load has returned).

I've run this patch (including test) on the tryserver twice with no problems (only a few known oranges).

Requesting review again plus a new round of approvals...
Comment 59 Serge Gautherie (:sgautherie) 2010-11-13 08:10:05 PST
Comment on attachment 490362 [details] [diff] [review]
Modified test slightly, unchanged otherwise
[Checked in: Comment 67]

Too soon to request approvals.

Nit: Would executeSoon() work, instead of setTimeout()?
Comment 60 Bjarne (:bjarne) 2010-11-13 13:17:53 PST
(In reply to comment #59)
> Nit: Would executeSoon() work, instead of setTimeout()?

It would do the same, yes.
Comment 61 Jonas Sicking (:sicking) 2010-11-13 21:12:46 PST
executeSoon is preferred, so please use that (no need to attach new patch unless reviewers ask for it though).
Comment 62 dwitte@gmail.com 2010-11-22 14:47:42 PST
Comment on attachment 490362 [details] [diff] [review]
Modified test slightly, unchanged otherwise
[Checked in: Comment 67]

I'm not really the right reviewer for this -- suggest biesi, though I can look at it if necessary. (Tests are fine, I'm not sure about the code changes.)
Comment 63 Mike Beltzner [:beltzner, not reading bugmail] 2011-02-23 10:12:24 PST
Comment on attachment 489050 [details] [diff] [review]
(Av1e) Allow content to be cached if it was loaded successfully
[Backed out: Comment 47]

(bookkeeping)
Comment 64 Jason Duell [:jduell] (needinfo? me) 2011-03-22 06:06:03 PDT
Comment on attachment 490362 [details] [diff] [review]
Modified test slightly, unchanged otherwise
[Checked in: Comment 67]

Creating 2nd XHR request in readyStateChange == 4 does seem like an improvement in the mochitest.

Change to use executeSoon, per comment 59.  And make sure no lines are over 80 chars (I think some comments may be longer).
Comment 65 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-23 20:38:39 PDT
I wanted to land this on cedar, but I was not sure which patch should be landed...
Comment 66 Jason Duell [:jduell] (needinfo? me) 2011-03-24 03:19:35 PDT
Comment on attachment 490362 [details] [diff] [review]
Modified test slightly, unchanged otherwise
[Checked in: Comment 67]

Ehsan, this is the only patch that should land.  Thanks!
Comment 67 :Ehsan Akhgari (busy, don't ask for review please) 2011-03-25 11:33:59 PDT
http://hg.mozilla.org/mozilla-central/rev/92b43aa07b7d
Comment 68 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-13 17:40:07 PDT
This bug has not been completely fixed.  See bug 648485.  That looks like a real code failure.
Comment 69 Bjarne (:bjarne) 2011-04-14 09:50:21 PDT
Please see comment #54 (I apparently did not get around to follow up the comment). If you push that test (which does not even touch the functionality addressed by this bug) does it also fail?
Comment 70 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-14 13:21:21 PDT
What do you mean if I push that test?  That patch doesn't even apply cleanly:

ehsanakhgari:~/moz/mozilla-central [04:19:55]$ hg qpus
applying attachment.cgi?id=490098
patching file content/base/test/Makefile.in
Hunk #1 FAILED at 242
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/Makefile.in.rej
file content/base/test/bug482935.sjs already exists
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/bug482935.sjs.rej
file content/base/test/test_bug482935.html already exists
1 out of 1 hunks FAILED -- saving rejects to file content/base/test/test_bug482935.html.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh attachment.cgi?id=490098

If you don't have a clear idea on what's wrong here, I should back out the patch (from mozilla-central and aurora), and let you investigate offline.  The failures on mozilla-central are hurting everybody.
Comment 71 Jason Duell [:jduell] (needinfo? me) 2011-04-15 10:17:36 PDT
My read of comment 69 is that Bjarne is suggesting that if the modified version of the test (which doesn't use the functionality in this bug) is breaking as often as this one, then the issue is a separate (as yet unfiled) bug (and we should leave this bug FIXED).

> That patch doesn't even apply cleanly:

I think it's just a different version of "add test file."

Bjarne, we should figure this out ASAP.  Let's get a patch that changes the test to the version that doesn't cancel, and see if it's still orange.
Comment 72 :Ehsan Akhgari (busy, don't ask for review please) 2011-04-15 12:35:14 PDT
(In reply to comment #71)
> Bjarne, we should figure this out ASAP.  Let's get a patch that changes the
> test to the version that doesn't cancel, and see if it's still orange.

Even better, let's have a separate test which does not exercise the code added in this bug, and land them both and see if they both fail.
Comment 73 Bjarne (:bjarne) 2011-05-02 13:28:52 PDT
(In reply to comment #68)
> This bug has not been completely fixed.  See bug 648485.  That looks like a
> real code failure.

Should we resolve this one since bug #648485 seems to be fixed?
Comment 74 Bjarne (:bjarne) 2011-06-24 12:31:49 PDT
Resolved/fixed since no-one objected to previous comment.
Comment 75 Serge Gautherie (:sgautherie) 2011-07-05 19:38:20 PDT
(In reply to comment #67)
> http://hg.mozilla.org/mozilla-central/rev/92b43aa07b7d

Ftr, this changeset still has setTimeout() instead of "expected" executeSoon()...
(And superfluous |netscape.security.PrivilegeManager.disablePrivilege("UniversalXPConnect");|.)
Comment 76 Patrick McManus [:mcmanus] 2016-02-09 12:41:56 PST
*** Bug 683817 has been marked as a duplicate of this bug. ***

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