Bug 189663 – REGRESSION (PSON): White or Black flash occurs when process swapping on navigation on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

Fix it.

[Antti Koivisto]

<rdar://problem/44184955>

[Antti Koivisto]

Created attachment 349886 [details]
patch

[Geoffrey Garen]

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.

[Antti Koivisto]

> 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'.

[Antti Koivisto]

/processes/pages/

[Geoffrey Garen]

Do we have a separate bug for the iOS flavor of this flashing problem?

[Antti Koivisto]

Created attachment 350013 [details]
patch

[Antti Koivisto]

(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

[WebKit Commit Bot]

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

[Antti Koivisto]

Created attachment 350021 [details]
patch

[WebKit Commit Bot]

Comment on attachment 350021 [details]
patch

Clearing flags on attachment: 350021

Committed r236138: <https://trac.webkit.org/changeset/236138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Chris Dumez]

(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?

[Ryan Haddad]

(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

[Chris Dumez]

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.

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r236138 for reason:

Caused API test and layout test failures on iOS.

Committed r236146: <https://trac.webkit.org/changeset/236146>

[Antti Koivisto]

> 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.

[Antti Koivisto]

Created attachment 350112 [details]
patch

[Antti Koivisto]

Created attachment 350116 [details]
patch

[Chris Dumez]

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?

[Antti Koivisto]

> > 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.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[Antti Koivisto]

Ah ok, I'll set the bit WebPage::didReceivePolicyDecision then.

[Antti Koivisto]

Created attachment 350186 [details]
patch

[Antti Koivisto]

Maybe the whole SetIsSuspended message is unnecessary and we can manage the state via PolicyActions.

[WebKit Commit Bot]

Comment on attachment 350186 [details]
patch

Clearing flags on attachment: 350186

Committed r236257: <https://trac.webkit.org/changeset/236257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.