XMLHttpRequest with async = false takes 100% CPU until request finished

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
XML
P2
major
RESOLVED FIXED
13 years ago
10 years ago

People

(Reporter: matiyam, Assigned: Darin Fisher)

Tracking

({fixed1.8, perf, testcase})

Trunk
mozilla1.8rc1
x86
All
fixed1.8, perf, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8rc1 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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 ***
Status: UNCONFIRMED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → DUPLICATE
(Reporter)

Comment 2

13 years ago
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?

Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
(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.
Depends on: 190313

Comment 4

13 years ago
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

Comment 5

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

Comment 7

13 years ago
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.

Updated

13 years ago
Keywords: perf
OS: Linux → All
(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.

Comment 9

13 years ago
Created attachment 172403 [details] [diff] [review]
a testcase
Depends on: 277056

Comment 10

12 years ago
(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

Updated

12 years ago
Attachment #172403 - Attachment is obsolete: true

Comment 11

12 years ago
Created attachment 175905 [details]
testcase

Updated

12 years ago
Attachment #175905 - Attachment is obsolete: true

Comment 12

12 years ago
Created attachment 175907 [details]
testcase

Comment 13

12 years ago
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....

Comment 15

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

Comment 17

12 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 18

12 years ago
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.

Updated

12 years ago
Keywords: testcase
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.
(Assignee)

Comment 21

12 years ago
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? ;-)
Assignee: xml → darin
Severity: normal → major
Priority: -- → P2
Target Milestone: --- → mozilla1.8beta2
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.

Comment 23

12 years ago
Is it dupe of bug 190313?
no
(Assignee)

Comment 25

12 years ago
This is related to bug 309424
(Assignee)

Updated

12 years ago
Target Milestone: mozilla1.8beta2 → mozilla1.8beta5
(Assignee)

Updated

12 years ago
Flags: blocking1.8rc1?

Comment 26

12 years ago
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.

Comment 28

12 years ago
it's too late for anything but low-risk fixes. the time to take a fix for
something like this was months ago. 
Flags: blocking1.8rc1? → blocking1.8rc1-
(Assignee)

Updated

12 years ago
Target Milestone: mozilla1.8beta5 → mozilla1.9alpha

Comment 29

12 years 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 ?

Comment 30

12 years ago
Created attachment 199529 [details] [diff] [review]
simple fix (workaround) with 10 millisecond sleep in the loop
Attachment #199529 - Flags: review?(peterv)
(Assignee)

Comment 31

12 years ago
See also my patch in bug 309424.  Perhaps we should combine these two patches.

Comment 32

12 years ago
(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 ?

Updated

12 years ago
Attachment #199529 - Flags: review?(peterv) → review?(jst)
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
Attachment #199529 - Flags: superreview?(bzbarsky)
Attachment #199529 - Flags: review?(jst)
Attachment #199529 - Flags: review+
(Assignee)

Comment 34

12 years ago
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?).
Attachment #199529 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 36

12 years ago
I'll check this in with the changes that I suggested.
(Assignee)

Comment 37

12 years ago
Created attachment 199966 [details] [diff] [review]
v1 patch

Here's the version of the patch that I commited on the trunk.
Attachment #199966 - Flags: approval1.8rc1?
(Assignee)

Comment 38

12 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED

Comment 39

12 years ago
let's get this verified on the trunk before we consider taking this. 

Comment 40

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

Comment 41

12 years ago
This fix is very low risk.  I think we should take it for FF 1.5.
Target Milestone: mozilla1.9alpha → mozilla1.8rc1
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

Updated

12 years ago
Attachment #199966 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 46

12 years ago
fixed1.8
Keywords: fixed1.8
Blocks: 218908
You need to log in before you can comment on or make changes to this bug.