Cancel() from OnStopRequest should not cause cache entry to be doomed

RESOLVED FIXED in mozilla5

Status

()

Core
Networking: HTTP
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: Biesinger, Assigned: bjarne)

Tracking

Trunk
mozilla5
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(status2.0 wontfix, status1.9.2 wontfix, status1.9.1 wontfix)

Details

Attachments

(1 attachment, 10 obsolete attachments)

6.23 KB, patch
jduell
: review+
Details | Diff | Splinter Review
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.
(Assignee)

Comment 1

8 years ago
(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?
(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).
(Assignee)

Updated

8 years ago
Assignee: nobody → bjarne
(Assignee)

Comment 3

8 years ago
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).
Attachment #369254 - Flags: review?(cbiesinger)
(Assignee)

Comment 4

8 years ago
Created attachment 369257 [details] [diff] [review]
Includes proper heading, otherwise unchanged
Attachment #369254 - Attachment is obsolete: true
Attachment #369257 - Flags: review?(cbiesinger)
Attachment #369254 - Flags: review?(cbiesinger)
(Assignee)

Comment 5

8 years ago
Any indication of situations not handled by the patch would be appreciated...
(Assignee)

Comment 6

8 years ago
Comment on attachment 369257 [details] [diff] [review]
Includes proper heading, otherwise unchanged

Trying new reviewer...
Attachment #369257 - Flags: review?(cbiesinger) → review?(jduell.mcbugs)
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 :)
(Assignee)

Comment 8

8 years ago
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).
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...
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).
(Assignee)

Comment 11

8 years ago
Created attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]

Test moved to content/base, otherwise unchanged.
Attachment #369257 - Attachment is obsolete: true
Attachment #392097 - Flags: review?(jduell.mcbugs)
Attachment #369257 - Flags: review?(jduell.mcbugs)
Attachment #392097 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 12

8 years ago
Requesting check-in..
Keywords: checkin-needed
Comment on attachment 392097 [details] [diff] [review]
Unbitrot + test moved
[Backout: Comment 15]


http://hg.mozilla.org/mozilla-central/rev/3a829715fd39
Attachment #392097 - Attachment description: Unbitrot + test moved → Unbitrot + test moved [Checkin: Comment 13]
Status: NEW → RESOLVED
Last Resolved: 8 years ago
status1.9.1: --- → ?
Flags: wanted1.9.2?
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Version: unspecified → Trunk
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.
Attachment #392097 - Attachment description: Unbitrot + test moved [Checkin: Comment 13] → Unbitrot + test moved [Checkin: See comment 13]
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backing this out did fix the leaks.
Bjarne, I don't know how you tested you patch ... Anyway, next time, have it run through TryServer.
Status: REOPENED → NEW
(Assignee)

Comment 18

