User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616 Hi! When I use a XMLHttpRequest object to fetch a file with async = false, both Firefox and Mozilla take 100% of the CPU until the request have been completed. If I ask for a long .xml file, my machine almost freezes until the file has been downloaded. I have a small cgi script that just writes a small xml text, but with a delay of 10 seconds. If the request is made with async=true, everything works flawlessly (Mozilla just waits until the script have been downloaded). But if the request is made with async=false, Mozilla takes 100% of the CPU until the script is downloaded. I guess it has something to do with the interaction between the parent process and the thread that makes the new petition. Probably the parent is using some "while" without using some blocking mechanism to wait without using CPU. I attach a small cgi that sleeps for a while, so Mozilla has to wait for a while. Test cgi script: xml.cgi ------------------ #!/bin/bash echo "Content-type: text/xml" echo sleep 10 echo "<?xml version='1.0' encoding='ISO-8859-1'?><root></root>" ------------------ Test javascript code: ---------------------- function load_nonasyncronous(url) { var o = XMLHttpRequest(); o.open( "GET", url, false ); o.send(null); alert("result: " + o.responseText); } function load_asyncronous(url) { var o = XMLHttpRequest(); o.onreadystatechange = function() { if (o.readyState == 4) alert("result: " + o.responseText); o.open( "GET", url, true ); o.send(null); } ----------------------- Thanks for such a great browser (and such a huge effort). Reproducible: Always Steps to Reproduce: 1.Write some js that fetches a xml from a CGI with some delay using XMLHttpRequest and async = false 2. See CPU usage until the xml is fetched 3. Actual Results: The CPU usage of Mozilla raises up to 100% Expected Results: Mozilla should have waited for the file without using CPU
*** This bug has been marked as a duplicate of 190313 ***
In Comment 1 Heikki marks this bug as a duplicate of Bug 190313 (XMLExtras syncloading blocks UI (syncloadservice)), but i don't think this is really a duplicate. It's not that the UI gets blocked (that could be a correct behaviour). The problem is that the CPU usage raises to 100%. There's another bug like this one (Bug 240678 (Mozilla 1.7b (and Firefox 0.8) is busy waiting on synchronous xmlhttprequest)), but that is already marked as a duplicate of 190313. Could someone verify this?
(In reply to comment #2) > In Comment 1 Heikki marks this bug as a duplicate of Bug 190313 (XMLExtras > syncloading blocks UI (syncloadservice)), but i don't think this is really a > duplicate. It's not that the UI gets blocked (that could be a correct behaviour). > The problem is that the CPU usage raises to 100%. The UI should never be blocked by JS running on a page. The reason it is blocked is because all the XML processing happens in a tight loop that does not yield to the UI. Incidentally, that also manifests as 100% CPU. I still believe this bug is duplicate of bug 190313. The only case where this would not be a duplicate is if you can use the UI while the CPU is pegged at 100% in the Mozilla process.
Created attachment 172074 [details] [diff] [review] Use WaitForEvent() instead of ProcessPendingEvent() I have tested the patch with the nightly codes (2005/01/21) on my sparc machine, it is one time faster than before
Can anyone help me to verify if the patch works for you? Thanks
(In reply to comment #4) > Created an attachment (id=172074) [edit] > Use WaitForEvent() instead of ProcessPendingEvent() > > I have tested the patch with the nightly codes (2005/01/21) on my sparc > machine, it is one time faster than before I believe WaitForEvent() works as well as ProcessPendingEvent(), but AFAIK it has the same problem - you hit 100% CPU locking the UI until request finishes. It *may* be slightly more efficient, but it is not the real solution IMO. Look at bug 190313 for more ideas (I still maintain these are dupes).
I think the root cause is in the following codes: // If we're synchronous, spin an event loop here and wait if (!(mState & XML_HTTP_REQUEST_ASYNC)) { while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { modalEventQueue->ProcessPendingEvents(); } when in sync mode, this thread will in a busy loop, which will waste a lot of CPU time. I think the solution is to block current thread until another thread posts an event in the modalEventQueue.
(In reply to comment #7) > I think the root cause is in the following codes: Yes (but I think I have explained this earlier in the dupe or some other bug) this code was put in so sync loading would also work in embedded builds. Before, sync loading was done in such a way that it did not block the UI (or cause 100% CPU) but it only worked in the normal Mozilla builds, not in the embedded case. As far as I know, the main question which still hasn't been resolved is what kind of technology to use so that it will work without blocking the UI and still work in embedded and normal case. The embedded case is problematic because there Mozilla does not own the event loop. > when in sync mode, this thread will in a busy loop, which will waste a lot of > CPU time. I think the solution is to block current thread until another thread > posts an event in the modalEventQueue. No. The current thread is the UI, so you must not block it. But maybe it would be possible to push the whole XMLHttpRequest into the network thread or something.
Created attachment 172403 [details] [diff] [review] a testcase
(In reply to comment #5) > Can anyone help me to verify if the patch works for you? Thanks Hello the patch https://bugzilla.mozilla.org/attachment.cgi?id=172074 works for me (i apply the patch manually). The opensi application (http://www.opensi.org/) does not work on windows without this patch (without the patch firefox or mozilla : 100% CPU / with the patch and mozilla : % of use of CPU is OK). In french : Description of the problem with opensi application without the patch : http://forums.opensi.org/viewtopic.php?t=126&sid=d6e823a4e2a3765fc543726d8309fe06
Created attachment 175905 [details] testcase
Created attachment 175907 [details] testcase
I don't think we can move XMLHttpRequest related codes to somewhere else. Because these javascript codes run in javascript interpreter, which runs in the current thread(UI thread). I don't know if there is some way to let a piece of Javascript codes run in another thread.
Boying, what do you mean, exactly? The code that actually does the sync request is in C++, not in JS....
In the following snippet: fetcher.open("POST",getServletURL(),false); fetcher.setRequestHeader("Content-type","text/plain; charset=UTF-8"); try { fetcher.send(infotosent); }catch (e) { window.alert("send error:"+e); } if (typeof fetcher.responseText=="string"&&fetcher.responseText.length>0) { window.alert("time taken:" + (end-start)+"ms\nServerRespons:\n"+fetcher.responseText); } where fetcher is an XMLHttpRequest object and previous codes are trigged by a button embedding in a HTML page. All above codes will be run in UI thread. If we move the implementation of send() method into an other thread. The UI thread still should be blocked at the last "if" statment in sync mode because fetcher.responseText contains servers response.
Oh, I see. That was in response to comment 8.... Yeah, I think we want to keep this on the UI thread. If we can use the syncload service, we should. If we can't, we should probably make the minor tweaks needed so we _can_ use it and use it.
The sync load service works by calling WaitForEvent. That method calls PR_Wait on a monitor. That means that if called on the UI thread, it will lock up the UI until some new PLEvent is added to the queue. This means that UI events will starve. It is definitely suboptimal. ProcessingPendingEvents works by allowing all PLEvents and native events a chance to run, but it does not block waiting for either. What we really need is a way to say: block for PLEvents or native events. I think the event queue system should be revised to somehow communicate better with the appshell to make this possible. confirming bug
In my JS-based application I see complete starvation and hang from time to time when using Mozilla or Firefox builds with SVG enabled (trunk builds from mid-March 2005 on Win2K): the HTTP request is sent but nothing more happens, except 100% cpu load. What makes the thing even more worse is, say have Firefox running and blocking, and now start a Mozilla instance running the same JS application and using XMLHttpRequest, it blocks and hangs the same as the other Firefox (!) instance. Note that the server is responding and has no problems, it is just that both FF and Moz block for no appearant reason. Only after several minutes of waiting it seems that some semaphore gets released, so FF and Moz work again.
bz, darin, anyone: who should own this bug? /be
Someone who understands appshell, preferably... It sounds like we want to go back out to the OS-level event loop and wait on OS events, but with an event queue pushed and without actually calling "return" from the function we're in.
So, in other words, we need the code from OpenWindowJS ;-) I can add this to my queue, but I'm already overburdened for Firefox 1.1. I would greatly appreciate some help. Jonas? ;-)
Yeah, I'd be interested in working on this since I think it's an important bug. I'm not sure how much time i'll have available just yet though. Should know more in a week or two.
Is it dupe of bug 190313?
no
This is related to bug 309424
Jonas, have you had any cycles to investigate here? Is there a possibility of a low-risk fix?
I havn't investigated yet, but i really doubt there's anything lowrisk to go on the branch.
it's too late for anything but low-risk fixes. the time to take a fix for something like this was months ago.
(In reply to comment #27) > I havn't investigated yet, but i really doubt there's anything lowrisk to go on the branch. The fix I tried was a oneliner: Add a PR_SLEEP with 10 miliseconds in the loop. while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { PR_Sleep( PR_MillisecondsToInterval(10)); modalEventQueue->ProcessPendingEvents(); } Can you add this as a low risk fix for the 100% cpu and use the other defects for the long term solutions as described by defect 190313 ?
Created attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop
See also my patch in bug 309424. Perhaps we should combine these two patches.
(In reply to comment #31) > See also my patch in bug 309424. Perhaps we should combine these two patches. Sounds like a fine idea, Who should review these ?
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop if (!(mState & XML_HTTP_REQUEST_ASYNC)) { while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { + PR_Sleep( PR_MillisecondsToInterval(20)); Seems reasonable, but that should be 10, not 20, right? And loose the space after '('. r=jst
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop > // If we're synchronous, spin an event loop here and wait > if (!(mState & XML_HTTP_REQUEST_ASYNC)) { > while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { >+ PR_Sleep( PR_MillisecondsToInterval(20)); > modalEventQueue->ProcessPendingEvents(); > } wouldn't it be better to only sleep if you are going to do another iteration? i.e., how about this: while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { modalEventQueue->ProcessPendingEvents(); // Be sure not to busy wait! (see bug 273578) if (mState & XML_HTTP_REQUEST_SYNCLOOPING) PR_Sleep( PR_MillisecondsToInterval(10)); }
Comment on attachment 199529 [details] [diff] [review] simple fix (workaround) with 10 millisecond sleep in the loop >Index: nsXMLHttpRequest.cpp > if (!(mState & XML_HTTP_REQUEST_ASYNC)) { > while (mState & XML_HTTP_REQUEST_SYNCLOOPING) { >+ PR_Sleep( PR_MillisecondsToInterval(20)); > modalEventQueue->ProcessPendingEvents(); > } Or even: if (!(mState & XML_HTTP_REQUEST_ASYNC)) { NS_ASSERTION(mState & XML_HTTP_REQUEST_SYNCLOOPING, "Unexpected state"); do { PR_Sleep( PR_MillisecondsToInterval(10)); modalEventQueue->ProcessPendingEvents(); } while (mState & XML_HTTP_REQUEST_SYNCLOOPING); With either that or Darin's suggestion, sr=bzbarsky (note also that 10ms is probably better than 20ms here, unless you had a good reason to pick the 20ms number?).
I'll check this in with the changes that I suggested.
Created attachment 199966 [details] [diff] [review] v1 patch Here's the version of the patch that I commited on the trunk.
fixed-on-trunk
let's get this verified on the trunk before we consider taking this.
We've tested this fix with today's nightly build. It works perfect. No more hangs. Thanks for pushing this through, it would be a big help to have this in 1.5, as it's a big problem with our complex AJAX app on linux installs. -Kevin Zimbra
This fix is very low risk. I think we should take it for FF 1.5.
Yeah, this fix is dope. We should not be doing risk management by fix decimation on this particular fix. /be
Brendan: I'm not sure what you mean by "this fix is dope". Is that a good thing? "Dope" in the UK is marijuana. Is it OK to check it into the branch? If so, can you approve the patch? Gerv
"dope" is marijuana in the US too. And it's a slang term for "a good thing", generally... at least when one keeps in mind that Brendan is in California. ;)
I'm old school. http://dope.urbanup.com/1449519 /be
fixed1.8
or
