Bug 65214 – REGRESSION (Safari 5.1): JavaScript dialogs not usable with VoiceOver

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

* STEPS TO REPRODUCE:
1) GO to any website with a dialog like this one.
2) Enter whatever is required to make it appear.

* EXPECTED RESULTS:
VoiceOver should read the dialogs.

* ACTUAL RESULTS:
VoiceOver seems to think Safari is freezing, and maybe it is. You cannot navigate the dialog or get any information.

[chris fleizach]

In WK2 JS alerts cause WebProcess to stall, making it unable to service AX requests

[chris fleizach]

rdar://9601778

[chris fleizach]

Created attachment 102068 [details]
patch

[WebKit Review Bot]

Attachment 102068 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Platform/CoreIPC/Connection.h:212:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:213:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 102068 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102068&action=review

Overall, I really like this patch; this is pretty much how I'd have implemented it myself. Great job, Chris! I'll r- it so you can upload a new patch with the suggested changes.

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
> +#if PLATFORM(MAC)
> +            RunLoop::current()->runForDuration(.25);
> +            timeout = currentTime() >= absoluteTime;
>  #endif

It looks like this will do a busy-wait and wake up the CPU 4 times a second when we're in this state.

Fixing this is tricky - we'd need a way to know when a nested run loop is running so we can quit it when we time out or receive a response.
I think for now you could just run the run loop forever and add a FIXME to fix this (since this is only used with messages that have an infinite timeout).

>> Source/WebKit2/Platform/CoreIPC/Connection.h:212
>> +    PassOwnPtr<ArgumentDecoder> sendSyncMessage(MessageID, uint64_t syncRequestID, PassOwnPtr<ArgumentEncoder>, double timeout, bool continueRunLoop = false);
> 
> Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]

Instead of using a boolean here, it would be better to add a flags enum, similar to the MessageSendFlags enum we have for async messages. You could call it SyncMessageSendFlags and use an unsigned flags = 0 instead of the boolean parameter.

[chris fleizach]

Created attachment 102075 [details]
patch

Thanks for the review. I think this addresses your points

[WebKit Review Bot]

Attachment 102075 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Platform/CoreIPC/Connection.h:217:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebKit2/Platform/CoreIPC/Connection.h:218:  Local variables should never be PassOwnPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Roben (:aroben)]

Comment on attachment 102075 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102075&action=review

> Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
> +            RunLoop::current()->runForDuration(1e10);

Won't this allow timers to fire, other messages to be processed, etc.? That doesn't seem good.

[chris fleizach]

(In reply to comment #8)
> (From update of attachment 102075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102075&action=review
> 
> > Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
> > +            RunLoop::current()->runForDuration(1e10);
> 
> Won't this allow timers to fire, other messages to be processed, etc.? That doesn't seem good.

It will, but that's also part of the idea, that Accessibility messages need to be processed while waiting, otherwise accessibility clients will hang.

This is also what happens in WK1

[Alexey Proskuryakov]

Will this allow running arbitrary JavaScript code, loader callbacks etc. below an alert? That would certainly be a big problem.

[chris fleizach]

(In reply to comment #10)
> Will this allow running arbitrary JavaScript code, loader callbacks etc. below an alert? That would certainly be a big problem.

i suspect not because because the web page is still blocked waiting for it's work to finish

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > Will this allow running arbitrary JavaScript code, loader callbacks etc. below an alert? That would certainly be a big problem.
> 
> i suspect not because because the web page is still blocked waiting for it's work to finish

(In reply to comment #10)
> Will this allow running arbitrary JavaScript code, loader callbacks etc. below an alert? That would certainly be a big problem.

Also, any other ideas on how to make this work are welcome

[Alexey Proskuryakov]

Anders reminded me that just like with WK1, it's still WebCore's job to defer loading, pause timers etc. while displaying modal windows.

[Alexey Proskuryakov]

(which it's still doing for the most part, of course)

[Anders Carlsson]

Comment on attachment 102075 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=102075&action=review

>>> Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
>>> +            RunLoop::current()->runForDuration(1e10);
>> 
>> Won't this allow timers to fire, other messages to be processed, etc.? That doesn't seem good.
> 
> It will, but that's also part of the idea, that Accessibility messages need to be processed while waiting, otherwise accessibility clients will hang.
> 
> This is also what happens in WK1

Since we only ever call this with a big value, we could just use RunLoop::run() here and not add runForDuration at all.

> Source/WebKit2/Platform/CoreIPC/Connection.h:67
> +    SpinRunLoopWhileWaitingForReplay = 1 << 0,

Typo, "Replay" instead of "Reply".

[chris fleizach]

(In reply to comment #15)
> (From update of attachment 102075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102075&action=review
> 
> >>> Source/WebKit2/Platform/CoreIPC/Connection.cpp:477
> >>> +            RunLoop::current()->runForDuration(1e10);
> >> 
> >> Won't this allow timers to fire, other messages to be processed, etc.? That doesn't seem good.
> > 
> > It will, but that's also part of the idea, that Accessibility messages need to be processed while waiting, otherwise accessibility clients will hang.
> > 
> > This is also what happens in WK1
> 
> Since we only ever call this with a big value, we could just use RunLoop::run() here and not add runForDuration at all.
> 

If we do that, then run() ends up calling [NSApplication run] which does not return. runForDuration uses CFRunLoopRunInMode which will return back each time it handles an event so that we don't continue adding run loops on top of each other




> > Source/WebKit2/Platform/CoreIPC/Connection.h:67
> > +    SpinRunLoopWhileWaitingForReplay = 1 << 0,
> 
> Typo, "Replay" instead of "Reply".

[chris fleizach]

http://trac.webkit.org/changeset/91939

[chris fleizach]

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