Bug 196168 – Web Automation: clean up some WebAutomationSession methods to use modern async IPC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Blaze Burg]

.

[Blaze Burg]

Created attachment 365780 [details]
WIP

[Blaze Burg]

Comment on attachment 365780 [details]
WIP

This is as far as I got today. I was unable to convert the screenshot method because it transfers a SharedBitmap::Handle / SharedMemory::Handle, which does not have a modern decoder.

It seems problematic to have a signature of Optional<SharedMemory::Handle> decode(IPC::Decoder&) because SharedMemory::Handle is noncopyable. I haven't found anyway to accomplish that, and since Optional is typically copyable, I'm not sure that it makes sense to put a noncopyable thing into a copyable Optional.

[Blaze Burg]

Created attachment 365891 [details]
Patch

[Blaze Burg]

<rdar://problem/48752230>

[Devin Rousso]

Comment on attachment 365891 [details]
Patch

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

r=me, most of my comments are applicable in more than just that spot, so if they are addressed please try to address them everywhere :)

> Source/WebKit/ChangeLog:9
> +        So, most messages between WebAutomationSession and its proxy can use this facility, and stop

NIT: unnecessary comma => "facility and stop".

> Source/WebKit/ChangeLog:16
> +        be able to cancel all pending replies when a page navigates away, the web process crashes,
> +        or when handling an alert.

NIT: I normally indent subsequent lines to make it clear where each bullet point begins/ends.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:973
> +    WTF::CompletionHandler<void(Optional<String>, uint64_t)> completionHandler = [this, protectedThis = makeRef(*this), callback = callback.copyRef()](Optional<String> errorType, uint64_t frameID) mutable {

Given that the `callback` is a `&&`, should we move it into the lambda (e.g. `callback = WTFMove(callback)`)?

Also, can you use `auto completionHandler` or is it necessary that you specify the type?

Style: there should be a space between the "](".

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:975
> +            callback->sendFailure(STRING_FOR_PREDEFINED_ERROR_MESSAGE(*errorType));

Style: I prefer using `errorType.value()` for `Optional`s.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1010
> +    WTF::CompletionHandler<void(Optional<String>, uint64_t)> completionHandler = [this, protectedThis = makeRef(*this), callback = callback.copyRef()](Optional<String> errorType, uint64_t frameID) mutable {

Could you inline this, since it's only used once (and that usage is a `WTFMove`)?

[Blaze Burg]

I'm investigating a few crashes that this change has caused, along the lines of:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BREAKPOINT (SIGTRAP)
Exception Codes:       0x0000000000000002, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Trace/BPT trap: 5
Termination Reason:    Namespace SIGNAL, Code 0x5
Terminating Process:   exc handler [9528]

Application Specific Information:
Received invalid message: 'WebAutomationSessionProxy::ComputeElementLayout'
Bundle controller class:
BrowserBundleController

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x00000006c014041b WebKit::AuxiliaryProcess::didReceiveInvalidMessage(IPC::Connection&, IPC::StringReference, IPC::StringReference) + 151 (AuxiliaryProcessCocoa.mm:38)
1   com.apple.WebKit              	0x00000006c0014b14 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 200 (Connection.cpp:1026)
2   com.apple.WebKit              	0x00000006c00182ab IPC::Connection::dispatchOneIncomingMessage() + 181
3   com.apple.JavaScriptCore      	0x00000006c6115aa4 WTF::RunLoop::performWork() + 228
4   com.apple.JavaScriptCore      	0x00000006c6115d32 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
5   com.apple.CoreFoundation      	0x00007fff42c2e0c3 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17

[Blaze Burg]

I can't reproduce this on a clean build, so I think something was out of sync.

[Blaze Burg]

Committed r244033: <https://trac.webkit.org/changeset/244033>