Bug 125308 - AX: WK2: Safari extension installation hangs Safari and VoiceOver
Summary: AX: WK2: Safari extension installation hangs Safari and VoiceOver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-05 10:42 PST by chris fleizach
Modified: 2013-12-05 15:30 PST (History)
6 users (show)

See Also:


Attachments
patch (4.00 KB, patch)
2013-12-05 10:43 PST, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2013-12-05 10:42:02 PST
1-open safari and search for safari extension (any so far)
2-once downloaded, go to finder and open the ext file.

* EXPECTED RESULTS:
extension to install with the dialog in safari asking to confirm installation of the extension.

* ACTUAL RESULTS:
safari and voice over freeze. Difficult to get out
Comment 1 chris fleizach 2013-12-05 10:42:11 PST
<rdar://problem/15594097>
Comment 2 chris fleizach 2013-12-05 10:43:32 PST
Created attachment 218524 [details]
patch
Comment 3 Alexey Proskuryakov 2013-12-05 11:02:41 PST
This is super scary. With this patch, dispatchDecidePolicyForResponse will have entirely different behavior when accessibility is enabled.
Comment 4 Alexey Proskuryakov 2013-12-05 11:04:38 PST
If you can prove that it is safe to spin run loop here, I suggest making it unconditionally.

Otherwise, we might need a deeper change to ho extensions are installed.
Comment 5 chris fleizach 2013-12-05 11:23:24 PST
(In reply to comment #4)
> If you can prove that it is safe to spin run loop here, I suggest making it unconditionally.
> 
> Otherwise, we might need a deeper change to ho extensions are installed.

This seems not that much different from javascript alerts where we do

    unsigned syncSendFlags = (WebCore::AXObjectCache::accessibilityEnabled()) ? CoreIPC::SpinRunLoopWhileWaitingForReply : 0;

    WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunJavaScriptAlert(webFrame->frameID(), alertText), Messages::WebPageProxy::RunJavaScriptAlert::Reply(), m_page->pageID(), CoreIPC::Connection::NoTimeout, syncSendFlags);

That code has been in for 2 years, and we do have one case where it has proven to be unsafe (https://bugs.webkit.org/show_bug.cgi?id=123828) which I am very close to having a fix for.
Comment 6 Alexey Proskuryakov 2013-12-05 11:28:13 PST
Should we make it unconditional in that case?

Anders and Brady are really the ones who should make the call on whether it's acceptable design to spin the run loop here.
Comment 7 chris fleizach 2013-12-05 11:30:45 PST
(In reply to comment #6)
> Should we make it unconditional in that case?
> 

Not sure. I think Anders and Darin helped me resolve the javascript case in the past, so (echoing you) they'll probably be able to speak to that more.

Thanks

> Anders and Brady are really the ones who should make the call on whether it's acceptable design to spin the run loop here.
Comment 8 Anders Carlsson 2013-12-05 15:00:02 PST
Comment on attachment 218524 [details]
patch

I think this looks good.
Comment 9 WebKit Commit Bot 2013-12-05 15:30:18 PST
Comment on attachment 218524 [details]
patch

Clearing flags on attachment: 218524

Committed r160197: <http://trac.webkit.org/changeset/160197>
Comment 10 WebKit Commit Bot 2013-12-05 15:30:21 PST
All reviewed patches have been landed.  Closing bug.