Summary: | Add SPI to use networking daemon instead of XPC service | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, dbates, ews-watchlist, ggaren, joepeck, mitz, rniwa, sam, tsavell, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Alex Christensen
2019-02-07 20:51:48 PST
Created attachment 361491 [details]
Patch
Comment on attachment 361491 [details]
Patch
r=me
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 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
Created attachment 361507 [details]
Patch
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. 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 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. 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> Created attachment 361554 [details]
Patch
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.
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.
I also gave MiniBrowser's sandbox permission to communicate with this new mach service. http://trac.webkit.org/r241223 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. (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? (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. (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? > > 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.
(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. (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. Reopening to attach new patch. Created attachment 362771 [details]
Patch
Created attachment 362783 [details]
Patch
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.
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 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
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. 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
Created attachment 363381 [details]
Patch
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.
|