190313 - XMLExtras syncloading blocks UI (syncloadservice)

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

This is a regression from bug 166978 and bug 111614.

We need a way to NOT block the UI while still loading synchronously.

Comment 1

[Darin Fisher]

cc danm

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 3

[Alec Flett]

see bug 189528 for a way to make this better (not fix it, but make it
potentially faster, so that it blocks *less*)

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 225274 has been marked as a duplicate of this bug. ***

Comment 5

[Boris Zbarsky [:bzbarsky]]

See bug 225274 comment 1

Comment 6

[Joe Walp]

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

Comment 7

[Joe Walp]

Jonas, can you answer the question posed in comment 6?

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 212820 has been marked as a duplicate of this bug. ***

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

workaround - use asynchronous loading

Comment 11

[Boris Zbarsky [:bzbarsky]]

*** Bug 240678 has been marked as a duplicate of this bug. ***

Comment 12

[Heikki Toivonen (remove -bugzilla when emailing directly)]

*** Bug 273578 has been marked as a duplicate of this bug. ***

Comment 13

[matiyam]

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!

Comment 14

[Anand Aiyer]

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?

Comment 15

[Anand Aiyer]

Attachment: 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.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

(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.

Comment 17

[Kees van Ginkel]

> 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

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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.

Comment 19

[Kees van Ginkel]

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

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

(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...

Comment 21

[Kees van Ginkel]

> 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 

Comment 22

[Heikki Toivonen (remove -bugzilla when emailing directly)]

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 [e-mail] or [e-mail]. 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/

Comment 23

[Kees van Ginkel]

Attachment: same patch but created from latest codeline

This is the same patch as from Anand Aiyer. I created the patch with the latest
codeline.

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

doesn't this allow the caller of this method to be re-entered?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Yeah, this won't behave anything resembling "synchronous" from the caller's
perspective...

Comment 26

[Kees van Ginkel]

(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 ?

Comment 27

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 28

[Kees van Ginkel]

Attachment: Simple patch to this problem, no other event queue's

A simpler approach without the disadvantages of the last patch. I have tested
this patch in firefox, mozillasuite and the mfcEmbed application.

Comment 29

[Boris Zbarsky [:bzbarsky]]

See bug 273578 comment 17.

Comment 30

[Darin Fisher]

> 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.

Comment 31

[Darin Fisher]

This bug will be resolved by my patch for bug 326273.

Comment 32

[Peter Van der Beken [:peterv]]

Comment on attachment 197392 [details] [diff] [review]
Simple patch to this problem, no other event queue's

See comment 30.

Comment 33

[Rob Marshall [tH]]

*** Bug 339703 has been marked as a duplicate of this bug. ***

Comment 34

[Graham Leggett]

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.

Comment 35

[Darin Fisher]

This bug can be closed as fixed, now that bug 326273 is fixed.

Comment 36

[mike brenner]

Attachment: A workaround for Synchronous AJAX (and multiple calls)

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.

Comment 37

[Graham Leggett]

Yay, more browser specific kruft on the net :(

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment 36 has nothing to do with this bug.

Comment 42

[C William Underwood]

What about the possibility of a malicious server taking advantage of this behaviour?