Bug 167236 – [SOUP] Custom protocols don't work in private browsing mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Carlos Garcia Campos]

We only register them in the default session. You can reproduce this with the GTK+ MiniBrowser. Open it, enable private browsing in settings dialog and navigate to about:minibrowser.

[Carlos Garcia Campos]

Created attachment 299336 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 299336 [details]
Patch

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

r+ but we need a test to verify that we don't regress

> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:304
> +        return;

I guess you add this for extra safety as we're asserting above. Not sure we need it but no strong feelings.

[Carlos Garcia Campos]

Comment on attachment 299336 [details]
Patch

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

Didn't want to add tests depending on the soon deprecated private browsing setting, I'll add more unit tests when we add the new API for private browsing.

>> Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:304
>> +        return;
> 
> I guess you add this for extra safety as we're asserting above. Not sure we need it but no strong feelings.

No, this is in case setCustomProtocolRequestType hasn't been called, like in the web process. It's true that in that case the type is 0, so we could just check if !type, but yes, added the type check for extra safety

[Carlos Garcia Campos]

Committed r211012: <http://trac.webkit.org/changeset/211012>

[Carlos Garcia Campos]

Reverted r211012 for reason:

It caused a lot of crashes in the network process

Committed r211014: <http://trac.webkit.org/changeset/211014>

[Carlos Garcia Campos]

It's not possible to register a soup request type without types, that's why we installed the feature on every register scheme. I did my tests with MiniBrowser that always registers a scheme, that's why it worked, but fails with all layout tests. I need to rethink how to properly fix this.

[Carlos Garcia Campos]

Created attachment 299509 [details]
Updated patch

Two changes to the previous version:

 * We call setup on all session after every scheme is registered instead of in registerProtocolClass().
 * We check if the class has schemes before adding it to the session.

[Sergio Villar Senin]

Comment on attachment 299509 [details]
Updated patch

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

> Source/WebKit2/ChangeLog:8
> +        We only register them in the default session.

This, combined with the title is the description of the issue, but I lack an explanation of the fix here. Yes I know you talk about specifics on the individual files, but I think it's far easier for the reader to have a general description here.

[Carlos Garcia Campos]

Committed r211140: <http://trac.webkit.org/changeset/211140>

[Csaba Osztrogonác]

(In reply to comment #9)
> Committed r211140: <http://trac.webkit.org/changeset/211140>

It broke the build on the 32 bit GTK bot:
https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/66858

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > Committed r211140: <http://trac.webkit.org/changeset/211140>
> 
> It broke the build on the 32 bit GTK bot:
> https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/66858

Thanks for the heads up, I guess we can't forward declare GType then...