8 years ago
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...)
(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
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 :-/
(Assignee)

Comment 21

8 years ago
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 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?
Attachment #392097 - Attachment description: Unbitrot + test moved [Checkin: See comment 13] → Unbitrot + test moved [Backout: Comment 15]
Attachment #392097 - Attachment is obsolete: true
(Assignee)

Comment 23

8 years ago
(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.
(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...
(Assignee)

Comment 25

8 years ago
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...
(Assignee)

Comment 26

8 years ago
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 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?
(Assignee)

Comment 28

7 years ago
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...
Attachment #396080 - Attachment is obsolete: true
(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!
Status: NEW → ASSIGNED
(Assignee)

Comment 30

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

Comment 31

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

Comment 32

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

Comment 33

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

Comment 34

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

Updated

7 years ago
Depends on: 552651
(Assignee)

Comment 35

7 years ago
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...
Attachment #431838 - Attachment is obsolete: true
Attachment #432790 - Flags: review?(jduell.mcbugs)
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);
(Assignee)

Comment 37

7 years ago
Tryserver is happy with this fix combined with the fix for bug #552651

I'll address nit asap...
(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.
(Assignee)

Comment 39

7 years ago
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.
Attachment #432790 - Attachment is obsolete: true
Attachment #433566 - Flags: review?(jduell.mcbugs)
Attachment #432790 - Flags: review?(jduell.mcbugs)
Comment on attachment 433566 [details] [diff] [review]
Updated according to comments from reviewer

Ping for review.
Attachment #433566 - Flags: review?(dwitte)

Comment 41

7 years ago
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.
Attachment #433566 - Flags: review?(jduell.mcbugs)
Attachment #433566 - Flags: review?(dwitte)
Attachment #433566 - Flags: review+
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.
Attachment #433566 - Attachment is obsolete: true
Attachment #489050 - Flags: approval2.0?
Attachment #489050 - Flags: approval2.0? → approval2.0+
Not "no risk", but acceptable. Let's get it landed soon please.
(Assignee)

Comment 44

7 years ago
Requesting check-in...
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.
Attachment #489050 - Attachment description: (Av1e) Allow content to be cached if it was loaded successfully → (Av1e) Allow content to be cached if it was loaded successfully [Checked in: Comment 45]
Attachment #489050 - Flags: approval1.9.2.13?
Attachment #489050 - Flags: approval1.9.1.16?
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.3a1 → mozilla2.0b8
(Assignee)

Comment 46

7 years ago
Backed out because test caused crash in mochitest-1
(Assignee)

Updated

7 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → UNCONFIRMED
Ever confirmed: false
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 48

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

Comment 49

7 years ago
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.
Attachment #489661 - Flags: approval1.9.2.13?
Attachment #489661 - Flags: approval1.9.1.16?
(Assignee)

Comment 50

7 years ago
Requesting checkin
Keywords: checkin-needed
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.
Attachment #489661 - Attachment description: Removed chrome:// urls from test-header - otherwise unchanged → (Av1f) Removed chrome:// urls from test-header - otherwise unchanged [Checked in: See comment 51]
Attachment #489050 - Attachment description: (Av1e) Allow content to be cached if it was loaded successfully [Checked in: Comment 45] → (Av1e) Allow content to be cached if it was loaded successfully
Attachment #489050 - Attachment is obsolete: true
Attachment #489050 - Flags: approval1.9.2.13?
Attachment #489050 - Flags: approval1.9.1.16?
Status: NEW → RESOLVED
Last Resolved: 7 years ago7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
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"
}
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 53

7 years ago
Backed out due to orange. http://hg.mozilla.org/mozilla-central/rev/f37a8ce6a103
(Assignee)

Comment 54

7 years ago
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 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.
Attachment #489661 - Flags: approval1.9.2.13?
Attachment #489661 - Flags: approval1.9.2.13-
Attachment #489661 - Flags: approval1.9.1.16?
Attachment #489661 - Flags: approval1.9.1.16-
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.
(Assignee)

Comment 57

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

Comment 58

7 years ago
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...
Attachment #489661 - Attachment is obsolete: true
Attachment #490362 - Flags: review?(jduell.mcbugs)
Attachment #490362 - Flags: approval2.0?
Attachment #490362 - Flags: approval1.9.2.13?
Attachment #490362 - Flags: approval1.9.1.16?
Attachment #489050 - Attachment description: (Av1e) Allow content to be cached if it was loaded successfully → (Av1e) Allow content to be cached if it was loaded successfully [Backed out: Comment 47]
Attachment #489661 - Attachment description: (Av1f) Removed chrome:// urls from test-header - otherwise unchanged [Checked in: See comment 51] → (Av1f) Removed chrome:// urls from test-header - otherwise unchanged [Backed out: Comment 53]
Attachment #489661 - Flags: approval1.9.2.13-
Attachment #489661 - Flags: approval1.9.1.16-
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()?
Attachment #490362 - Flags: review?(dwitte)
Attachment #490362 - Flags: approval2.0?
Attachment #490362 - Flags: approval1.9.2.13?
Attachment #490362 - Flags: approval1.9.1.16?
(Assignee)

Comment 60

7 years ago
(In reply to comment #59)
> Nit: Would executeSoon() work, instead of setTimeout()?

It would do the same, yes.
executeSoon is preferred, so please use that (no need to attach new patch unless reviewers ask for it though).

Comment 62

7 years ago
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.)
Attachment #490362 - Flags: review?(dwitte)
Comment on attachment 489050 [details] [diff] [review]
(Av1e) Allow content to be cached if it was loaded successfully
[Backed out: Comment 47]

(bookkeeping)
Attachment #489050 - Flags: approval2.0+ → approval2.0-
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).
Attachment #490362 - Flags: review?(jduell.mcbugs) → review+
I wanted to land this on cedar, but I was not sure which patch should be landed...
Attachment #490098 - Attachment is obsolete: true
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!
http://hg.mozilla.org/mozilla-central/rev/92b43aa07b7d
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla2.0b8 → mozilla2.2
This bug has not been completely fixed.  See bug 648485.  That looks like a real code failure.
Status: RESOLVED → REOPENED
Depends on: 648485
Resolution: FIXED → ---
(Assignee)

Comment 69

6 years ago
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?
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.
Depends on: 650132
No longer depends on: 650132
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.
(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.
(Assignee)

Comment 73

6 years ago
(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?
(Assignee)

Comment 74

6 years ago
Resolved/fixed since no-one objected to previous comment.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(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");|.)
status1.9.1: ? → wontfix
status1.9.2: --- → wontfix
status2.0: --- → wontfix
Flags: wanted1.9.2?
Attachment #490362 - Attachment description: Modified test slightly, unchanged otherwise → Modified test slightly, unchanged otherwise [Checked in: Comment 67]
Duplicate of this bug: 683817
You need to log in before you can comment on or make changes to this bug.