Bug 194427 – Add SPI to use networking daemon instead of XPC service

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Add SPI to use networking daemon instead of XPC service

[Alex Christensen]

Created attachment 361491 [details]
Patch

[Geoffrey Garen]

Comment on attachment 361491 [details]
Patch

r=me

[EWS Watchlist]

Comment on attachment 361491 [details]
Patch

Attachment 361491 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11076179

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html

[EWS Watchlist]

Created attachment 361494 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Alex Christensen]

Created attachment 361507 [details]
Patch

[Alex Christensen]

Comment on attachment 361507 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:177
> +    xpc_dictionary_set_value(bootstrapMessage.get(), "initialization-message", initializationMessage.get());

I made the initialization message a member of the bootstrap message instead of sending a separate message to reduce the number of messages sent during process launching.

[Alex Christensen]

http://trac.webkit.org/r241197

[Radar WebKit Bug Importer]

<rdar://problem/47918588>

[Truitt Savell]

It looks like https://trac.webkit.org/changeset/241197/webkit 

Has broken iOS Debug https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Debug%20%28Build%29/builds/3331

as well as caused 1 API failure on High Sierra:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/9384

Failed

    TestWebKitAPI.WebKit.InjectedBundleAppleEvent
        
        /Volumes/Data/slave/highsierra-release/build/Tools/TestWebKitAPI/Tests/WebKit/mac/InjectedBundleAppleEvent.cpp:55
        Expected: (0) != (returnCode), actual: 0 vs 0

[Joseph Pecoraro]

Comment on attachment 361507 [details]
Patch

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:76
> +    case ProcessLauncher::ProcessType::NetworkDaemon:
> +        ASSERT_NOT_REACHED();

I'd expect a `break` here to avoid potential -Wswitch/-Wfallthrough warnings.

[Truitt Savell]

Reverted r241197 for reason:

Broke iOS Simulator Debug build and casued 1 API failure on High Sierra

Committed r241205: <https://trac.webkit.org/changeset/241205>

[Alex Christensen]

Created attachment 361554 [details]
Patch

[Alex Christensen]

Comment on attachment 361554 [details]
Patch

I put the SPI header definitions in the correct sections, moved the __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling, and put the whole "bootstrap" event handler in a std::call_once because it sets the environment that we won't be able to set with the second xpc connection to a daemon.

[EWS Watchlist]

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


ERROR: Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm:115:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WTF/wtf/spi/darwin/XPCSPI.h:84:  _xpc_error_key_description is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 15 files


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

[Alex Christensen]

I also gave MiniBrowser's sandbox permission to communicate with this new mach service.

http://trac.webkit.org/r241223

[Alex Christensen]

http://trac.webkit.org/r241235

[Alex Christensen]

My changelog entry for r241235 indicated an API test on Mojave.  It was actually a fix fo the WebKit.InjectedBundleAppleEvent API test on High Sierra, and it seems to have worked.

[Daniel Bates]

