Bug 65214 - REGRESSION (Safari 5.1): JavaScript dialogs not usable with VoiceOver
Summary: REGRESSION (Safari 5.1): JavaScript dialogs not usable with VoiceOver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
: 65425 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-07-26 15:53 PDT by chris fleizach
Modified: 2011-08-01 15:48 PDT (History)
5 users (show)

See Also:


Attachments
patch (10.11 KB, patch)
2011-07-26 15:56 PDT, chris fleizach
andersca: review-
Details | Formatted Diff | Diff
patch (10.83 KB, patch)
2011-07-26 17:10 PDT, chris fleizach
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2011-07-26 15:53:42 PDT
* 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.
Comment 1 chris fleizach 2011-07-26 15:54:10 PDT
In WK2 JS alerts cause WebProcess to stall, making it unable to service AX requests
Comment 2 chris fleizach 2011-07-26 15:54:24 PDT
rdar://9601778
Comment 3 chris fleizach 2011-07-26 15:56:39 PDT
Created attachment 102068 [details]
patch
Comment 4 WebKit Review Bot 2011-07-26 15:59:16 PDT
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.
Comment 5 Anders Carlsson 2011-07-26 16:47:08 PDT
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.
Comment 6 chris fleizach 2011-07-26 17:10:52 PDT
Created attachment 102075 [details]
patch

Thanks for the review. I think this addresses your points
Comment 7 WebKit Review Bot 2011-07-26 17:12:42 PDT
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.
Comment 8 Adam Roben (:aroben) 2011-07-27 04:53:33 PDT
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.
Comment 9 chris fleizach 2011-07-27 08:53:40 PDT
(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
Comment 10 Alexey Proskuryakov 2011-07-27 10:41:33 PDT
Will this allow running arbitrary JavaScript code, loader callbacks etc. below an alert? That would certainly be a big problem.
Comment 11 chris fleizach 2011-07-27 11:20:57 PDT
(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
Comment 12 chris fleizach 2011-07-27 11:22:07 PDT
(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
Comment 13 Alexey Proskuryakov 2011-07-27 11:34:17 PDT
Anders reminded me that just like with WK1, it's still WebCore's job to defer loading, pause timers etc. while displaying modal windows.
Comment 14 Alexey Proskuryakov 2011-07-27 11:34:37 PDT
(which it's still doing for the most part, of course)
Comment 15 Anders Carlsson 2011-07-28 09:46:32 PDT
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".
Comment 16 chris fleizach 2011-07-28 10:01:06 PDT
(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".
Comment 17 chris fleizach 2011-07-28 10:39:58 PDT
http://trac.webkit.org/changeset/91939
Comment 18 chris fleizach 2011-08-01 15:48:03 PDT
*** Bug 65425 has been marked as a duplicate of this bug. ***