Changeset 259673 in webkit
- Timestamp:
- Apr 7, 2020 3:07:54 PM (4 years ago)
- Location:
- trunk/Source/WebKit
- Files:
-
- 10 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebKit/ChangeLog
r259672 r259673 1 2020-04-07 Alex Christensen <achristensen@webkit.org> 2 3 Simplify and fortify network getNetworkProcessConnection and getGPUProcessConnection 4 https://bugs.webkit.org/show_bug.cgi?id=210142 5 <rdar://problem/59488963> 6 7 Reviewed by Youenn Fablet. 8 9 We have reports of hangs inside WebKit::getNetworkProcessConnection that seem to last forever. 10 Some of the reports indicate the network process is being suspended while a connection is being established with it. 11 12 To fix this issue we do three things: 13 1. We take a foregroundActivity when sending an async message to establish a connection. 14 2. We use sendWithAsyncReply which already has logic to handle the case where we are currently launching the process. 15 Instead of the complicated retry logic, we add a retry attempt in WebProcessPool if the connection identifier is invalid. 16 3. Add some release logging so we can better diagnose problems with this flow in the future. 17 18 The functional change is adding the foreground activity, which should prevent some hangs. 19 The rest is just to make this code more sane to understand and debug. 20 I do the same changes to NetworkProcess and GPUProcess because they are intended to be the same. The latter is based on the former. 21 22 The API test WebKit.NetworkProcessCrashWithPendingConnection covers what happens when the network process crashes during connection establishment. 23 It fails if we don't retry somewhere, which I did in WebProcessPool. We also need to try again in getNetworkProcessConnection and getGPUProcessConnection. 24 If it fails twice, there's nothing we can do, and we crash the web process to avoid a crash loop. 25 26 * UIProcess/GPU/GPUProcessProxy.cpp: 27 (WebKit::GPUProcessProxy::getGPUProcessConnection): 28 (WebKit::GPUProcessProxy::didFinishLaunching): 29 (WebKit::GPUProcessProxy::~GPUProcessProxy): Deleted. 30 (WebKit::GPUProcessProxy::openGPUProcessConnection): Deleted. 31 * UIProcess/GPU/GPUProcessProxy.h: 32 * UIProcess/Network/NetworkProcessProxy.cpp: 33 (WebKit::NetworkProcessProxy::~NetworkProcessProxy): 34 (WebKit::NetworkProcessProxy::getNetworkProcessConnection): 35 (WebKit::NetworkProcessProxy::networkProcessCrashed): 36 (WebKit::NetworkProcessProxy::didFinishLaunching): 37 (WebKit::NetworkProcessProxy::openNetworkProcessConnection): Deleted. 38 * UIProcess/Network/NetworkProcessProxy.h: 39 * UIProcess/WebProcessPool.cpp: 40 (WebKit::WebProcessPool::networkProcessCrashed): 41 (WebKit::WebProcessPool::getNetworkProcessConnection): 42 (WebKit::WebProcessPool::getGPUProcessConnection): 43 * UIProcess/WebProcessPool.h: 44 * WebProcess/GPU/GPUProcessConnectionInfo.h: 45 (WebKit::GPUProcessConnectionInfo::identifier const): 46 (WebKit::GPUProcessConnectionInfo::identifier): Deleted. 47 * WebProcess/Network/NetworkProcessConnectionInfo.h: 48 (WebKit::NetworkProcessConnectionInfo::identifier const): 49 (WebKit::NetworkProcessConnectionInfo::identifier): Deleted. 50 1 51 2020-04-07 Simon Fraser <simon.fraser@apple.com> 2 52 -
trunk/Source/WebKit/UIProcess/GPU/GPUProcessProxy.cpp
r258322 r259673 133 133 } 134 134 135 GPUProcessProxy::~GPUProcessProxy() 136 { 137 for (auto& request : m_connectionRequests.values()) 138 request.reply({ }); 139 } 135 GPUProcessProxy::~GPUProcessProxy() = default; 140 136 141 137 #if ENABLE(MEDIA_STREAM) … … 179 175 void GPUProcessProxy::getGPUProcessConnection(WebProcessProxy& webProcessProxy, Messages::WebProcessProxy::GetGPUProcessConnection::DelayedReply&& reply) 180 176 { 181 m_connectionRequests.add(++m_connectionRequestIdentifier, ConnectionRequest { makeWeakPtr(webProcessProxy), WTFMove(reply) });182 if (state() == State::Launching)183 return;184 openGPUProcessConnection(m_connectionRequestIdentifier, webProcessProxy);185 }186 187 void GPUProcessProxy::openGPUProcessConnection(ConnectionRequestIdentifier connectionRequestIdentifier, WebProcessProxy& webProcessProxy)188 {189 177 addSession(webProcessProxy.websiteDataStore()); 190 191 178 #if HAVE(VISIBILITY_PROPAGATION_VIEW) 192 179 if (m_contextIDForVisibilityPropagation) … … 196 183 #endif 197 184 198 sendWithAsyncReply(Messages::GPUProcess::CreateGPUConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(this), webProcessProxy = makeWeakPtr(webProcessProxy), connectionRequestIdentifier](auto&& connectionIdentifier) mutable { 199 if (!weakThis) 200 return; 201 202 if (!connectionIdentifier) { 203 // GPU process probably crashed, the connection request should have been moved. 204 ASSERT(m_connectionRequests.isEmpty()); 205 return; 185 RELEASE_LOG(ProcessSuspension, "%p - GPUProcessProxy is taking a background assertion because a web process is requesting a connection", this); 186 sendWithAsyncReply(Messages::GPUProcess::CreateGPUConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(*this), reply = WTFMove(reply), activity = throttler().backgroundActivity("GPUProcessProxy::getGPUProcessConnection"_s)](auto&& connectionIdentifier) mutable { 187 if (!weakThis) { 188 RELEASE_LOG_ERROR(Process, "GPUProcessProxy::getGPUProcessConnection: GPUProcessProxy deallocated during connection establishment"); 189 return reply({ }); 206 190 } 207 208 ASSERT(m_connectionRequests.contains(connectionRequestIdentifier));209 auto request = m_connectionRequests.take(connectionRequestIdentifier);210 211 191 #if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS) 212 re quest.reply(GPUProcessConnectionInfo { WTFMove(*connectionIdentifier) });192 reply(GPUProcessConnectionInfo { WTFMove(*connectionIdentifier) }); 213 193 #elif OS(DARWIN) 214 194 MESSAGE_CHECK(MACH_PORT_VALID(connectionIdentifier->port())); 215 re quest.reply(GPUProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND }, this->connection()->getAuditToken() });195 reply(GPUProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND }, this->connection()->getAuditToken() }); 216 196 #else 217 197 notImplemented(); … … 253 233 gpuProcessCrashed(); 254 234 return; 255 }256 257 auto connectionRequests = WTFMove(m_connectionRequests);258 for (auto& connectionRequest : connectionRequests.values()) {259 if (connectionRequest.webProcess)260 getGPUProcessConnection(*connectionRequest.webProcess, WTFMove(connectionRequest.reply));261 else262 connectionRequest.reply({ });263 235 } 264 236 -
trunk/Source/WebKit/UIProcess/GPU/GPUProcessProxy.h
r259540 r259673 97 97 void didFinishLaunching(ProcessLauncher*, IPC::Connection::Identifier) override; 98 98 99 typedef uint64_t ConnectionRequestIdentifier;100 void openGPUProcessConnection(ConnectionRequestIdentifier, WebProcessProxy&);101 102 99 // IPC::Connection::Client 103 100 void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override; … … 110 107 111 108 static GPUProcessProxy* m_singleton; 112 113 struct ConnectionRequest {114 WeakPtr<WebProcessProxy> webProcess;115 Messages::WebProcessProxy::GetGPUProcessConnectionDelayedReply reply;116 };117 ConnectionRequestIdentifier m_connectionRequestIdentifier { 0 };118 HashMap<ConnectionRequestIdentifier, ConnectionRequest> m_connectionRequests;119 109 120 110 ProcessThrottler m_throttler; -
trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
r259540 r259673 109 109 if (m_downloadProxyMap) 110 110 m_downloadProxyMap->invalidate(); 111 112 for (auto& request : m_connectionRequests.values())113 request.reply({ });114 111 } 115 112 … … 141 138 void NetworkProcessProxy::getNetworkProcessConnection(WebProcessProxy& webProcessProxy, Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply&& reply) 142 139 { 143 m_connectionRequests.add(++m_connectionRequestIdentifier, ConnectionRequest { makeWeakPtr(webProcessProxy), WTFMove(reply) }); 144 if (state() == State::Launching) 145 return; 146 openNetworkProcessConnection(m_connectionRequestIdentifier, webProcessProxy); 147 } 148 149 void NetworkProcessProxy::openNetworkProcessConnection(uint64_t connectionRequestIdentifier, WebProcessProxy& webProcessProxy) 150 { 151 connection()->sendWithAsyncReply(Messages::NetworkProcess::CreateNetworkConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(this), webProcessProxy = makeWeakPtr(webProcessProxy), connectionRequestIdentifier](auto&& connectionIdentifier, HTTPCookieAcceptPolicy cookieAcceptPolicy) mutable { 152 if (!weakThis) 153 return; 154 155 if (!connectionIdentifier) { 156 // Network process probably crashed, the connection request should have been moved. 157 ASSERT(m_connectionRequests.isEmpty()); 158 return; 140 RELEASE_LOG(ProcessSuspension, "%p - NetworkProcessProxy is taking a background assertion because a web process is requesting a connection", this); 141 sendWithAsyncReply(Messages::NetworkProcess::CreateNetworkConnectionToWebProcess { webProcessProxy.coreProcessIdentifier(), webProcessProxy.sessionID() }, [this, weakThis = makeWeakPtr(*this), reply = WTFMove(reply), activity = throttler().backgroundActivity("NetworkProcessProxy::getNetworkProcessConnection"_s)](auto&& connectionIdentifier, auto cookieAcceptPolicy) mutable { 142 if (!weakThis) { 143 RELEASE_LOG_ERROR(Process, "NetworkProcessProxy::getNetworkProcessConnection: NetworkProcessProxy deallocated during connection establishment"); 144 return reply({ }); 159 145 } 160 146 161 ASSERT(m_connectionRequests.contains(connectionRequestIdentifier));162 auto request = m_connectionRequests.take(connectionRequestIdentifier);163 164 147 #if USE(UNIX_DOMAIN_SOCKETS) || OS(WINDOWS) 165 re quest.reply(NetworkProcessConnectionInfo { WTFMove(*connectionIdentifier), cookieAcceptPolicy });148 reply(NetworkProcessConnectionInfo { WTFMove(*connectionIdentifier), cookieAcceptPolicy }); 166 149 #elif OS(DARWIN) 167 150 MESSAGE_CHECK(MACH_PORT_VALID(connectionIdentifier->port())); 168 re quest.reply(NetworkProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND }, cookieAcceptPolicy, connection()->getAuditToken() });151 reply(NetworkProcessConnectionInfo { IPC::Attachment { connectionIdentifier->port(), MACH_MSG_TYPE_MOVE_SEND }, cookieAcceptPolicy, connection()->getAuditToken() }); 169 152 #else 170 153 notImplemented(); … … 249 232 clearCallbackStates(); 250 233 251 Vector<std::pair<RefPtr<WebProcessProxy>, Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply>> pendingRequests;252 pendingRequests.reserveInitialCapacity(m_connectionRequests.size());253 for (auto& request : m_connectionRequests.values()) {254 if (request.webProcess)255 pendingRequests.uncheckedAppend(std::make_pair(makeRefPtr(request.webProcess.get()), WTFMove(request.reply)));256 else257 request.reply({ });258 }259 m_connectionRequests.clear();260 261 234 // Tell the network process manager to forget about this network process proxy. This will cause us to be deleted. 262 m_processPool.networkProcessCrashed(*this , WTFMove(pendingRequests));235 m_processPool.networkProcessCrashed(*this); 263 236 } 264 237 … … 408 381 networkProcessCrashed(); 409 382 return; 410 }411 412 auto connectionRequests = WTFMove(m_connectionRequests);413 for (auto& connectionRequest : connectionRequests.values()) {414 if (connectionRequest.webProcess)415 getNetworkProcessConnection(*connectionRequest.webProcess, WTFMove(connectionRequest.reply));416 else417 connectionRequest.reply({ });418 383 } 419 384 -
trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
r259540 r259673 287 287 void didFinishLaunching(ProcessLauncher*, IPC::Connection::Identifier) override; 288 288 289 void openNetworkProcessConnection(uint64_t connectionRequestIdentifier, WebProcessProxy&);290 291 289 void processAuthenticationChallenge(PAL::SessionID, Ref<AuthenticationChallengeProxy>&&); 292 290 293 291 WebProcessPool& m_processPool; 294 295 struct ConnectionRequest {296 WeakPtr<WebProcessProxy> webProcess;297 Messages::WebProcessProxy::GetNetworkProcessConnectionDelayedReply reply;298 };299 uint64_t m_connectionRequestIdentifier { 0 };300 HashMap<uint64_t, ConnectionRequest> m_connectionRequests;301 292 302 293 HashMap<uint64_t, CompletionHandler<void(WebsiteData)>> m_pendingFetchWebsiteDataCallbacks; -
trunk/Source/WebKit/UIProcess/WebProcessPool.cpp
r259540 r259673 40 40 #include "DownloadProxy.h" 41 41 #include "DownloadProxyMessages.h" 42 #include "GPUProcessConnectionInfo.h" 42 43 #include "GamepadData.h" 43 44 #include "HighPerformanceGraphicsUsageSampler.h" … … 45 46 #include "LogInitialization.h" 46 47 #include "Logging.h" 48 #include "NetworkProcessConnectionInfo.h" 47 49 #include "NetworkProcessCreationParameters.h" 48 50 #include "NetworkProcessMessages.h" … … 712 714 } 713 715 714 void WebProcessPool::networkProcessCrashed(NetworkProcessProxy& networkProcessProxy , Vector<std::pair<RefPtr<WebProcessProxy>, Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply>>&& pendingReplies)716 void WebProcessPool::networkProcessCrashed(NetworkProcessProxy& networkProcessProxy) 715 717 { 716 718 ASSERT(m_networkProcess); … … 728 730 m_networkProcess = nullptr; 729 731 730 // Attempt to re-launch.731 if (pendingReplies.isEmpty())732 return;733 auto& newNetworkProcess = ensureNetworkProcess();734 for (auto& reply : pendingReplies)735 newNetworkProcess.getNetworkProcessConnection(*reply.first, WTFMove(reply.second));736 737 732 terminateServiceWorkers(); 738 733 } … … 748 743 { 749 744 ensureNetworkProcess(); 750 ASSERT(m_networkProcess); 751 752 m_networkProcess->getNetworkProcessConnection(webProcessProxy, WTFMove(reply)); 745 m_networkProcess->getNetworkProcessConnection(webProcessProxy, [this, weakThis = makeWeakPtr(*this), webProcessProxy = makeWeakPtr(webProcessProxy), reply = WTFMove(reply)] (auto& connectionInfo) mutable { 746 if (UNLIKELY(!IPC::Connection::identifierIsValid(connectionInfo.identifier()) && webProcessProxy && weakThis)) { 747 WEBPROCESSPOOL_RELEASE_LOG_ERROR(Process, "getNetworkProcessConnection: Failed first attempt, retrying"); 748 ensureNetworkProcess(); 749 m_networkProcess->getNetworkProcessConnection(*webProcessProxy, WTFMove(reply)); 750 return; 751 } 752 reply(connectionInfo); 753 }); 753 754 } 754 755 … … 765 766 void WebProcessPool::getGPUProcessConnection(WebProcessProxy& webProcessProxy, Messages::WebProcessProxy::GetGPUProcessConnection::DelayedReply&& reply) 766 767 { 767 GPUProcessProxy::singleton().getGPUProcessConnection(webProcessProxy, WTFMove(reply)); 768 GPUProcessProxy::singleton().getGPUProcessConnection(webProcessProxy, [this, weakThis = makeWeakPtr(*this), webProcessProxy = makeWeakPtr(webProcessProxy), reply = WTFMove(reply)] (auto& connectionInfo) mutable { 769 if (UNLIKELY(!IPC::Connection::identifierIsValid(connectionInfo.identifier()) && webProcessProxy && weakThis)) { 770 WEBPROCESSPOOL_RELEASE_LOG_ERROR(Process, "getGPUProcessConnection: Failed first attempt, retrying"); 771 GPUProcessProxy::singleton().getGPUProcessConnection(*webProcessProxy, WTFMove(reply)); 772 return; 773 } 774 reply(connectionInfo); 775 }); 768 776 } 769 777 #endif -
trunk/Source/WebKit/UIProcess/WebProcessPool.h
r258949 r259673 390 390 NetworkProcessProxy& ensureNetworkProcess(WebsiteDataStore* withWebsiteDataStore = nullptr); 391 391 NetworkProcessProxy* networkProcess() { return m_networkProcess.get(); } 392 void networkProcessCrashed(NetworkProcessProxy& , Vector<std::pair<RefPtr<WebProcessProxy>, Messages::WebProcessProxy::GetNetworkProcessConnectionDelayedReply>>&&);392 void networkProcessCrashed(NetworkProcessProxy&); 393 393 394 394 void getNetworkProcessConnection(WebProcessProxy&, Messages::WebProcessProxy::GetNetworkProcessConnectionDelayedReply&&); -
trunk/Source/WebKit/WebProcess/GPU/GPUProcessConnectionInfo.h
r254392 r259673 36 36 #endif 37 37 38 IPC::Connection::Identifier identifier() 38 IPC::Connection::Identifier identifier() const 39 39 { 40 40 #if USE(UNIX_DOMAIN_SOCKETS) -
trunk/Source/WebKit/WebProcess/Network/NetworkProcessConnectionInfo.h
r254931 r259673 37 37 #endif 38 38 39 IPC::Connection::Identifier identifier() 39 IPC::Connection::Identifier identifier() const 40 40 { 41 41 #if USE(UNIX_DOMAIN_SOCKETS) -
trunk/Source/WebKit/WebProcess/WebProcess.cpp
r259307 r259673 1095 1095 NetworkProcessConnectionInfo connectionInfo; 1096 1096 if (!connection.sendSync(Messages::WebProcessProxy::GetNetworkProcessConnection(), Messages::WebProcessProxy::GetNetworkProcessConnection::Reply(connectionInfo), 0)) { 1097 // If we failed the first time, retry once. The attachment may have become invalid 1098 // before it was received by the web process if the network process crashed. 1099 if (!connection.sendSync(Messages::WebProcessProxy::GetNetworkProcessConnection(), Messages::WebProcessProxy::GetNetworkProcessConnection::Reply(connectionInfo), 0)) { 1097 1100 #if PLATFORM(GTK) || PLATFORM(WPE) 1098 // GTK+ and WPE ports don't exit on send sync message failure.1099 // In this particular case, the network process can be terminated by the UI process while the1100 // Web process is still initializing, so we always want to exit instead of crashing. This can1101 // happen when the WebView is created and then destroyed quickly.1102 // See https://bugs.webkit.org/show_bug.cgi?id=183348.1103 exit(0);1101 // GTK+ and WPE ports don't exit on send sync message failure. 1102 // In this particular case, the network process can be terminated by the UI process while the 1103 // Web process is still initializing, so we always want to exit instead of crashing. This can 1104 // happen when the WebView is created and then destroyed quickly. 1105 // See https://bugs.webkit.org/show_bug.cgi?id=183348. 1106 exit(0); 1104 1107 #else 1105 CRASH(); 1106 #endif 1108 CRASH(); 1109 #endif 1110 } 1107 1111 } 1108 1112 … … 1218 1222 { 1219 1223 GPUProcessConnectionInfo connectionInfo; 1220 if (!connection.sendSync(Messages::WebProcessProxy::GetGPUProcessConnection(), Messages::WebProcessProxy::GetGPUProcessConnection::Reply(connectionInfo), 0)) 1221 CRASH(); 1224 if (!connection.sendSync(Messages::WebProcessProxy::GetGPUProcessConnection(), Messages::WebProcessProxy::GetGPUProcessConnection::Reply(connectionInfo), 0)) { 1225 // If we failed the first time, retry once. The attachment may have become invalid 1226 // before it was received by the web process if the network process crashed. 1227 if (!connection.sendSync(Messages::WebProcessProxy::GetGPUProcessConnection(), Messages::WebProcessProxy::GetGPUProcessConnection::Reply(connectionInfo), 0)) 1228 CRASH(); 1229 } 1222 1230 1223 1231 return connectionInfo;
Note: See TracChangeset
for help on using the changeset viewer.