Bug 217870 – IPC testing API should have the capability to observe messages being sent and received

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

IPC testing JS API should have the ability to intercept IPC messaging being sent and received in Web process.\

<rdar://problem/70350874>

[Ryosuke Niwa]

Created attachment 411963 [details]
Patch

[Darin Adler]

Comment on attachment 411963 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:1044
> +        if (hasDeadObservers)
> +            m_messageObservers.removeAllMatching([](auto& observer) { return !observer; });

Seems like we could use removeAllMatching to remove while iterating, and call didReceiveMessage on the ones we don’t remove instead of having two loops and a boolean.

> Source/WebKit/Platform/IPC/JSIPCBinding.h:55
> +    object->putDirect(vm, JSC::Identifier::fromString(vm, "type"_s), JSC::jsNontrivialString(vm, type));

What guarantees non-trivial for this string? Maybe these are always string constants. If so, consider making the argument ASCIILiteral instead of const String&.

Even type could be ASCIILiteral instead of const String&.

> Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909
> +    JSObjectCallAsFunction(m_context, m_callback, m_callback, WTF_ARRAY_LENGTH(arguments), arguments, nullptr);

Should be able to use std::size instead of WTF_ARRAY_LENGTH in new code. We can probably get rid of WTF_ARRAY_LENGTH.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:257
> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:302
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 300, 300) configuration:configuration.get()]);

auto?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> Comment on attachment 411963 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=411963&action=review
> 
> > Source/WebKit/Platform/IPC/Connection.cpp:1044
> > +        if (hasDeadObservers)
> > +            m_messageObservers.removeAllMatching([](auto& observer) { return !observer; });
> 
> Seems like we could use removeAllMatching to remove while iterating, and
> call didReceiveMessage on the ones we don’t remove instead of having two
> loops and a boolean.

Hm... that sounds a bit fishy given removeAllMatching could resize the buffer. I'd rather separate the removal part after the loop where we don't have a pointer anymore.

> > Source/WebKit/Platform/IPC/JSIPCBinding.h:55
> > +    object->putDirect(vm, JSC::Identifier::fromString(vm, "type"_s), JSC::jsNontrivialString(vm, type));
> 
> What guarantees non-trivial for this string? Maybe these are always string
> constants. If so, consider making the argument ASCIILiteral instead of const
> String&.

Yes.

> Even type could be ASCIILiteral instead of const String&.

Oh, I thought _s creates ASCIILiteral. I guess it directly creates String string instead. Good to know.

> > Source/WebKit/WebProcess/WebPage/IPCTestingAPI.cpp:909
> > +    JSObjectCallAsFunction(m_context, m_callback, m_callback, WTF_ARRAY_LENGTH(arguments), arguments, nullptr);
> 
> Should be able to use std::size instead of WTF_ARRAY_LENGTH in new code. We
> can probably get rid of WTF_ARRAY_LENGTH.

Oh, that's a good point. Done.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/IPCTestingAPI.mm:257
> > +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
> 
> auto?

Oh, actually, I should be using createWebViewWithIPCTestingAPI that I had recently added. Done that instead.

[Ryosuke Niwa]

Committed r268848: <https://trac.webkit.org/changeset/268848>

[Ryosuke Niwa]

Reopening to attach new patch.

[Ryosuke Niwa]

Created attachment 412118 [details]
Patch

[Ryosuke Niwa]

Created attachment 412119 [details]
Patch

[Darin Adler]

Comment on attachment 411963 [details]
Patch

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

>>> Source/WebKit/Platform/IPC/Connection.cpp:1044
>>> +            m_messageObservers.removeAllMatching([](auto& observer) { return !observer; });
>> 
>> Seems like we could use removeAllMatching to remove while iterating, and call didReceiveMessage on the ones we don’t remove instead of having two loops and a boolean.
> 
> Hm... that sounds a bit fishy given removeAllMatching could resize the buffer. I'd rather separate the removal part after the loop where we don't have a pointer anymore.

No, I don’t agree.

Are you worried about didReceiveMessage calling out while iterating m_messageObservers and then m_messageObservers or this being changed out form under us? If so, then that’s *already* a problem in the code above, which needs to change.

[Darin Adler]

(In reply to Ryosuke Niwa from comment #3)
> > Even type could be ASCIILiteral instead of const String&.
> 
> Oh, I thought _s creates ASCIILiteral. I guess it directly creates String
> string instead. Good to know.

You were right, _s *does* create ASCIILiteral.

I meant that the argument named type could be ASCIILiteral. Yes, "type"_s already is.

[EWS]

Committed r268883: <https://trac.webkit.org/changeset/268883>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412119 [details].