Bug 196168

Summary: Web Automation: clean up some WebAutomationSession methods to use modern async IPC
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebDriverAssignee: BJ Burg <bburg>
Severity: Normal CC: achristensen, bburg, cdumez, cgarcia, drousso, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch drousso: review+

Description BJ Burg 2019-03-22 16:56:18 PDT
Comment 1 BJ Burg 2019-03-22 17:01:17 PDT
Created attachment 365780 [details]
Comment 2 BJ Burg 2019-03-22 17:03:35 PDT
Comment on attachment 365780 [details]

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.
Comment 3 BJ Burg 2019-03-25 13:28:38 PDT
Created attachment 365891 [details]
Comment 4 BJ Burg 2019-04-04 09:07:49 PDT
Comment 5 Devin Rousso 2019-04-04 13:00:01 PDT
Comment on attachment 365891 [details]

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`)?
Comment 6 BJ Burg 2019-04-08 11:15:15 PDT
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:

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
Comment 7 BJ Burg 2019-04-08 12:11:00 PDT
I can't reproduce this on a clean build, so I think something was out of sync.
Comment 8 BJ Burg 2019-04-08 12:31:52 PDT
Committed r244033: <https://trac.webkit.org/changeset/244033>