Bug 167236 - [SOUP] Custom protocols don't work in private browsing mode
Summary: [SOUP] Custom protocols don't work in private browsing mode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Soup
Depends on:
Blocks:
 
Reported: 2017-01-20 01:59 PST by Carlos Garcia Campos
Modified: 2017-01-25 06:26 PST (History)
8 users (show)

See Also:


Attachments
Patch (6.35 KB, patch)
2017-01-20 02:04 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (6.42 KB, patch)
2017-01-23 00:24 PST, Carlos Garcia Campos
svillar: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-01-20 01:59:14 PST
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.
Comment 1 Carlos Garcia Campos 2017-01-20 02:04:30 PST
Created attachment 299336 [details]
Patch
Comment 2 Sergio Villar Senin 2017-01-20 02:20:36 PST
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.
Comment 3 Carlos Garcia Campos 2017-01-20 02:29:46 PST
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
Comment 4 Carlos Garcia Campos 2017-01-20 23:41:46 PST
Committed r211012: <http://trac.webkit.org/changeset/211012>
Comment 5 Carlos Garcia Campos 2017-01-21 01:24:07 PST
Reverted r211012 for reason:

It caused a lot of crashes in the network process

Committed r211014: <http://trac.webkit.org/changeset/211014>
Comment 6 Carlos Garcia Campos 2017-01-21 01:26:14 PST
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.
Comment 7 Carlos Garcia Campos 2017-01-23 00:24:56 PST
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.
Comment 8 Sergio Villar Senin 2017-01-25 01:52:45 PST
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.
Comment 9 Carlos Garcia Campos 2017-01-25 04:24:22 PST
Committed r211140: <http://trac.webkit.org/changeset/211140>
Comment 10 Csaba Osztrogonác 2017-01-25 06:16:21 PST
(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
Comment 11 Carlos Garcia Campos 2017-01-25 06:26:43 PST
(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...