Bug 103729 – [GTK] Custom URI schemes stop working on Epiphany using WebKit2 after killing the web process

• Component: WebKit2
• Status: RESOLVED FIXED

[Joaquim Rocha]

1) Open Epiphany and access e.g. about:plugins;
2) Kill the respective running WebKitWebProcess;
3) Try to access about:plugins again

Expected:
The about:plugins works as always.

Outcome:
about:plugins is not found.

I am working on a solution and will have a patch soon.

[Joaquim Rocha]

Created attachment 177001 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 177001 [details]
Patch

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

Thanks for the patch, it looks good, I've just a few comments.

> Source/WebKit2/ChangeLog:3
> +        Fixes the use of URI schemes when using libsoup

This is not accurate, the bug title probably express better the problem so I would just skip this line.

> Source/WebKit2/ChangeLog:9
> +        When a URI scheme is registered and the WebProcess is killed,
> +        those schemes would not work anymore after the process is relaunched.
> +
> +        This was observed in Epiphany and possibly affects any port that
> +        uses libsoup.

All this text should go after the Reviewed by line, replacing the Additional information line.

> Source/WebKit2/ChangeLog:34
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> +
> +        * Shared/WebProcessCreationParameters.cpp:
> +        (WebKit::WebProcessCreationParameters::encode):
> +        (WebKit::WebProcessCreationParameters::decode):
> +        * Shared/WebProcessCreationParameters.h:
> +        (WebProcessCreationParameters):
> +        * UIProcess/WebContext.cpp:
> +        (WebKit::WebContext::createNewWebProcess):
> +        * UIProcess/soup/WebSoupRequestManagerProxy.cpp:
> +        (WebKit::WebSoupRequestManagerProxy::registerURIScheme):
> +        (WebKit::WebSoupRequestManagerProxy::getRegisteredURISchemes):
> +        (WebKit):
> +        * UIProcess/soup/WebSoupRequestManagerProxy.h:
> +        (WebSoupRequestManagerProxy):
> +        * WebProcess/WebProcess.cpp:
> +        (WebKit::WebProcess::initializeWebProcess):
> +        * WebProcess/soup/WebSoupRequestManager.h:
> +        (WebSoupRequestManager):

As the comment says, please add per-function descriptions briefly explaining the changes.

> Source/WebKit2/Shared/WebProcessCreationParameters.h:79
> +    Vector<String> urlSchemesRegisteredForSoup;

I would remove the ForSoup suffix.

> Source/WebKit2/UIProcess/WebContext.cpp:405
> +#if USE(SOUP)
> +    const Vector<String> &registeredUriSchemes = m_soupRequestManagerProxy->getRegisteredURISchemes();
> +    for (Vector<String>::const_iterator it = registeredUriSchemes.begin(); it != registeredUriSchemes.end(); it++)
> +        parameters.urlSchemesRegisteredForSoup.append(*it);
> +#endif

Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp. We should do the same for EFL in Source/WebKit2/UIProcess/efl/WebContextEfl.cpp
Can't you do a simple assignment instead of iterating the vector? 

parameters.urlSchemesRegistered = m_soupRequestManagerProxy->getRegisteredURISchemes();

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64
> +    m_registeredURISchemes.append(scheme);

The API layer should make sure the same scheme is not registered twice, now that we have a vector with the list of schemes, we could add an ASSERT to check the scheme is not already in the Vector.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:97
> +Vector<String>& WebSoupRequestManagerProxy::getRegisteredURISchemes()
> +{
> +    return m_registeredURISchemes;
> +}

I think it would be better to return const Vector<String>& and the function should be const too. The name of the function should be registeredURISchemes(), see http://www.webkit.org/coding/coding-style.html#names-setter-getter. Also for simple getters like this, we typically implement them in the header.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.h:68
> +

Remove this extra line

> Source/WebKit2/WebProcess/WebProcess.cpp:288
> +#if USE(SOUP)
> +    for (Vector<String>::const_iterator it = parameters.urlSchemesRegisteredForSoup.begin(); it != parameters.urlSchemesRegisteredForSoup.end(); it++)
> +        m_soupRequestManager.registerURIScheme(*it);
> +#endif

Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. 
I think the vector iteration is easier to read as 