(In reply to Alex Christensen from comment #17)
> My changelog entry for r241235 indicated an API test on Mojave.  It was
> actually a fix fo the WebKit.InjectedBundleAppleEvent API test on High
> Sierra, and it seems to have worked.

You can fix the ChangeLog. Fix it?

[Sam Weinig]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 361554 [details]
> Patch
> 
> I put the SPI header definitions in the correct sections, moved the
> __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> and put the whole "bootstrap" event handler in a std::call_once because it
> sets the environment that we won't be able to set with the second xpc
> connection to a daemon.

I don't think this is correct for the daemon case. If two UIProcesses with different expectations of the networking daemon connect to it, each with different  initialization criteria, this will not yield the right result. For instance, any OverrideLanguages will not be set. I would think you would either need to remove those UIProcess specific settings completely, or make it so multiple daemons could be spawned, one for each initialization criteria set.

[Sam Weinig]

(In reply to Sam Weinig from comment #19)
> (In reply to Alex Christensen from comment #13)
> > Comment on attachment 361554 [details]
> > Patch
> > 
> > I put the SPI header definitions in the correct sections, moved the
> > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > and put the whole "bootstrap" event handler in a std::call_once because it
> > sets the environment that we won't be able to set with the second xpc
> > connection to a daemon.
> 
> I don't think this is correct for the daemon case. If two UIProcesses with
> different expectations of the networking daemon connect to it, each with
> different  initialization criteria, this will not yield the right result.
> For instance, any OverrideLanguages will not be set. I would think you would
> either need to remove those UIProcess specific settings completely, or make
> it so multiple daemons could be spawned, one for each initialization
> criteria set.

When we initially added the bootstrap message using xpc_connection_set_bootstrap/xpc_copy_bootstrap, it was needed because the actions taken needed to happen before xpc_main() was called. What has changed to make that no longer necessary?

[Geoffrey Garen]

> > I put the SPI header definitions in the correct sections, moved the
> > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > and put the whole "bootstrap" event handler in a std::call_once because it
> > sets the environment that we won't be able to set with the second xpc
> > connection to a daemon.
> 
> I don't think this is correct for the daemon case. If two UIProcesses with
> different expectations of the networking daemon connect to it, each with
> different  initialization criteria, this will not yield the right result.
> For instance, any OverrideLanguages will not be set. I would think you would
> either need to remove those UIProcess specific settings completely, or make
> it so multiple daemons could be spawned, one for each initialization
> criteria set.

I don't think the OS supports multi-daemon, so we'll need to resolve the UIProcess specific settings.

[Alex Christensen]

(In reply to Sam Weinig from comment #19)
> (In reply to Alex Christensen from comment #13)
> > Comment on attachment 361554 [details]
> > Patch
> > 
> > I put the SPI header definitions in the correct sections, moved the
> > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > and put the whole "bootstrap" event handler in a std::call_once because it
> > sets the environment that we won't be able to set with the second xpc
> > connection to a daemon.
> 
> I don't think this is correct for the daemon case. If two UIProcesses with
> different expectations of the networking daemon connect to it, each with
> different  initialization criteria, this will not yield the right result.
> For instance, any OverrideLanguages will not be set. I would think you would
> either need to remove those UIProcess specific settings completely, or make
> it so multiple daemons could be spawned, one for each initialization
> criteria set.

I don't think this will be a problem right away because there will only be one client that uses the SPI to use the daemon, and I think its initialization criteria will be the same for each instance.

(In reply to Sam Weinig from comment #20)
> (In reply to Sam Weinig from comment #19)
> > (In reply to Alex Christensen from comment #13)
> > > Comment on attachment 361554 [details]
> > > Patch
> > > 
> > > I put the SPI header definitions in the correct sections, moved the
> > > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > > and put the whole "bootstrap" event handler in a std::call_once because it
> > > sets the environment that we won't be able to set with the second xpc
> > > connection to a daemon.
> > 
> > I don't think this is correct for the daemon case. If two UIProcesses with
> > different expectations of the networking daemon connect to it, each with
> > different  initialization criteria, this will not yield the right result.
> > For instance, any OverrideLanguages will not be set. I would think you would
> > either need to remove those UIProcess specific settings completely, or make
> > it so multiple daemons could be spawned, one for each initialization
> > criteria set.
> 
> When we initially added the bootstrap message using
> xpc_connection_set_bootstrap/xpc_copy_bootstrap, it was needed because the
> actions taken needed to happen before xpc_main() was called. What has
> changed to make that no longer necessary?

It seems that the setting of the __APPLEEVENTSSERVICENAME environment variable is only effective if done before calling xpc_main, but the rest seems to work unless I'm missing something.  The part that must be done before xpc_main does not depend on any information from the UIProcess.  It's definitely possible I overlooked something, but everything seems to work.

[Sam Weinig]

(In reply to Alex Christensen from comment #22)
> (In reply to Sam Weinig from comment #19)
> > (In reply to Alex Christensen from comment #13)
> > > Comment on attachment 361554 [details]
> > > Patch
> > > 
> > > I put the SPI header definitions in the correct sections, moved the
> > > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > > and put the whole "bootstrap" event handler in a std::call_once because it
> > > sets the environment that we won't be able to set with the second xpc
> > > connection to a daemon.
> > 
> > I don't think this is correct for the daemon case. If two UIProcesses with
> > different expectations of the networking daemon connect to it, each with
> > different  initialization criteria, this will not yield the right result.
> > For instance, any OverrideLanguages will not be set. I would think you would
> > either need to remove those UIProcess specific settings completely, or make
> > it so multiple daemons could be spawned, one for each initialization
> > criteria set.
> 
> I don't think this will be a problem right away because there will only be
> one client that uses the SPI to use the daemon, and I think its
> initialization criteria will be the same for each instance.

How do you expect to test this? How will developer builds work? How will Safari Technology Preview work? 

> 
> (In reply to Sam Weinig from comment #20)
> > (In reply to Sam Weinig from comment #19)
> > > (In reply to Alex Christensen from comment #13)
> > > > Comment on attachment 361554 [details]
> > > > Patch
> > > > 
> > > > I put the SPI header definitions in the correct sections, moved the
> > > > __APPLEEVENTSSERVICENAME setting to before the "bootstrap" event handling,
> > > > and put the whole "bootstrap" event handler in a std::call_once because it
> > > > sets the environment that we won't be able to set with the second xpc
> > > > connection to a daemon.
> > > 
> > > I don't think this is correct for the daemon case. If two UIProcesses with
> > > different expectations of the networking daemon connect to it, each with
> > > different  initialization criteria, this will not yield the right result.
> > > For instance, any OverrideLanguages will not be set. I would think you would
> > > either need to remove those UIProcess specific settings completely, or make
> > > it so multiple daemons could be spawned, one for each initialization
> > > criteria set.
> > 
> > When we initially added the bootstrap message using
> > xpc_connection_set_bootstrap/xpc_copy_bootstrap, it was needed because the
> > actions taken needed to happen before xpc_main() was called. What has
> > changed to make that no longer necessary?
> 
> It seems that the setting of the __APPLEEVENTSSERVICENAME environment
> variable is only effective if done before calling xpc_main, but the rest
> seems to work unless I'm missing something.  The part that must be done
> before xpc_main does not depend on any information from the UIProcess.  It's
> definitely possible I overlooked something, but everything seems to work.

One thing I would expect to not be working as expected is localization. From memory, trying to set languages after xpc_main() did not work in the past.

[Alex Christensen]

http://trac.webkit.org/r241287

[Alex Christensen]

Reopening to attach new patch.

[Alex Christensen]

Created attachment 362771 [details]
Patch

[Alex Christensen]

Created attachment 362783 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/spi/darwin/XPCSPI.h:159:  xpc_connection_set_bootstrap is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/darwin/XPCSPI.h:160:  xpc_copy_bootstrap is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 8 files


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

[EWS Watchlist]

Comment on attachment 362783 [details]
Patch

Attachment 362783 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11254020

New failing tests:
fast/text/international/system-language/han-quotes.html
fast/text/international/system-language/declarative-language.html

[EWS Watchlist]

Created attachment 362803 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 362783 [details]
Patch

Attachment 362783 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11256733

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 362815 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Alex Christensen]

Created attachment 363381 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WTF/wtf/spi/darwin/XPCSPI.h:159:  xpc_connection_set_bootstrap is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/spi/darwin/XPCSPI.h:160:  xpc_copy_bootstrap is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 8 files


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

[Alex Christensen]

http://trac.webkit.org/r242303