Bug 164922 – [SOUP] Simplify custom protocols handler implementation

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are using too many classes for this and also the C API that is not needed, because both GTK+ and EFL have their own APIs and this is not used by WTR at all.

[Carlos Garcia Campos]

Created attachment 295145 [details]
Patch

Not asking r? yet because it won't build for EFL, I'll check EWS to try to fix the build

[Carlos Garcia Campos]

Created attachment 295146 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295148 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295151 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295152 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295155 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295156 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295157 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295158 [details]
Patch

[Carlos Garcia Campos]

Created attachment 295159 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 295159 [details]
Patch

EFL EWS is green now, but of course the changes are not tested at all. If someone from EFL could try it out it would be great, otherwise I hope test bots will do their work.

[Michael Catanzaro]

Comment on attachment 295159 [details]
Patch

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

r=me on the GTK+ and EFL changes. I think an owner should take a quick look at the CustomProtocolManagerProxy and WebProcessPool changes, even though the changes are all in #ifdefs, just in case of design objection.

> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request_private.h:59
> +    WebKit::CustomProtocolManagerProxy* manager() const { return m_wkRequestManager; }

The return value here should be a const pointer. If you need to return a non-const pointer, then you should add an overload that is not a const member function.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:-170
> -#if USE(SOUP)
> -    , m_initialHTTPCookieAcceptPolicy(HTTPCookieAcceptPolicyOnlyFromMainDocumentDomain)
> -#endif

This patch is big and confusing enough that I would prefer to do this in a separate patch.

[Michael Catanzaro]

(In reply to comment #12)
> The return value here should be a const pointer. If you need to return a
> non-const pointer, then you should add an overload that is not a const
> member function.

I mean this:

const WebKit::CustomProtocolManagerProxy*

(pointer to a const object... not technically a const pointer)

[Carlos Garcia Campos]

Comment on attachment 295159 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_url_scheme_request_private.h:59
>> +    WebKit::CustomProtocolManagerProxy* manager() const { return m_wkRequestManager; }
> 
> The return value here should be a const pointer. If you need to return a non-const pointer, then you should add an overload that is not a const member function.

This is EFL code I can't test, so I prefer to not change anything in this part, unless EFL guys confirm the change is ok.

[Carlos Garcia Campos]

(In reply to comment #12)
> Comment on attachment 295159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295159&action=review
> 
> r=me on the GTK+ and EFL changes. I think an owner should take a quick look
> at the CustomProtocolManagerProxy and WebProcessPool changes, even though
> the changes are all in #ifdefs, just in case of design objection.
> 

Darin/Alex? The only cross-platform change I did was adding CustomProtocolManagerProxy::processDidClose() that is called from NetworkProcessProxy::didClose() and it's empty in CustomProtocolManagerProxyMac.mm

[Michael Catanzaro]

(In reply to comment #14)
> This is EFL code I can't test, so I prefer to not change anything in this
> part, unless EFL guys confirm the change is ok.

If it builds, it's OK.

[Darin Adler]

Comment on attachment 295159 [details]
Patch

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

There’s s surprising amount of SOUP-specific #if in all these classes. As a direction for WebKit2 seems like a step in the wrong direction; would be nice to find a more elegant way of doing it.

But I approve of landing it as is.

> Source/WebKit2/UIProcess/Network/CustomProtocols/CustomProtocolManagerProxy.h:67
> +#if USE(SOUP)
> +    void didReceiveResponse(uint64_t customProtocolID, const WebCore::ResourceResponse&);
> +    void didLoadData(uint64_t customProtocolID, const IPC::DataReference&);
> +    void didFailWithError(uint64_t customProtocolID, const WebCore::ResourceError&);
> +    void didFinishLoading(uint64_t customProtocolID);
> +#endif

This doesn’t seem quite right to me; I don’t have strong objections to landing it, but it seems like the wrong direction long term. The people I would ask about alternatives are Sam and Anders.

> Source/WebKit2/UIProcess/WebProcessPool.h:140
> +    void setCustomProtocolManagerClient(std::unique_ptr<API::CustomProtocolManagerClient>);

A function like this should take unique_ptr&&, not just unique_ptr. See <http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-passed.html> for one version of the rationale.

[Carlos Garcia Campos]

(In reply to comment #17)
> Comment on attachment 295159 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295159&action=review
> 
> There’s s surprising amount of SOUP-specific #if in all these classes. As a
> direction for WebKit2 seems like a step in the wrong direction; would be
> nice to find a more elegant way of doing it.

Yes, I agree, but I would say platform-specific #if, not just soup, in general in custom protocols implementation. I think this is because the way it works and the way it's exposed to the API users is different between soup based ports and Apple ones. For example, in Cocoa custom protocols are always registered globally by WKBrowsingContextController, while soup based ports register the custom protocols per WebProcessPool, so I used ifdefs to save the registered protocols in each WebProcessPool to avoid having them duplicated in Cocoa, but there's nothing soup specific there. In any case, platform ifdefs can be reduced with just a few refactoring I think, I'll check it in more detail.

> But I approve of landing it as is.

Thank you, I'll try to refactor it after the cleanup, because it will be easier.

> > Source/WebKit2/UIProcess/Network/CustomProtocols/CustomProtocolManagerProxy.h:67
> > +#if USE(SOUP)
> > +    void didReceiveResponse(uint64_t customProtocolID, const WebCore::ResourceResponse&);
> > +    void didLoadData(uint64_t customProtocolID, const IPC::DataReference&);
> > +    void didFailWithError(uint64_t customProtocolID, const WebCore::ResourceError&);
> > +    void didFinishLoading(uint64_t customProtocolID);
> > +#endif
> 
> This doesn’t seem quite right to me; I don’t have strong objections to
> landing it, but it seems like the wrong direction long term. The people I
> would ask about alternatives are Sam and Anders.

This can be fixed for sure, I'll do it in a follow up.

> > Source/WebKit2/UIProcess/WebProcessPool.h:140
> > +    void setCustomProtocolManagerClient(std::unique_ptr<API::CustomProtocolManagerClient>);
> 
> A function like this should take unique_ptr&&, not just unique_ptr. See
> <http://scottmeyers.blogspot.com/2014/07/should-move-only-types-ever-be-
> passed.html> for one version of the rationale.

I followed the current pattern of all other clients in WebProcessPool, but I agree anyway, I'll change it. There's another side effect, though, to do it that way that is important to know when doing this, I learned it recently the hard way in bug #164631. When using the sink paramater there's no move constructor for the parameter so that when the value is moved the ownership is transferred, but the moved value is not set to nullptr, you have to use exchange or swap in that particular case if you need to ensure the value is nullptr after the move.

[Carlos Garcia Campos]

Committed r208959: <http://trac.webkit.org/changeset/208959>

[Darin Adler]

(In reply to comment #18)
> I learned it recently
> the hard way in bug #164631. When using the sink paramater there's no move
> constructor for the parameter so that when the value is moved the ownership
> is transferred, but the moved value is not set to nullptr, you have to use
> exchange or swap in that particular case if you need to ensure the value is
> nullptr after the move.

I need to learn more about this. Could you show me a simple test program that demonstrates a moved value and a unique_ptr that is not set to nullptr after the value is moved out? I don’t think this exists, but a sample .cpp file could help me understand.

[Carlos Garcia Campos]

(In reply to comment #20)
> (In reply to comment #18)
> > I learned it recently
> > the hard way in bug #164631. When using the sink paramater there's no move
> > constructor for the parameter so that when the value is moved the ownership
> > is transferred, but the moved value is not set to nullptr, you have to use
> > exchange or swap in that particular case if you need to ensure the value is
> > nullptr after the move.
> 
> I need to learn more about this. Could you show me a simple test program
> that demonstrates a moved value and a unique_ptr that is not set to nullptr
> after the value is moved out? I don’t think this exists, but a sample .cpp
> file could help me understand.

Sure, check this:

http://people.igalia.com/cgarcia/unique-sink.cpp

There are 3 cases:

 * Pass by value: a new unique_ptr is created and the move constructor is used. The new unique_ptr takes ownership and the other one is cleared.

 * Pass sink parameter: one of the advantages of move over the old PassRefPtr is that we don't need to adopt it to not leak it. So in this case the new owner doesn't call move again, it simply uses the value. In this case no other unique_ptr is created, as explained in the link you shared, and therefore the move constructor is not called, the received unique_ptr is exactly the same as the passed one, so it can't be cleared.

 * Pass sink parameter, but in this case the function moves again. In this case a new unique_ptr is created locally and the move constructor is called, clearing the passed unique_ptr.

Check the output of the program:

$ ./unique-sink 
value: u(0x7ffda4ba2410) p(0x55c06b529c20) v(25)
f1: u(0x7ffda4ba2400) p((nil)), v(0)
sink: u(0x7ffda4ba23f0) p(0x55c06b529c20) v(50)
f2: u(0x7ffda4ba23f0) p(0x55c06b529c20) v(50)
sinkAndMove: u(0x7ffda4ba23e0) p(0x55c06b52a050) v(150)
f3: u(0x7ffda4ba23e0) p((nil)) v(0)

u: unique_ptr
p: pointer
v: value

So, this is only a problem when we move a unique_ptr and the receiver doesn't move again to unique_ptr (but not to another unique_ptr&&, of course). In such cases if we want to ensure the unique_ptr is cleared we need to either use a local variable to move first and then transfer that, or use std::exchange, but in both cases the effect would be the same as passing by value, because a new unique_ptr will be created. The only way I can think of to avoid the creation of the second unique_ptr would be to explicitly set to nullptr after the function call:

sink (std::move(f1));
f1 = nullptr;

Instead of relying on move clearing the unique_ptr. If the function doesn't move again, the value is not useful after the function call, so we can clear it manually.

[Darin Adler]

Yes, I understand now. Passing to a std::unique_ptr&& is "might move" operation whereas passing to a std::unique_ptr is a "definitely move" operation.