333198 - Suppress Input events for web content during synchronous XMLHttpRequest loads

Status: RESOLVED FIXED

(Core :: XML, defect, P1)

Description

[Darin Fisher]

Suppress Input events for web content during synchronous loads

Comment 1

[Darin Fisher]

-> reassign to default owner

Comment 5

[Jesse Ruderman]

See also bug 340345, DOM timeouts can fire during a sync XMLHttpRequest.

Comment 6

[Brendan Eich [:brendan]]

From bug 340345 it seems this should be assigned and marked a blocker too.

/be

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Blocking, though weakly. Smaug: please update this bug with an ETA and risk assessment ASAP?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I'll try to write the patch tomorrow.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

The nsXMLHttpRequest::Send parts of the patch require bug 340345,
but since that patch isn't quite ready, this needs to wait a bit too.

This includes fix to not dispatch progress events when XHR is synchronous.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Is that really the right thing to do with progress events I can see wanting progress events in a worker to update a progress bar on the UI thread, no?  What does the spec say here?

What about onReadyStateChange events?  See bug 313646.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

It is the right thing per the spec.
If worker thread wants to update UI, don't use sync XHR.

Actually, is worker thread ever using sync XHR internally?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

And readystatechange is a diffent thing. It is a legacy event.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

http://mxr.mozilla.org/mozilla-central/source/dom/src/threads/nsDOMWorkerXHRProxy.cpp?mark=871-874#865

Comment 14

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Carry on, then.  ;)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

I need to handle focus/blur somehow.
Better patch coming ...

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 with blur handling

Depends on bug 340345.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Hmm, where did one version of the patch go? Perhaps accidentally to some
wrong bug.

Anyway, this is tested when focus moves between moz windows, or moz and non-moz
windows or between chrome and content.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Something for manual testing

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 362118 [details] [diff] [review]
v2 with blur handling

I think this looks good. Please go ahead and land this on the trunk asap. I'd like bz to look this over once too before we take this for 1.9.1, but getting mileage on this on the trunk ASAP is very valuable, so feel free to land on trunk before bz has a chance to review.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Some questions:

1) Why suppress on only the primary shell, not on all shells?  It's probably ok,
   but just checking.
2) Why can't EventHandlingSuppressed() just be passed the relevant prescontext?
   Or does gLastFocusedPresContextWeak possibly not match gLastFocusedDocument?
3) If the script happens to reframe a subframe of the document during the sync
   load (e.g. off a onreadystatechange callback), that presshell won't have
   event handling suppressed as far as I can tell.  That should probably be
   fixed.
4) UnsuppressEventHandling() should probably call MaybeFireSuppressedEvents()
   on |this| and should be calling UnsuppressEventHandling() on the kids.  The
   current code just looks wrong.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #20)
> Some questions:
> 
> 1) Why suppress on only the primary shell, not on all shells?  It's probably
> ok,
>    but just checking.
Because I don't care printing here and print preview isn't really interactive
(excluding scrolling).

> 2) Why can't EventHandlingSuppressed() just be passed the relevant prescontext?
>    Or does gLastFocusedPresContextWeak possibly not match gLastFocusedDocument?

I don't understand the first question and the answer to the second one is yes.

> 3) If the script happens to reframe a subframe of the document during the sync
>    load (e.g. off a onreadystatechange callback), that presshell won't have
>    event handling suppressed as far as I can tell.  That should probably be
>    fixed.
Indeed. When presshell is created it should inherit its parent shell's
event suppression.


> 4) UnsuppressEventHandling() should probably call MaybeFireSuppressedEvents()
>    on |this| and should be calling UnsuppressEventHandling() on the kids.  The
>    current code just looks wrong.
Why does it look wrong? I'm trying to avoid the case when calling 
EnumerateSubDocuments could add or remove sub documents.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #20)
> 3) If the script happens to reframe a subframe of the document during the sync
>    load (e.g. off a onreadystatechange callback), that presshell won't have
>    event handling suppressed as far as I can tell.  That should probably be
>    fixed.
Btw,  we don't fire readystatechange events, and with this patch progress events either during sync load.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #21)
> (In reply to comment #20)
> > Some questions:
> > 
> > 1) Why suppress on only the primary shell, not on all shells?  It's probably
> > ok,
> >    but just checking.
> Because I don't care printing here and print preview isn't really interactive
> (excluding scrolling).
And scripts are disabled when print preview is active.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Btw,  we don't fire readystatechange events,

Yes, but we need to fix that.

> Why does it look wrong? 

Say XHR calls UnsuppressEventHandling on the top shell.  I'd missed that we add this top shell to the array manually, so that's ok.  Right now that will:

1) Call MaybeFireSuppressedEvents on the top shell.
2) Call MaybeFireSuppressedEvents on all the kids of the top shell.
3) Do nothing with grandkids and lower descendants of the top shell.

