Bug 163777 – NetworkSession: switch to use subclasses for NetworkSession and NetworkDataTask implementations

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The subclass model allows us to choose the implementation at run time, so we could add other implementations like a mock network class to measure performance, and even one for blobs and finally get rid of ResourceHandle in WebKit2.

[Carlos Garcia Campos]

Created attachment 292336 [details]
Patch for EWS

Not asking r? yet because I'm not sure I did the XCode changes right and even in that case I bet it won't build on mac at the moment.

[Carlos Garcia Campos]

Created attachment 292337 [details]
Patch for EWS

[Carlos Garcia Campos]

Created attachment 292340 [details]
Patch for EWS

[Carlos Garcia Campos]

Created attachment 292341 [details]
Patch for EWS

[Carlos Garcia Campos]

Remaining issues are link failures, but I have no idea how to fix those. It complains about static memebers that are implemente din mm files. Maybe the xcdoe changes were not correct in the end.

[Keith Rollin]

I'm looking into the iOS build issues. I get the linker errors in the release build. In the debug build, I get the error below. I've not looked into the source or fix for either of these yet, but I can do that next. I've not tried the macOS builds.

In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:27:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:30:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/NetworkDataTask.h:30:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/Downloads/DownloadID.h:30:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/Platform/IPC/Decoder.h:30:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/Platform/IPC/StringReference.h:31:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/HashTraits.h:26:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/HashFunctions.h:26:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/RefPtr.h:30:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/PassRefPtr.h:24:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/Ref.h:33:
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/TypeCasts.h:42:9: error: static_assert failed "Missing TypeCastTraits specialization"
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/TypeCasts.h:59:63: note: in instantiation of member function 'WTF::TypeCastTraits<const WebKit::NetworkSessionCocoa, const WebKit::NetworkSession, false>::isOfType' requested here
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/WebKitBuild/Debug-iphoneos/usr/local/include/wtf/TypeCasts.h:81:38: note: in instantiation of function template specialization 'WTF::is<WebKit::NetworkSessionCocoa, WebKit::NetworkSession>' requested here
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:93:19: note: in instantiation of function template specialization 'WTF::downcast<WebKit::NetworkSessionCocoa, WebKit::NetworkSession>' requested here

[Keith Rollin]

Some of the build output made it look like there may have been an issue with how the files were added to the project. I can't read an Xcode project file so I didn't try to figure out what (if anything) might be wrong. What I did instead was to revert the changes to the Xcode project and then re-add the new files. That's had some effect to the release build; I'm now getting the following error. The debug build still emits the same error reported above.

/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:302:143: error: 'suggestedFilename' is a private member of 'WebKit::NetworkDataTaskCocoa'
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:27:
In file included from /Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.h:35:
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:70:12: note: declared private here
/Volumes/Data/dev/WebKit/branches/network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:463:22: error: expected the class name after '~' to name the enclosing class
NetworkSessionCocoa::~NetworkSession()
2 errors generated.

[Keith Rollin]

Regarding the Xcode project, I've compared the original patch to my local changed. I think the problem is around line 5465:

 5465   5C20CB9E1BB0DD1800895BB1 /* NetworkSession.cpp */,

In my version, in this vicinity, I have both new implementation files:

+ 532159511DBAE6FC0054AA3C /* NetworkDataTask.cpp */,
+ 532159521DBAE6FC0054AA3C /* NetworkSession.cpp */,

[Keith Rollin]

I've addressed the compilation errors noted here and one or two others. I'll upload a patch to overlay your patch in a few hours.

[Keith Rollin]

Created attachment 292471 [details]
Patch to be applied after the main patch

The attached patch should be applied after applying the patch in comment #4. This patch, however, might need a few tweaks before being acceptable for inclusion in the main patch:

* The change to NetworkDataTaskCocoa.h was meant merely to get the build to work. It's possible that the final change would be to move the affected private functions into the public section.

* The downcast<>() in NetworkDataTaskCocoa.mm was changed to a reinterpret_cast<>(). The downcast<>() wasn't working and I didn't know enough about that facility to get it to work. A real solution might to be get downcast<>() to work.

[Carlos Garcia Campos]

Comment on attachment 292471 [details]
Patch to be applied after the main patch

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

> network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.h:64
> +public:

This is not correct, I think, the problem is that NetworkSessionCocoa is using NetworkDataTaskCocoa::suggestedFilename() directly, so the solution would be to cast there to use NetworkDataTask::suggestedFilename() instead. I think these overrides should remain private.

