Closed Bug 562901 Opened 14 years ago Closed 14 years ago

Synchronous XMLHttpRequests cause setInterval to stop functioning

Categories

(Core :: DOM: Core & HTML, defect, P1)

Product:
Core
Component:
DOM: Core & HTML
Version:
1.9.1 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0

People

(Reporter: michaelblizzard, Assigned: smaug)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Simple repro of the setInterval/XMLHttpRequest issue
3.95 KB, application/x-zip-compressed
Details
patch
1.01 KB, patch
Details | Diff | Splinter Review
patch
2.07 KB, patch
jst
: review+
christian
: approval1.9.2.9-
christian
: approval1.9.1.12-
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3

setInterval will stop functioning in Firefox 3.5+ when making multiple synchronous XMLHttpRequests.  This issue is intermittent but I am able to reproduce it.

NOTE: I am NOT able to reproduce this issue when Firebug is installed.

Reproducible: Sometimes

Steps to Reproduce:
1.Create a page with two frames each containing an input box.
2.On the load event of each page, use setInterval to populate the input boxes with a counter running every 50 milliseconds.
3.For each setInterval, make a simple synchronous XMLHttpRequest:

var xhReq = new XMLHttpRequest();
xhReq.open("POST", "form1.aspx",false);
xhReq.send(null);
var serverResponse = xhReq.responseText; 

Most of the time the counters will run as expected but if you refresh the page a few times you'll eventually see that one (if not both) of the counters will not work.

NOTE: When the issue is reproduced, setInterval never initially fires (it doesn't stop working half way through the process).
Actual Results:  
Intermittently, you'll see that the input boxes don't get populated, the counter never actually starts.

Expected Results:  
For every iteration of of setInterval, the counter in the input boxes should increment.
Version: unspecified → 3.5 Branch
Version: 3.5 Branch → 3.6 Branch
Priority: -- → P1
OS: Windows 7 → All
Hardware: x86 → All
This is not an issue before version 3.5.
Version: 3.6 Branch → 3.5 Branch
Component: General → DOM
Product: Firefox → Core
QA Contact: general → general
Version: 3.5 Branch → 1.9.1 Branch
Sounds like this is a request that bug 340345 be undone.
Blocks: 340345
Just to simplify my repro steps, you don’t need multiple setIntervals running.  You make a synchronous XMLHttpRequest within a set interval and refresh the page a few times; you’ll see that the setInterval will not fire.  I believe it’s more apparent with multiple setIntervals because they interfere with each other; plus, this is more of a realistic real world scenario.  In our application we have multiple setIntervals running and allot of the time when the page loads one of those setIntervals never initially executes.
I'll try to look at this next week
Assignee: nobody → Olli.Pettay
Yeah, I've definitely seen this in the past too.  It affects my apps as well.  Please fix this.

James
Thanks Olli, let me know if you need any additional details, I really appreciate the help.
Well, could you provide a *minimal* testcase attached using  "Add an attachment".

And if I don't remember to debug this before next Tuesday, please add a comment 
here :)
Run the attached example from a virtual directory.  The ability to reproduce the issue seems to vary from machine to machine.  It only seems possible to reproduce in Firefox 3.5 plus.  I’ve been able to reproduce this with and without firebug enabled but if you can’t get it to happen one way then try the other.   When you open default.htm you should see 4 counters running.  Hit refresh a couple of times and on occasion you’ll see that some of the counters stop running.
I have no idea what is "virtual directory".
Does the example work if I unzip it to a local directory.
The example seem to use some Windows thing "Web.config".
is that needed? (I use mainly just Linux and OSX)
But seems like the testcase is simple enough to work on my webserver.
I'll try later this week.
Sorry Olli, webserver will do.  Thanks.
Also, the web.config is not needed, I was using IIS and it got packaged with my zip file.  Thanks again.
Bug 504569
Depends on: 504569
No longer depends on: 504569
Bug 504569 (mentioned above) seems to be the same issue.  I've tested this with setTimeout and setInterval with simular results.
Looking at this today.
Ok, I can reproduce this on 3.6, but not on trunk.

Michael, do you think you could try to search when this was fixed on trunk.
You can find nightly builds here
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
mozilla-central is "trunk".
3.6 branched sometime last august I think, so after that mozilla-central is different than 3.6 (i.e. 1.9.2).

Binary-search like testing works fine when trying to find (un)regression ranges
in builds.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Ben, do you remember any relevant fixes on trunk?
Olli,

I'm glad you've been able to reproduce our issue, that's great news.  I'll do some searching first thing tomorrow.
Hi Olli, I'm new Mozilla bug tracking, what's the best way to go about tracking down when this was fixed?  Is there documentation I can look over that lists bug fixes in various versions?
I was able to reproduce the error with the following build: firefox-3.7a5pre.en-US.win32.installer.exe ... could you point me in the direction of the "truck" build you tried?
I was using my own builds.

I guess I need to try harder with trunk builds, if you managed to reproduce on
trunk.
It’s so intermittent that is seems the slightest change can throw it off.  For example, for the longest time I couldn’t reproduce it when I was using Firebug but then suddenly it would happen every time.  If you are still running my sample project with the four timers try changing their intervals.  I had them all running at 50 milliseconds.  Try making them run at different intervals (e.g. 50, 75, 100, and 150).  

FYI, the issue doesn't happen as often with "Minefield 3.7a5pre (rv: 1.9.3a5pre)" as it does with "Firefox 3.6.3"
Hi Ollie, any luck reproducing this issue on trunk?
I am also experiencing this issue and I was wondering how the progress is going towards fixing it?
I am also experiencing this issue and I was wondering how the progress is going towards fixing it?
Well, at least I need to find some way to reproduce it on trunk.
Though, I guess I could try to fix it first on branch and port the fix to trunk.
(I've been just a bit busy lately.)
Hi Olli, I made a few suggestions in comment 23 that might help you reproduce this on trunk; it's sort of hit and miss.  I’m not 100% familiar with how Mozilla handles its defects; since this is open source is it up to the community to resolve these issues or do you work actually work for Mozilla?  The reason I ask is because I want to take the quickest path possible to resolve this issue as it is seriously effecting one of our commercial products.  If there is anything I can do to help please let me know, we appreciate your efforts so far.
I do work for Mozilla. I have plenty of other things in my todo list, so the
problem is to find the time for this. But I'll try, this week.
On Thursday, I hope.
Actually, I think I know where the problem is. Patch and tryserver builds coming
today or tomorrow, I hope.
Attached patch patch (obsolete) — — Splinter Review 
This seems to help on 1.9.2 branch.

Look for 7d545e6f5cca in https://build.mozilla.org/tryserver-builds/?C=M;O=D
in a few hours. That should be a trunk build with the patch.

I need to write a mochitest for this.
Bah, the patch is probably wrong after all.
Attached patch patch — — Splinter Review 
I've tried to find an automated way to test this, but not luck yet.
Attachment #447424 - Attachment is obsolete: true
Attachment #447513 - Flags: review?(bent.mozilla)
Comment on attachment 447513 [details] [diff] [review]
patch

Another possibility would be perhaps to change the code
to iterate over subdocuments, not docshells.
End result should be the same.
Attachment #447513 - Flags: review?(bent.mozilla) → review?(jst)
If you could post a Win32 installation I can test this thoroughly on my side Olli.
This seems to have resolved the issue in our sandbox environment, and by looking at your code I think I can see why, but unfortunately the issue is still present in our production application.  Our application uses multiple setInterval/setTimeout calls across multiple frame sets.  Is it possible the root of the problem lies deeper in the architecture of how Firefox handles setInterval/setTimeout?
(In reply to comment #37)
> Is it possible the
> root of the problem lies deeper in the architecture of how Firefox handles
> setInterval/setTimeout?
I'm pretty sure the bug is in Suspend/Resume, not in timeout/interval code.

But without a testcase it is hard fix the problem you're still seeing.
Could you perhaps try to find a minimal testcase for that?
(Btw, have to say, synchronous XHR is generally bad for user experience, since it locks up UI. But nevertheless, this bug must be fixed.)
Is it possible to use my original test case to track down the root of the problem?  That is, unless that patch you've already implemented is still required to solve the over all problem.
Well, the patch does fix the problem your testcase shows.
I assume there is some other variant of the problem you're seeing in your
web app.
Would it be possible to get access to your web app?
Definitly, I'll email you the login information and repro steps shortly, I just need to create you an account.  Thanks.
This is awesome news that the issue is being addressed.

I was told of this issue with some who were using Windows XP. Even when I was on XP I didn't see the issue but I always had Firebug installed and near as I can tell, the cases when the issue was seen, they didn't have the add-on.

Will this fix be part of the 3.6.4 release or would it wait until a later release (4.0 perhaps)?

Awesome stuff! Very excited to see this one ironed out.
Attachment #447513 - Flags: review?(jst)
Attached patch patch — — Splinter Review 
I pushed this to tryserver.

This seems to fix the testcase and the web app I have been testing.

The patch is very close to the previous one. The main change is the inner window check, which prevents 
rv = win->ResumeTimeouts(aThawChildren);
NS_ENSURE_SUCCESS(rv, rv);
to run, and thus allows looping over all the child windows.

Jst, this is still simpler approach than what I mentioned to you yesterday.
So should be safer for branches.

But let's see what tryserver says about this and would be great if you Michael could 
test the tryserver builds once they are ready.
That's great news!  I'll test it thoroughly once the builds are up.  Thanks Olli.
Great, I'll keep a look out for them.
Tryserver doesn't complain any new failures because of the patch.

Not sure why only maemo build is upload to the build.mozilla.org atm.
Comment on attachment 449016 [details] [diff] [review]
patch

Asking review for this (assuming Michael can confirm that the web app works).
I need to still find some way to test this.
Attachment #449016 - Flags: review?(jst)
Olli, your patch works perfectly, I wasn't able to repro the issue in any of my test enviroments.  Thank you so much.  What is the process for getting this patch included in a Firefox release, do you need anything else from me?
The process is that the patch needs to get reviewed, and there should be automated
tests for it. Then for branches (FF 3.5.x and FF 3.6.x) the patch needs
approval.

I agree this is something which should be fixed on branches.

If you want to help, would be awesome if you could make tests which don't
need user interaction. I could change such test to mochitest.
Jst, could you review the patch? I'd like to land to patch to trunk even
without tests so that we could see whether this causes regressions.
I somehow don't seem to find the way to test this all
in mochitest, but still trying.
Comment on attachment 449016 [details] [diff] [review]
patch

r=jst, but I'd like to know how this makes us work compared to other browsers with respect to the interaction between timeouts and sync XHRs.
Attachment #449016 - Flags: review?(jst) → review+
We are anxious to know if we will be seeing this patch in an upcoming version of Firefox.  Is it possible to get a status update?  Thanks, Mike
Uh, I forgot to post the link to the hg changeset. 
http://hg.mozilla.org/mozilla-central/rev/1181eb2743a9

So this should be fixed on trunk, but before asking approval for
branch, I need to figure out some test which doesn't involve
user interaction.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Ok, so "trunk" is the Minefield build and "branch" would be Firefox?
Trunk is Minefield, or the becoming Firefox 4.
Branches are Firefox 3.6.x and 3.5.x
Ok great, thanks for the info. I also found this link for anyone following that wants to learn more:

http://forums.mozillazine.org/viewtopic.php?t=305281

I guess branch is what we are pushing for then, the sooner we can get this to our clients the better.  We definitly appreciate the efforts so far.
What release can we expect the patch to be in?
Attachment #449016 - Flags: approval1.9.2.8?
Attachment #449016 - Flags: approval1.9.1.12?
Comment on attachment 449016 [details] [diff] [review]
patch

Asking approval, since this is pretty nasty bug. But I haven't found yet a way to test this automatically - so no mochitests yet.
Martijn, could you perhaps help with the mochitest?
Any luck getting that patch pushed through without the mochitest?
To anyone waiting for this fix, it appears as if it is no longer an issue in Firefox Beta 4.  Could anyone confirm?
Comment on attachment 449016 [details] [diff] [review]
patch

Denying for branches. This is an old bug, behavior we specifically took, and it is not worth the risk to fix it on branches.
Attachment #449016 - Flags: approval1.9.2.9?
Attachment #449016 - Flags: approval1.9.2.9-
Attachment #449016 - Flags: approval1.9.1.12?
Attachment #449016 - Flags: approval1.9.1.12-
(In reply to comment #66)
>This is an old bug,
Yes

> behavior we specifically took,
No. This is pretty serious regression from Sync XHR handling changes.


> and it
> is not worth the risk to fix it on branches.
This might be true.
Keywords: regression
Depends on: 592947
No longer depends on: 592947
I am struggling with this same problem on FF 3.6 and am not in a position to move up.
Is there any chance this will be reconsidered for inclusion in 3.6?
(In reply to sgrube from comment #68)
> I am struggling with this same problem on FF 3.6 and am not in a position to
> move up.
> Is there any chance this will be reconsidered for inclusion in 3.6?

No, sorry.
Target Milestone: --- → mozilla2.0
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: