Closed Bug 190313 Opened 22 years ago Closed 18 years ago

XMLExtras syncloading blocks UI (syncloadservice)

Categories

(Core :: XML, defect)

Product:
Core
Component:
XML
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: hjtoi-bugzilla, Unassigned)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: workaround - use asynchronous loading)

Attachments

(3 files, 1 obsolete file)

This seems to do the trick
1.41 KB, patch
Details | Diff | Splinter Review
Simple patch to this problem, no other event queue's
1.20 KB, patch
peterv
: review-
Details | Diff | Splinter Review
A workaround for Synchronous AJAX (and multiple calls)
3.50 KB, text/javascript
Details 
This is a regression from bug 166978 and bug 111614.

We need a way to NOT block the UI while still loading synchronously.
cc danm
Testcase in URL does a synchronous post, where the the POST response has a 5
second delay so you have some time to detect that it really is blocking the UI.
URL: http://green.mcom.com/heikki/syncpost...
see bug 189528 for a way to make this better (not fix it, but make it
potentially faster, so that it blocks *less*)
QA Contact: rakeshmishra → ashishbhatt
*** Bug 225274 has been marked as a duplicate of this bug. ***
I notice that revising XMLHttpRequest to use SyncLoadService [1] (proposed in 
bug 166978 comment 5 and tracked in bug 189513) has been planned for some 
time. And I gather also from bug 166978 comment 5 that 
SyncLoadService.LoadDocument(..) [2] blocks either UI event processing or UI 
painting.  Who should I contact regarding whether there's a plan to add (to 
SyncLoadService) a method that doesn't block?

[1]
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsSyncLoadService.cpp
[2]
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsSyncLoadService.cpp#
536
Jonas, can you answer the question posed in comment 6?
Nowadays you can load from a normal http-channel syncronously (using
nsIChannel::Open) without blocking the UI so that's the method that should be used.

The syncload service should either be removed or rewritten to use sycrounyous
loading.
*** Bug 212820 has been marked as a duplicate of this bug. ***
workaround - use asynchronous loading
Whiteboard: workaround - use asynchronous loading
*** Bug 240678 has been marked as a duplicate of this bug. ***
Depends on: 189513
Whiteboard: workaround - use asynchronous loading → [good first bug]workaround - use asynchronous loading
Summary: XMLExtras syncloading blocks UI → XMLExtras syncloading blocks UI (syncloadservice)
*** Bug 273578 has been marked as a duplicate of this bug. ***
Is there any progress on this bug?

My employer is interested in getting it fixed. He could set a bounty, make a
donation, or something like that.

If anyone is interested, drop me an email.

Thanks!
Blocks: 273578
Why not instead of creating a new Event Queue and processing that until the 
document loads to just process the Current Event Queue there itself until the 
request is complete?
This seems to work. I am not sure if there are any problems by doing this. This
probably wont work in an embedded enviroment.
(In reply to comment #15)
> Created an attachment (id=190245) [edit]
> This seems to do the trick
> 
> This seems to work. I am not sure if there are any problems by doing this. This
> probably wont work in an embedded enviroment.

Then please test it. The reason things are now as they are because it was/is(?)
the only way to make it work both in the regular app and embedding case.
Previously things worked without blocking UI, but did not work at all in
embedding case.
> Then please test it. The reason things are now as they are because it was/is(?)
> the only way to make it work both in the regular app and embedding case.
> Previously things worked without blocking UI, but did not work at all in
> embedding case.

I build a new firefox with this patcvh and that works.

I would like to test it, but can you give me some hints on the complete test ?
What do I need to test before it is seen as a good fix ? How can I test embedded ?

I am new to the mozilla bug and defect fixing, I read some information on
http://www.mozilla.org/quality/ but found no reference to testing the embedded case.

Kees

I am not totally sure what to do nowadays, but I would start by trying out the
various test programs under the mozilla/embedding directory. Try at least
tests/mfcembed and tests/winEmbed. Use these test browsers to try out both
synchronous and asynchronous XMLHttpRequest tests. Without your changes both
async and sync cases should work. I suspect with your changes one or both kind
of tests will fail (either nothing retrieved, or the application hangs). If they
work even with your changes, then I would suggest you seek some help from the
embedding people (post to netscape.public.mozilla.embedding, or check on IRC) to
really verify that your changes won't break the embedding cases.
OK, Thanks.
I have tried the winEmbed program and tested all tests including some new ones
on my own machine. It doesn't break. I will try the embedding people later today.

Kees
(In reply to comment #19)
> I have tried the winEmbed program and tested all tests including some new ones
> on my own machine. It doesn't break. I will try the embedding people later today.

Great!

Please also test mfcEmbed (or ask someone else test it if you can't build it). I
believe that was the one I used long time ago...

> Please also test mfcEmbed (or ask someone else test it if you can't build it). 

I have build and tested the mfcembed, not a single error. What is next. Who can
put this fix in the codeline ? I really like to see this one fixed in the next
release.

I have posted a mail in the netscape.public.mozilla.embbedding newsgroup but i
got no reaction.

Kees 


Reassigning to Kees since he's done the work.

Kees: The next step is to get reviews for your patch. I am not sure who would be
the best to review this particular patch, but I would suggest starting with
either peterv@propagandism.org or jst@mozilla.org. Klick the Edit link on the
attachment above, then change the review drop-down to a '?' and put in the
requestee email (either of the ones I recommended). One or more reviews may be
required. Once you have all the reviews you can ask one of the reviewers to
check the patch in.

To read more about the process, read: http://www.mozilla.org/hacking/
Assignee: hjtoi-bugzilla → kvginkel
This is the same patch as from Anand Aiyer. I created the patch with the latest
codeline.
Attachment #196312 - Flags: review?(peterv)
doesn't this allow the caller of this method to be re-entered?
Yeah, this won't behave anything resembling "synchronous" from the caller's
perspective...
(In reply to comment #24)
> doesn't this allow the caller of this method to be re-entered?

Can you explain that ? The thread is blocked for the javascript caller. The 100%
cpu problem is gone. Apart from that there is no difference with the before
situation or am i missing something ?




> The thread is blocked for the javascript caller.

It's not -- existing events that were posted in that event queue (or even new
ones that get posted there) will get processed.  Which means that timeouts can
fire, reflows can happen, etc, etc.  Which should not be happening during a
"synchronous" operation...

Granted, some of these problems existed even with the code as it is.  But this
patch exacerbates them.
A simpler approach without the disadvantages of the last patch. I have tested
this patch in firefox, mozillasuite and the mfcEmbed application.
Attachment #196312 - Attachment is obsolete: true
Attachment #197392 - Flags: review?(peterv)
> Created an attachment (id=197392) [edit]

This patch will hang the UI thread of the browser until a network request
completes (WaitForEvent blocks on a monitor and will not be woken up by UI
activity).  The user will not be able to stop the page to recover control over
their browser.  I don't think this is a good change.

By the way, this bug is so not "[good first bug]" material.  The proper solution
is very complex, and it involves dealing with a lot of stuff across the codebase.

In my opinion, a synchronous XMLHttpRequest.send call made on the UI thread
should be treated more like a modal dialog.  When we show a window.alert, we
suppress events in the parent window.  XMLHttpRequest.send need not be treated
much different.  I think we should suppress all events in the window invoking
XMLHttpRequest.send and permit the browser's STOP button to function as a way to
abort the load (much like when an alert dialog is closed).

See also bug 309424.
Whiteboard: [good first bug]workaround - use asynchronous loading → workaround - use asynchronous loading
Attachment #196312 - Flags: review?(peterv)
Assignee: kvginkel → xml
This bug will be resolved by my patch for bug 326273.
Depends on: nsIThreadManager
Blocks: 277056
Comment on attachment 197392 [details] [diff] [review]
Simple patch to this problem, no other event queue's

See comment 30.
Attachment #197392 - Flags: review?(peterv) → review-
*** Bug 339703 has been marked as a duplicate of this bug. ***
I would like to request that the status whiteboard "workaround - use asynchronous loading" be removed.

Trying to use asynchronous requests in javascript code with a heavy requirement for synchronous behaviour uses the exact same krufty methods to do so as existing workarounds for bugs in IE6.
This bug can be closed as fixed, now that bug 326273 is fixed.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Firefox does NOT call the onreadystatechange event handler if the AJAX request is synchronous. However, it will call the Firefox-only onload event handler either way. This also takes care of multiple AJAX calls.
Yay, more browser specific kruft on the net :(
Comment 36 has nothing to do with this bug.
URL: http://green.mcom.com/heikki/syncpost... → http://dev.wulong.name/xmlhttprequest...
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9alpha
What about the possibility of a malicious server taking advantage of this behaviour?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: