Fix it.
<rdar://problem/44184955>
Created attachment 349886 [details] patch
Comment on attachment 349886 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=349886&action=review r=me > Source/WebKit/ChangeLog:24 > + We no longer tear down drawing area (layer tree) for suspended pages automatically. Send an explicit > + message for it. How do we ensure that suspended but not navigated pages tear down their layer trees? I don't see a call to tearDownDrawingAreaInWebProcess in that case. Is it already covered some other way? > Source/WebKit/WebProcess/WebPage/WebPage.messages.in:509 > + TearDownDrawingAreaForSuspended(); Let's call this TearDownDrawingAreaForSuspend.
> How do we ensure that suspended but not navigated pages tear down their > layer trees? I don't see a call to tearDownDrawingAreaInWebProcess in that > case. Is it already covered some other way? Currently we always navigate suspended processes to 'about:blank'.
/processes/pages/
Do we have a separate bug for the iOS flavor of this flashing problem?
Created attachment 350013 [details] patch
(In reply to Geoffrey Garen from comment #6) > Do we have a separate bug for the iOS flavor of this flashing problem? https://bugs.webkit.org/show_bug.cgi?id=189695
Comment on attachment 350013 [details] patch Rejecting attachment 350013 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 5000 characters of output: iagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKSyntheticClickTapGestureRecognizer.dia -c /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/ios/WKSyntheticClickTapGestureRecognizer.m -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKSyntheticClickTapGestureRecognizer.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WebPage.o WebProcess/WebPage/WebPage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65 BGPU -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DENABLE_MANUAL_SANDBOXING -DHAVE_CORE_PREDICTION -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -DFRAMEWORK_NAME=WebKit -DOBJC_OLD_DISPATCH_PROTOTYPES=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk -fasm-blocks -fstrict-aliasing -Wprotocol -Wdeprecated-declarations -mmacosx-version-min=10.12 -g -fvisibility=hidden -Wno-sign-conversion -Winfinite-recursion -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/WebKit-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebCore.framework/PrivateHeaders/ForwardingHeaders -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebKit2 -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/WebKitAdditions -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/WebKitAdditions -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include/webrtc -I/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/usr/local/include/webrtc -I/Volumes/Data/EWS/WebKit/Source/WebKit -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wextra-tokens -Wformat-security -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wno-unused-parameter -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Carbon.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/Quartz.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/CoreServices.framework/Frameworks -iframework /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/PrivateFrameworks -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/WebKit2Prefix-gxxdgeyqarhvegasvqrycxzmlohu/WebKit2Prefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKSyntheticClickTapGestureRecognizer.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKSyntheticClickTapGestureRecognizer.dia -c /Volumes/Data/EWS/WebKit/Source/WebKit/UIProcess/ios/WKSyntheticClickTapGestureRecognizer.m -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WKSyntheticClickTapGestureRecognizer.o ** BUILD FAILED ** The following build commands failed: CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release/WebKit.build/Objects-normal/x86_64/WebPage.o WebProcess/WebPage/WebPage.cpp normal x86_64 c++ com.apple.compilers.llvm.clang.1_0.compiler (1 failure) Full output: https://webkit-queues.webkit.org/results/9255304
Created attachment 350021 [details] patch
Comment on attachment 350021 [details] patch Clearing flags on attachment: 350021 Committed r236138: <https://trac.webkit.org/changeset/236138>
All reviewed patches have been landed. Closing bug.
This change caused 6 TestWebKitAPI.ProcessSwap failures and 1 timeout on iOS Simulator: https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/7469
(In reply to Ryan Haddad from comment #13) > This change caused 6 TestWebKitAPI.ProcessSwap failures and 1 timeout on iOS > Simulator: > > https://build.webkit.org/builders/ > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/7469 Chances are it is causing some WebContent crashes?
(In reply to Chris Dumez from comment #14) > (In reply to Ryan Haddad from comment #13) > > This change caused 6 TestWebKitAPI.ProcessSwap failures and 1 timeout on iOS > > Simulator: > > > > https://build.webkit.org/builders/ > > Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/builds/7469 > > Chances are it is causing some WebContent crashes? Now we have results from a debug bot: https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Debug%20WK2%20%28Tests%29/builds/6484/steps/run-api-tests/logs/stdio ASSERTION FAILED: m_messageReceivers.contains(std::make_pair(messageReceiverName, destinationID)) /Volumes/Data/slave/ios-simulator-11-debug/build/Source/WebKit/Platform/IPC/MessageReceiverMap.cpp(72) : void IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference, uint64_t) 1 0x10e50b9d9 WTFCrash 2 0x1148277cb WTFCrashWithInfo(int, char const*, char const*, int) 3 0x1148b0c50 IPC::MessageReceiverMap::removeMessageReceiver(IPC::StringReference, unsigned long long) 4 0x114dc6307 WebKit::ChildProcessProxy::removeMessageReceiver(IPC::StringReference, unsigned long long) 5 0x115085a58 WebKit::SmartMagnificationController::~SmartMagnificationController() 6 0x115085aa5 WebKit::SmartMagnificationController::~SmartMagnificationController() 7 0x115085ac9 WebKit::SmartMagnificationController::~SmartMagnificationController() 8 0x1158ab985 -[WKContentView(WKInteraction) cleanupInteraction] 9 0x11589e50b -[WKContentView dealloc] 10 0x113e631b2 (anonymous namespace)::AutoreleasePoolPage::pop(void*) 11 0x12a019136 _CFAutoreleasePoolPop 12 0x11383e8bc -[NSAutoreleasePool drain] 13 0x10db89888 main 14 0x12a6b1955 start
Comment on attachment 350021 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=350021&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:802 > + pageClient().didRelaunchProcess(); The crashes are likely caused by not calling pageClient().didRelaunchProcess() anymore in case of process swap. The PageClient needs to be notified that the WebPageProxy's process has changed. Otherwise, it cannot do the necessary updates and things like SmartMagnificationController keep trying to receive IPC from the old process instead of the new one.
Layout test http/tests/navigation/process-swap-window-open.html is also crashing on iOS after this change. https://build.webkit.org/results/Apple%20iOS%2011%20Simulator%20Release%20WK2%20(Tests)/r236142%20(7470)/http/tests/navigation/process-swap-window-open-crash-log.txt
Reverted r236138 for reason: Caused API test and layout test failures on iOS. Committed r236146: <https://trac.webkit.org/changeset/236146>
> The crashes are likely caused by not calling > pageClient().didRelaunchProcess() anymore in case of process swap. The > PageClient needs to be notified that the WebPageProxy's process has changed. > Otherwise, it cannot do the necessary updates and things like > SmartMagnificationController keep trying to receive IPC from the old process > instead of the new one. The reason turned out to be that earlier suspend message to web process caused problems on iOS when it tore down the drawing area. Nevertheless I put processDidExit/didRelaunchProcess calls back using a separate PageClient::processWillSwap path that avoids clearing root layer on but is otherwise the same as processDidExit.
Created attachment 350112 [details] patch
Created attachment 350116 [details] patch
Comment on attachment 350116 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=350116&action=review r=me with comments. > Source/WebKit/UIProcess/WebPageProxy.cpp:2454 > + process().send(Messages::WebPage::SetIsSuspended(true), pageID()); I am wondering if we still need this IPC given that the WebProcess already gets a Policy decision IPC with PolicyAction::Suspend. Also, with your change, SetIsSuspended(true) IPC is sent twice to the process I believe. > Source/WebKit/mac/postprocess-framework-headers.sh:56 > + IOS_VERSION="12.0" Why this change?
> > Source/WebKit/UIProcess/WebPageProxy.cpp:2454 > > + process().send(Messages::WebPage::SetIsSuspended(true), pageID()); > > I am wondering if we still need this IPC given that the WebProcess already > gets a Policy decision IPC with PolicyAction::Suspend. Also, with your > change, SetIsSuspended(true) IPC is sent twice to the process I believe. The message from SuspendedPageProxy constructor comes too late (trip to runloop puts it after the policy delegate) as the page will have already transitioned to about:blank at that point. We need to tell the process immediately that it suspended so it knows to keep the layer tree frozen. I didn't remove the second message as it was harmless but maybe I should. > > Source/WebKit/mac/postprocess-framework-headers.sh:56 > > + IOS_VERSION="12.0" > > Why this change? Build hack I forgot to remove.
(In reply to Antti Koivisto from comment #23) > > > Source/WebKit/UIProcess/WebPageProxy.cpp:2454 > > > + process().send(Messages::WebPage::SetIsSuspended(true), pageID()); > > > > I am wondering if we still need this IPC given that the WebProcess already > > gets a Policy decision IPC with PolicyAction::Suspend. Also, with your > > change, SetIsSuspended(true) IPC is sent twice to the process I believe. > > The message from SuspendedPageProxy constructor comes too late (trip to > runloop puts it after the policy delegate) as the page will have already > transitioned to about:blank at that point. We need to tell the process > immediately that it suspended so it knows to keep the layer tree frozen. > > I didn't remove the second message as it was harmless but maybe I should. You misunderstanding my comment. We already send a policy decision IPC to the WebProcesss with PolicyAction::Suspend, in the same method where you now send the WebPage::SetIsSuspended(true) IPC. Therefore, the WebProcess, could use the existing IPC as signal that it is suspended. > > > > Source/WebKit/mac/postprocess-framework-headers.sh:56 > > > + IOS_VERSION="12.0" > > > > Why this change? > > Build hack I forgot to remove.
(In reply to Chris Dumez from comment #24) > (In reply to Antti Koivisto from comment #23) > > > > Source/WebKit/UIProcess/WebPageProxy.cpp:2454 > > > > + process().send(Messages::WebPage::SetIsSuspended(true), pageID()); > > > > > > I am wondering if we still need this IPC given that the WebProcess already > > > gets a Policy decision IPC with PolicyAction::Suspend. Also, with your > > > change, SetIsSuspended(true) IPC is sent twice to the process I believe. > > > > The message from SuspendedPageProxy constructor comes too late (trip to > > runloop puts it after the policy delegate) as the page will have already > > transitioned to about:blank at that point. We need to tell the process > > immediately that it suspended so it knows to keep the layer tree frozen. > > > > I didn't remove the second message as it was harmless but maybe I should. > > You misunderstanding my comment. We already send a policy decision IPC to > the WebProcesss with PolicyAction::Suspend, in the same method where you now > send the > WebPage::SetIsSuspended(true) IPC. Therefore, the WebProcess, could use the > existing IPC as signal that it is suspended. > I am referring to WebPage::didReceivePolicyDecision() which gets called with PolicyAction::Suspend in case of process swap, which we already use as signal to navigate to about:blank. > > > > > > Source/WebKit/mac/postprocess-framework-headers.sh:56 > > > > + IOS_VERSION="12.0" > > > > > > Why this change? > > > > Build hack I forgot to remove.
Ah ok, I'll set the bit WebPage::didReceivePolicyDecision then.
Created attachment 350186 [details] patch
Maybe the whole SetIsSuspended message is unnecessary and we can manage the state via PolicyActions.
Comment on attachment 350186 [details] patch Clearing flags on attachment: 350186 Committed r236257: <https://trac.webkit.org/changeset/236257>