for (size_t i = 0; i < parameters.urlSchemesRegistered.size(); ++i)
    m_soupRequestManager.registerURIScheme(parameters.urlSchemesRegistered[i]);

[Joaquim Rocha]

Hi,

I will upload a new patch addressing your comments.
About the vector iteration, I'll leave that as I did because it's common in C++ and *arguably* results in faster code.

Thanks for the review.

[Joaquim Rocha]

Forgot to comment on some of your suggestions:

>> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64
>> +    m_registeredURISchemes.append(scheme);
>
> The API layer should make sure the same scheme is not registered twice, now that >we have a vector with the list of schemes, we could add an ASSERT to check the >scheme is not already in the Vector.

This means that we have to iterate (one way or another) through the vector which, although it might be insignificant, is extra processing.

> Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp.

I didn't put it in there because I need the soupRequestManagerProxy that the WebContext.cpp already has.
Also, if this works for both the Gtk and Efl implementations, isn't it better than touching two files?

> Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. 

I didn't put it in there because I need the soupRequestManager that the WebProcess.cpp already has.

Let me know if I'm missing some smart way to access these members from the files you mentioned.

[Carlos Garcia Campos]

(In reply to comment #3)
> Hi,
> 
> I will upload a new patch addressing your comments.
> About the vector iteration, I'll leave that as I did because it's common in C++ and *arguably* results in faster code.

Why faster code? The code is easier to read as a simple assignment, so if possible I think it would be better.

> Thanks for the review.

np :-)

[Carlos Garcia Campos]

(In reply to comment #4)
> Forgot to comment on some of your suggestions:
> 
> >> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64
> >> +    m_registeredURISchemes.append(scheme);
> >
> > The API layer should make sure the same scheme is not registered twice, now that >we have a vector with the list of schemes, we could add an ASSERT to check the >scheme is not already in the Vector.
> 
> This means that we have to iterate (one way or another) through the vector which, although it might be insignificant, is extra processing.

ASSERT is no-op for a release build, so it's not a problem.

> > Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp.
> 
> I didn't put it in there because I need the soupRequestManagerProxy that the WebContext.cpp already has.
> Also, if this works for both the Gtk and Efl implementations, isn't it better than touching two files?

WebContextGtk.cpp and WebContextEfl.cpp are the same class than WebContext, the files simply add implementation for methods specific to the platform, so you can use soupRequestManagerProxy from them. WebContext::platformInitializeWebProcess() was added exactly for that, so I prefer code in platform specific files and #ifdefs in cross platform code, when possible.
 
> > Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. 
> 
> I didn't put it in there because I need the soupRequestManager that the WebProcess.cpp already has.

Same here, you can use soupRequestManager from WebProcessSoup.cpp, it's the same class.

> Let me know if I'm missing some smart way to access these members from the files you mentioned.

Nothing smart, simply use them :-)

[Joaquim Rocha]

Created attachment 177214 [details]
Patch

[EFL EWS Bot]

Comment on attachment 177214 [details]
Patch

Attachment 177214 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15120285

[Joaquim Rocha]

Created attachment 177219 [details]
Patch

[Joaquim Rocha]

The last patch (2012-12-03 03:17 PST) fixes the issues found the in EFL's bot output.

[Carlos Garcia Campos]

Comment on attachment 177214 [details]
Patch

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

> Source/WebKit2/ChangeLog:33
> +        * WebProcess/soup/WebSoupRequestManager.h: Make registerURIScheme public

nit: End all sentences with a period.

> Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:43
> -    notImplemented();
> +    parameters.urlSchemesRegistered = m_soupRequestManagerProxy->registeredURISchemes();

I guess you need to include WebSoupRequestManagerProxy.h in this file too.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:65
> +    ASSERT(!m_registeredURISchemes.find(scheme));

find() returns notFound when item is not found, so you should use either m_registeredURISchemes.find(scheme) != notFound, or use Vector::contains() that returns a bool.

> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.h:57
> +    const Vector<String>& registeredURISchemes() const
> +    {
> +        return m_registeredURISchemes;
> +    }

This could be a single line.

[Joaquim Rocha]

Created attachment 177223 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 177223 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 177223 [details]
Patch

Clearing flags on attachment: 177223

Committed r136389: <http://trac.webkit.org/changeset/136389>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.