> network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-95
> +    auto& cocoaSession = reinterpret_cast<NetworkSessionCocoa&>(m_session.get()); // downcast<NetworkSessionCocoa>(m_session.get());
>      if (storedCredentials == WebCore::AllowStoredCredentials) {
> -        m_task = [downcast<NetworkSessionCocoa>(m_session.get()).m_sessionWithCredentialStorage dataTaskWithRequest:nsRequest];
> -        ASSERT(!downcast<NetworkSessionCocoa>(m_session.get()).m_dataTaskMapWithCredentials.contains([m_task taskIdentifier]));
> -        downcast<NetworkSessionCocoa>(m_session.get()).m_dataTaskMapWithCredentials.add([m_task taskIdentifier], this);

I don't know why it worked for Soup, though. I guess we need to either add type traits (adding a pure virtual type() or several isFoo() methods to NetworkSession) or simply use reinterpret_cast since here the network session should always be NetworkSessionCocoa.

> network_session/OpenSource/Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:463
> -NetworkSessionCocoa::~NetworkSession()
> +NetworkSessionCocoa::~NetworkSessionCocoa()

Oops, I forgot this.

[Keith Rollin]

> I think these overrides should remain private.

I'm curious about your desire for this. This is an idiom that I've not seen used before. It seems to me that it could be problematic (for instance, the actual compilation problem I encountered). Others seem to feel the same way: <http://stackoverflow.com/questions/484592/overriding-public-virtual-functions-with-private-functions-in-c>. That article (in part) refers to <https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public>.

So how do you come down on the opposite side of these opinions? What's being communicated by making public methods private in the derived class?

[Carlos Garcia Campos]

(In reply to comment #12)
> > I think these overrides should remain private.
> 
> I'm curious about your desire for this. This is an idiom that I've not seen
> used before. It seems to me that it could be problematic (for instance, the
> actual compilation problem I encountered). Others seem to feel the same way:
> <http://stackoverflow.com/questions/484592/overriding-public-virtual-
> functions-with-private-functions-in-c>. That article (in part) refers to
> <https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public>.
> 
> So how do you come down on the opposite side of these opinions? What's being
> communicated by making public methods private in the derived class?

In this particular case those methods are pure virtual an expected to be used by NetworkDataTask users who don't know about Soup/Cocoa implementations. Those overrides are providing the implementation for the base class. I'm pretty sure we do this in WebKit, but still, I don't have a strong opinion, we can make them public if you think it's better solution.

[Carlos Garcia Campos]

Created attachment 292594 [details]
Patch for EWS

Included xcode fixes, made overrides public in cocoa impl, fixed the NetworkSessionCocoa destructor and switched to use static_cast instead of downcast (I think static_cast should work in this case). Thanks Keith!

[Carlos Garcia Campos]

Created attachment 292600 [details]
Patch

[Alex Christensen]

Comment on attachment 292600 [details]
Patch

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

This is such a huge improvement.  Thanks Carlos! r=me

> Source/WebKit2/NetworkProcess/NetworkSession.cpp:62
> +    ASSERT(isMainThread());

Let's put this assertion outside the #if PLATFORM?

> Source/WebKit2/NetworkProcess/NetworkSession.h:49
> +    virtual void invalidateAndCancel() { };
> +    virtual void clearCredentials() { };

These should be purely virtual.  Put notImplemented() in Soup's.  clearCredentials isn't used yet, but I hope to use it sometime soon.

[Carlos Garcia Campos]

(In reply to comment #16)
> Comment on attachment 292600 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292600&action=review
> 
> This is such a huge improvement.  Thanks Carlos! r=me
> 
> > Source/WebKit2/NetworkProcess/NetworkSession.cpp:62
> > +    ASSERT(isMainThread());
> 
> Let's put this assertion outside the #if PLATFORM?

I didn't add the assert for cocoa because NetworkSessionCocoa::defaultSession() already has that assert. It's unfortunate that this code can't be common, maybe we can find a different way to handle the custom protocol manager in cocoa that doesn't require passing it to the constructor.

> > Source/WebKit2/NetworkProcess/NetworkSession.h:49
> > +    virtual void invalidateAndCancel() { };
> > +    virtual void clearCredentials() { };
> 
> These should be purely virtual.  Put notImplemented() in Soup's. 
> clearCredentials isn't used yet, but I hope to use it sometime soon.

invalidateAndCancel() should probably be implemented by all implementations, but clearCredentials is different, I'm thinking in blobs for example that I already started to move to network data task btw.

[Carlos Garcia Campos]

Committed r207813: <http://trac.webkit.org/changeset/207813>

[Alex Christensen]

> I'm thinking in blobs for example that I
> already started to move to network data task btw.
Hooray!  I think we'll have to get rid of PlatformStrategies::blobRegistry to do this.