| Summary: | [SOUP] Custom protocols don't work in private browsing mode | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
| Component: | Platform | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | berto, bugs-noreply, commit-queue, danw, gustavo, mcatanzaro, mrobinson, ossy | ||||||
| Priority: | P2 | Keywords: | Soup | ||||||
| Version: | WebKit Local Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Carlos Garcia Campos
2017-01-20 01:59:14 PST
Created attachment 299336 [details]
Patch
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 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 Committed r211012: <http://trac.webkit.org/changeset/211012> Reverted r211012 for reason: It caused a lot of crashes in the network process Committed r211014: <http://trac.webkit.org/changeset/211014> 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. 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 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. Committed r211140: <http://trac.webkit.org/changeset/211140> (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 (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... |