Bug 181331 – VoiceOver does not work when the WebContent process is using NSRunLoop.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

When NSRunLoop is used in the WebContent process, accessibility must be initialized for VoiceOver to work.

[Per Arne Vollan]

Created attachment 330558 [details]
Patch

[Brent Fulgham]

Comment on attachment 330558 [details]
Patch

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

This looks good, but I'm sad we have to instantiate an NSApp instance. We should see if the accessibility team can live without that change (but that's not relevant to this patch).

> Source/WebKit/Configurations/WebContentService.xcconfig:56
> +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain;

Oh, nice!

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:302
> +    NSApp = [NSApplication alloc];

We need to see if allocating an NSApplication causes us to attach to stuff we don't want. So the accessibility code needs an NSApp instantiated to work? Or is this just something that they could potentially fix on their side?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #2)
> Comment on attachment 330558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330558&action=review
> 
> This looks good, but I'm sad we have to instantiate an NSApp instance. We
> should see if the accessibility team can live without that change (but
> that's not relevant to this patch).
> 
> > Source/WebKit/Configurations/WebContentService.xcconfig:56
> > +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain;
> 
> Oh, nice!
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:302
> > +    NSApp = [NSApplication alloc];
> 
> We need to see if allocating an NSApplication causes us to attach to stuff
> we don't want. So the accessibility code needs an NSApp instantiated to
> work? Or is this just something that they could potentially fix on their
> side?

Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side.

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 330558 [details]
Patch

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

>>> Source/WebKit/Configurations/WebContentService.xcconfig:56
>>> +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain;
>> 
>> Oh, nice!
> 
> Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side.
> 
> Thanks for reviewing!

Can you file a Radar about that and add it as a comment to the place where we make the NSApp allocation?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 330558 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330558&action=review
> 
> >>> Source/WebKit/Configurations/WebContentService.xcconfig:56
> >>> +RUNLOOP_TYPE[sdk=macosx10.11*] = _NSApplicationMain;
> >> 
> >> Oh, nice!
> > 
> > Currently, allocating an NSApplication object is needed for VoiceOver to work, but I believe this could be fixed on their side.
> > 
> > Thanks for reviewing!
> 
> Can you file a Radar about that and add it as a comment to the place where
> we make the NSApp allocation?

Yes, will do :)

[Per Arne Vollan]

Created attachment 330567 [details]
Patch

[Per Arne Vollan]

Created attachment 330717 [details]
Patch

[Anders Carlsson]

Comment on attachment 330717 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304
> +    NSApp = [NSApplication alloc];
> +    [NSApplication _accessibilityInitialize];

Calling +alloc without -init is bad.

[Per Arne Vollan]

(In reply to Anders Carlsson from comment #8)
> Comment on attachment 330717 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330717&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304
> > +    NSApp = [NSApplication alloc];
> > +    [NSApplication _accessibilityInitialize];
> 
> Calling +alloc without -init is bad.

That makes sense. However, we are trying to avoid all initialization of NSApplication, since it should not be needed in the WebContent process. This allocation will be unnecessary when NSApp is no longer needed for VoiceOver to work.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 330914 [details]
Patch

[Brent Fulgham]

Comment on attachment 330914 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=181331

Please put the <rdar> here.

> Source/WebCore/platform/mac/ThemeMac.mm:71
> +    // Creating a NSWindow will crash if the NSApplication event loop is not running.

Creating AN (not Creating A)

> Source/WebKit/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=181331

<radar>

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:92
> ++ (void)_accessibilityInitialize;

We should do this in a header file (Source/WebCore/pal/spi/cocoa/NSApplicationSPI.h), then include that header file here.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:304
> +    NSApp = [NSApplication alloc];

While Anders is correct, I prefer that we not initialize NSApp, since we are trying to get rid of it, and it does bad things (sandbox-wise) when it starts up.

[Brent Fulgham]

This patch looks good, but r- until the SPI is moved to our "spi" folder.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #12)
> This patch looks good, but r- until the SPI is moved to our "spi" folder.

Thanks for reviewing! I will update the patch.

[Radar WebKit Bug Importer]

<rdar://problem/36408004>

[Per Arne Vollan]

Created attachment 330926 [details]
Patch

[Brent Fulgham]

Comment on attachment 330926 [details]
Patch

Excellent! r=me.

[EWS Watchlist]

Comment on attachment 330926 [details]
Patch

Attachment 330926 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/6023079

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[EWS Watchlist]

Created attachment 330937 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 330926 [details]
Patch

Attachment 330926 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6024538

New failing tests:
http/tests/workers/service/postmessage-after-sw-process-crash.https.html

[EWS Watchlist]

Created attachment 330956 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Per Arne Vollan]

Created attachment 331100 [details]
Patch

[Brent Fulgham]

Comment on attachment 331100 [details]
Patch

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

Assuming the statement about [NSApplication sharedApplication] I made is true, r=me.

> Source/WebCore/PAL/ChangeLog:5
> +        rdar://problem/36408004

<rdar://problem/3640004>

(I.e., angle brackets around the radar URL please!)

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:177
> +    [NSApplication sharedApplication];

So I believe that this call causes an alloc and init of NSApplication to occur. Is that correct?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #22)
> Comment on attachment 331100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331100&action=review
> 
> Assuming the statement about [NSApplication sharedApplication] I made is
> true, r=me.
> 
> > Source/WebCore/PAL/ChangeLog:5
> > +        rdar://problem/36408004
> 
> <rdar://problem/3640004>
> 
> (I.e., angle brackets around the radar URL please!)
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:177
> > +    [NSApplication sharedApplication];
> 
> So I believe that this call causes an alloc and init of NSApplication to
> occur. Is that correct?

Yes, that is correct.

Thanks for reviewing!

[Alexey Proskuryakov]

rdar://problem/36408004

(because comment 22 had a typo, targeting the wrong radar number)

[Per Arne Vollan]

Committed r226807: <https://trac.webkit.org/changeset/226807/webkit>.