Bug 167022 – Ignore Connection Assertion if we are not using connection to send messages

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Ignore Connection Assertion if we are not using connection to send messages

[Megan Gardner]

Created attachment 298793 [details]
Patch

[Megan Gardner]

Created attachment 298803 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 298803 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:106
> -        if (webProcessProxy->connection() != &connection)
> +        if (webProcessProxy->connection(ChildProcessProxy::IgnoreAssertion::Yes) != &connection)

Stepping back a little, this check for connection being the same looks unusual. What is its purpose?

[Megan Gardner]

It is serving as an identifier for the WebProcessProxy. Odd, I know, but it's the best solution to this particular problem.

[Darin Adler]

Comment on attachment 298803 [details]
Patch

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

> Source/WebKit2/UIProcess/ChildProcessProxy.h:55
> -    IPC::Connection* connection() const
> +    enum class IgnoreAssertion { No, Yes };
> +    IPC::Connection* connection(IgnoreAssertion ignoreAssertion = IgnoreAssertion::No) const
>      {
> -        ASSERT(m_connection);
> +        if (ignoreAssertion == IgnoreAssertion::No)
> +            ASSERT(m_connection);
>          return m_connection.get();
>      }

I don’t see any good reason to use a single function with an argument for this. It would be more straightforward to have two separate functions for these two separate purposes.

One good approach would be to have a function with a boolean return value that takes a Connection& and answers whether it is this process proxy’s connection; maybe named “hasThisConnection” or some other phrase like that. Longer term, the one named "connection" that gets you the connection should be changed to return a reference, not a pointer, to express that it returns something that is never null. And of course would continue to assert that it is non-null.

> Source/WebKit2/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:29
>  #import "config.h"
> +#import "ChildProcessProxy.h"
>  #import "WebPasteboardProxy.h"
>  #import "WebProcessProxy.h"

The formatting here is unconventional for WebKit. The only header that should be next to "config.h" without a blank line after it would be the header for this implementation file. If this file does not have its own, then there should be a blank line after "config.h".

This include should not be added. I don’t see any reason we need to include it. We are calling the connection function, so clearly we are including the header that contains the function definition. So there is no additional need to include a header to get the type that is defined next to the function.

[Alexey Proskuryakov]

A pointer is not a good identifier, because the same memory location can be reused.

What is the higher level problem being solved by this check?

[Alex Christensen]

(In reply to comment #6)
> A pointer is not a good identifier, because the same memory location can be
> reused.
> 
> What is the higher level problem being solved by this check?
We need a way to get the WebProcessProxy from WebPasteboardProxy::setPasteboardPathnamesForType so we can use it to check the URL coming from the WebProcess.  I suggested just casting the Connection's client to a WebProcessProxy, but Anders said that would become unsafe and I agree.  We have a Connection, but WebProcessProxy::didReceiveMessage doesn't forward *this to ChildProcessProxy::dispatchMessage because MessageReceiver::didReceiveMessage doesn't always have something forwarding the message to it.  Another solution might be to make a ForwardedMessageReceiver<typename Forwarder> that has a didReceiveMessage that has a reference to the original receiver of the message, but I'm not sure it would be worth it just for this.

[Alexey Proskuryakov]

WebPageProxy just has an m_process member variable for this. That's somewhat unfortunate, as it duplicates information, but it seems better at first glance than making pointer equality tests.

In any case, we shouldn't be using two mechanisms for exactly the same purpose in different proxies.

[Megan Gardner]

Created attachment 299040 [details]
Patch

[Megan Gardner]

Thanks for the suggestion, Darin. I do think this a better way to ask the question. And as we only have a connection, and a list of potential WebProcessProxies, this is the quickest and easiest way of figure out which one we should be talking to. The connections are also meticulously maintained by the WebProcessProxy, so it will be able to tell us if the connection we have is the one it is using.

[Darin Adler]

Comment on attachment 299040 [details]
Patch

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

> Source/WebKit2/UIProcess/ChildProcessProxy.h:55
> +    bool hasConnection(IPC::Connection& connection)

I suggest both making this function const and making the argument a const&.

> Source/WebKit2/UIProcess/ChildProcessProxy.h:57
> +        return (m_connection.get() == &connection);

I suggest omitting the parentheses and the get() because RefPtr == raw pointer should compile without a get().

[Megan Gardner]

https://trac.webkit.org/changeset/210861