That looks wrong because it seems like we should enter MaybeFireSuppressedEvents() for every presshell for which we entered SuppressEventHandling(), if only so that we'll decrement the counter.  The current patch permanently disables events in grandkids, as far as I can tell.  That should even be mochitestable.

Perhaps GetSubShells should recurse or something?  Or better yet we should walk down our array as we build it, adding further subshells to the list.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Oh, as far as multiple presshells my point is we shouldn't assume that it's only used for printing/print-preview, unless we made some arch decision I missed...

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #24)
> 1) Call MaybeFireSuppressedEvents on the top shell.
> 2) Call MaybeFireSuppressedEvents on all the kids of the top shell.
> 3) Do nothing with grandkids and lower descendants of the top shell.
Ah, indeed.

> Perhaps GetSubShells should recurse or something?
That would be the easiest way, I guess.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for testing

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better

suppresses all presshells and handles bfcache and new iframes etc.
But, I realized there is a problem with plugins.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

About plugins, see also Bug 340345 comment 79

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Chrome blocks page UI etc, but doesn't block plugin UI.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

So since Opera and Chrome don't block plugins' UI, perhaps we don't have to either.

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 362381 [details] [diff] [review]
better

Or not this particular patch, but something I'm about to upload.

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 362381 [details] [diff] [review]
better

Er, this is the one after all.
Boris, what you think about this? Moved event suppression counter to document so that all the presshells can check it.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 362381 [details] [diff] [review]
better

Bah, this was wrong after all. Missing the relevant Suppress call.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Attachment: this one

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Tryserver builds https://build.mozilla.org/tryserver-builds/2009-02-16_05:[e-mail]-sync_xhr_2/

Comment 37

[robinpelgrim]

I took a quick look at the patch and I noticed the following:

(Correct me if I am wrong ;) )

In PresShell::HandleEvent most events are not suspended but suppressed(keyup/keydown etc.), I think they should be delayed i.s.o. ignored (as in IE /chrome etc.)

further more I noticed that
suspendedDoc->UnsuppressEventHandling() is called in nsXMLHttpRequest::Send 

UnsuppressEventHandling() finally calls FireDelayedEvents, so that means that events (focus/blur) are still fired during a sync XHR.
I think they should be called when the calling javascript is completed (as in IE /chrome etc.)

I more or less expected an event queue for delayed events but I couldn't find it.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #37)
> In PresShell::HandleEvent most events are not suspended but
> suppressed(keyup/keydown etc.), I think they should be delayed i.s.o. ignored
> (as in IE /chrome etc.)
This bug is "Suppress Input events for web content during synchronous XMLHttpRequest loads".

> UnsuppressEventHandling() finally calls FireDelayedEvents, so that means that
> events (focus/blur) are still fired during a sync XHR.
> I think they should be called when the calling javascript is completed (as in
> IE /chrome etc.)
Well, Chrome, Safari, and Opera fire blur after load, and so does the patch.
But sure, firing blur could be delayed to happen after send() returns.

Comment 39

[robinpelgrim]

(In reply to comment #38)
> (In reply to comment #37)
> > In PresShell::HandleEvent most events are not suspended but
> > suppressed(keyup/keydown etc.), I think they should be delayed i.s.o. ignored
> > (as in IE /chrome etc.)
> This bug is "Suppress Input events for web content during synchronous
> XMLHttpRequest loads".
> 

I see. But Chrome, IE etc, do suspend the events. I also think that is what a user expects, now events are 'eaten' by sync XHR.

> > UnsuppressEventHandling() finally calls FireDelayedEvents, so that means that
> > events (focus/blur) are still fired during a sync XHR.
> > I think they should be called when the calling javascript is completed (as in
> > IE /chrome etc.)
> Well, Chrome, Safari, and Opera fire blur after load, and so does the patch.
> But sure, firing blur could be delayed to happen after send() returns.

That is not what I am trying to tell:
in the following function the blur/focus event can be fired between sendSync1 and sendSync2(when javascript has not run to completion yet).
function doTwoSyncXHR() {
sendSync1
sendSync2
}
In chrome/IE etc. they fire when javascript has run to completion. That's what a javascript developer expects (I think).

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #39)
> I see. But Chrome, IE etc, do suspend the events. I also think that is what a
> user expects, now events are 'eaten' by sync XHR.
For 1.9.1 suspending events isn't easily doable, IMO.
And it would need more testing, like what do IE/Chrome do if readystatechanged listeners change focused element?

> In chrome/IE etc. they fire when javascript has run to completion. That's what
> a javascript developer expects (I think).
Right. And I'm just about to upload an updated patch which brings the behavior you want ;)

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

Attachment: async unsuppress

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

For testing I've used for example
http://mozilla.pettay.fi/xhr_upload/xhr_upload_demo_sync.html , but note, I may
edit that test at any time.

Comment 43

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

I'm not at all convinced that we want to suspend rather than suppress. When I'm interacting with an app and clicking something does not immediately respond I tend to click around a few things to see if it's just that function that is broken, or if the whole app is. If all those clicks come crashing down on me in a rapid succession a bit later that is unlikely what I want.

Especially since things might move around which cause the clicks that I did turn into clicks on completely unrelated buttons. With consequences possibly as bad as data loss.

Things are possibly not as bad for keyboard events since at least they will be targetting the same element as the UI indicated, i.e. the focused element. But things like enter and tab can cause just as bad things to happen.

If we want to talk about suspending, lets do that in a different bug. Especially since we need to minimize risk here, given that this patch is going in so late in the game.

Comment 44

[robinpelgrim]

(In reply to comment #43)
> I'm not at all convinced that we want to suspend rather than suppress. When I'm
> interacting with an app and clicking something does not immediately respond I
> tend to click around a few things to see if it's just that function that is
> broken, or if the whole app is. If all those clicks come crashing down on me in
> a rapid succession a bit later that is unlikely what I want.
> 
> Especially since things might move around which cause the clicks that I did
> turn into clicks on completely unrelated buttons. With consequences possibly as
> bad as data loss.
> 
> Things are possibly not as bad for keyboard events since at least they will be
> targetting the same element as the UI indicated, i.e. the focused element. But
> things like enter and tab can cause just as bad things to happen.
> 
> If we want to talk about suspending, lets do that in a different bug.
> Especially since we need to minimize risk here, given that this patch is going
> in so late in the game.

I agree for this stage.

Comment 45

[Johnny Stenback (:jst)]

Comment on attachment 362749 [details] [diff] [review]
async unsuppress

- In nsXMLHttpRequest::Send():

+    if (suspendedDoc) {
+      NS_DispatchToCurrentThread(
+        NS_NEW_RUNNABLE_METHOD(nsIDocument, suspendedDoc,
+                               UnsuppressEventHandling));
+    }

Do we need to worry about the case where someone does in script:

  xhr1.send(sync=true)
  alert("foo")

In that case we'll fire the suppressed events when the modal dialog opens, I think.

Comment 46

[Boris Zbarsky [:bzbarsky]]

smaug, any chance of an interdiff here?  It seems like that would be a lot easier to review.... and bugzilla's refusing to provide one.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #45) 
> In that case we'll fire the suppressed events when the modal dialog opens, I
> think.
Ah right. Tricky. I wonder if alert should suppress events too.

(In reply to comment #46)
> smaug, any chance of an interdiff here?
I'm not sure what kind of interdiff would help with reviewing.

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #47)
> (In reply to comment #45) 
> > In that case we'll fire the suppressed events when the modal dialog opens, I
> > think.
> Ah right. Tricky. I wonder if alert should suppress events too.
Or perhaps not. I could add some stack object to automatically suppress UI events
while alert/confirm/prompt/any-modal-window is open.
Need to figure out the right place for that, so that embedding doesn't break.

Comment 49

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what kind of interdiff would help with reviewing.

One between the patch I already reviewed and the current one, unless it's just all different...

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 362749 [details] [diff] [review]
async unsuppress

I'm about to upload a new patch which handles the alert case too.

Comment 51

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +handels modal dialogs

Comment 52

[Brendan Eich [:brendan]]

Wow, I've been waiting years for modals to block events. We lost Netscape parity  (IE and other browsers too) way back when (there may be a bug on it to dup fwd).

/be

Comment 53

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

I *think* that this doesn't block *all* events. Things like XHR events and pending <script>s will still fire. Same thing for localStorage and database callbacks once we have those.

But I think this patch covers 95% of the important cases. And we can always block the other ones on a case by case basis if it becomes important.

Comment 54

[Jeff Walden [:Waldo]]

nsIThreadInternal.pushEventQueue is *almost* what you want here, as far as I can tell, except that it doesn't filter/run already-pending events, only new events.  That seems on its face like it could be a reasonably small addition to nsIThreadInternal to push a filter that could recognize content-generated script events and queue them up until whatever blocking event (sync XHR, modal dialog, etc.) completed.  Post-191 material in any case if this idea is even implementable, threads and our use of them being what they are...

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 363350 [details] [diff] [review]
+handels modal dialogs

Yet another small change coming. I'll move the nsDocumentViewer change to a better place.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v8

Comment 57

[Boris Zbarsky [:bzbarsky]]

Pushing an event queue will wreak havoc with existing in-progress necko loads and the like.  It's not to be done lightly.

Comment 58

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 363691 [details] [diff] [review]
v8

>+++ b/content/base/public/nsIDocument.h
>+   * Prevents user initiated events to be dispatched to the document and

 ... events from being dispatched ...

Looks good otherwise!  r=bzbarsky

Comment 59

[Ginn Chen]

It was pushed by someone
http://hg.mozilla.org/mozilla-central/rev/7184f7d69ff0

I pushed a followup fix for Solaris bustage
http://hg.mozilla.org/mozilla-central/rev/84a3bd5c760a

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch + comment fix + solaris fix

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

I'll push this to 1.9.1

Comment 62

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.9.1

Comment 63

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.9.1

Comment 64

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6c0e2a2167c5

Comment 65

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Smaug, please use the fixed1.9.1 keyword when checking in patches on 1.9.1 branch. Thanks.

Comment 66

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backout patch for 1.9.1

Comment 67

[Olli Pettay [:smaug][[e-mail]]]

Attachment: backout patch for trunk

Comment 68

[Johnny Stenback (:jst)]

Marking this a P1, as it blocks P1 blocker bug 479490.

Comment 69

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/b095eaf3e8b4

Comment 70

[Mike Beltzner [:beltzner, not reading bugmail]]

and on branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/af9654f48010

Comment 71

[Henrik Skupin [:whimboo][⌚️UTC+1]]

So it was backed out on trunk and 1.9.1 too. Will the initial issue be fixed on another bug?

Comment 72

[Olli Pettay [:smaug][[e-mail]]]

This was backed out and then relanded with the fix for bug 480767.

Comment 73

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Smaug. Can we get an automated test for this fix?

Comment 75

[Fernando Colombo]

I performed a test for bug 491571 (marked as duplicate of this bug) on Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4.

The behavior is still different from Safari, Chrome, IE, etc. FireFox is suppressing events, which may cause typographical errors (data loss?) on text input and text areas. I think the correct behavior would be queueing such events. There's a test case on bug 491571 for this.

About alert or any open-dialog-box from the event handler, after XHR send(): Yeah, in this case the queued events must be suppressed. Windows has PeekMessage with PM_REMOVE|PM_QS_INPUT, but I don't know how do you wrap Windows.

Comment 76

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #75)
 > The behavior is still different from Safari, Chrome, IE, etc. 
FYI, all the browsers do this in a bit different way. Even Safari and Chrome
behave differently for example when alert() or plugins are used.

Comment 77

[Fernando Colombo]

(In reply to comment #76)
> FYI, all the browsers do this in a bit different way. Even Safari and Chrome
> behave differently for example when alert() or plugins are used.

I'm just worried about the data-loss issue that can arise from quietly suppressing keyboard events. Imagine a user typing a memo in a text area. While he/she types, the client must make some requests to the server, and because we only target fast servers on fast connections, we decided to use synchronous XHR. Suppressing keyboard events might produce typos that will be stored and perceived too late, breaking future text searches or even causing more serious problems.

As JavaScript developers, this issue will force us to switch from synchronous to asynchronous, which greatly increases complexity as we need to code event queues and event flushing in JavaScript. Although this will work on any browser, only FireFox is forcing us to do this, from the list of browsers we wish to support.

IMHO, keyboard events must never be suppressed due to slow response (which seems to be the case). Most people are somehow aware of a "keyboard buffer", and so, during slow-response periods, the user simply waits for typed letters to appear. Also, most people accept loosing key presses when the system breaks the typing flow, such as when showing a dialog box.

Well, that's just an opinion from a somewhat (12 yrs) experienced UI developer...

Comment 78

[Olli Pettay [:smaug][[e-mail]]]

FYI, at least on OSX Safari does not queue keyboard events when sync XHR is used.

Comment 79

[Fernando Colombo]

(In reply to comment #78)
> FYI, at least on OSX Safari does not queue keyboard events when sync XHR is
> used.

Which version o OSX / Safari are you mentioning? We tested testcase of bug 491571 on:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; pt-br) AppleWebKit/525.27.1 (KHTML, like Gecko) Version/3.2.1 Safari/525.27.1

And it works like Safari on Windows, IE, Chrome, etc: all keyboard events are queued (none is lost) during a sync XHR. A think mouse events are queued too, and it doesn't seem to be related to sync XHR. It seems that while JavaScript code is running, user events on that page are just queued, like an ordinary single-threaded GUI application. Because a sync XHR blocks the thread until the server responds or timeout, events are naturally queued there.

Hence, handling of all events are done in an ordered, serialized way. No event is suppressed and there is no context-switching, even during blocking operations. IMHO, this enhances robustness and reliability of complex pages. If the page author wants to deal with server slowness, there's the option of using async XHR.

Comment 80

[Olli Pettay [:smaug][[e-mail]]]

I was testing some nightly Webkit+Safari. And btw, at least Safari
breaks keydown/press/up order when used with sync XHR.

If you want event queue, please open a new bug (and CC me).
This bug is about suppression.