Bug 150898 – [WK2] Extract networking code out of NetworkResourceLoader class to improve reusability

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Extract networking code out of NetworkResourceLoader class to improve reusability (will be used for speculative revalidation) and simplify the NetworkResourceLoader.

[Chris Dumez]

Created attachment 264799 [details]
WIP Patch

[Chris Dumez]

Created attachment 264800 [details]
WIP Patch

[Chris Dumez]

Created attachment 264801 [details]
WIP Patch

[Chris Dumez]

Created attachment 264802 [details]
Patch

[Alex Christensen]

Comment on attachment 264802 [details]
Patch

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

r=me with a few comments:

> Source/WebKit2/ChangeLog:9
> +        reusability (will be used for speculative revalidation) and simplify

The speculative revalidator will be a NetworkLoadClient, right?

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:209
> -    loader->handle()->continueDidReceiveResponse();
> +    loader->networkLoad()->handle()->continueDidReceiveResponse();

I guess the assumption that the loader has a networkLoad and the networkLoad has a handle is just as good as the old assumption that the we have a loader with a handle.

> Source/WebKit2/NetworkProcess/NetworkLoadParameters.cpp:44
> +NetworkLoadParameters::NetworkLoadParameters()
> +    : webPageID(0)
> +    , webFrameID(0)
> +    , sessionID(SessionID::emptySessionID())
> +    , contentSniffingPolicy(SniffContent)
> +    , allowStoredCredentials(DoNotAllowStoredCredentials)
> +    , clientCredentialPolicy(DoNotAskClientForAnyCredentials)
> +    , shouldClearReferrerOnHTTPSToHTTPRedirect(true)
> +    , defersLoading(false)
> +    , needsCertificateInfo(false)

If this constructor is needed, you should have webPageID { 0 }; etc. in the header instead of here.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)

This should be an actual constructor.

[Chris Dumez]

Comment on attachment 264802 [details]
Patch

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

>> Source/WebKit2/ChangeLog:9
>> +        reusability (will be used for speculative revalidation) and simplify
> 
> The speculative revalidator will be a NetworkLoadClient, right?

Yes.

>> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:209
>> +    loader->networkLoad()->handle()->continueDidReceiveResponse();
> 
> I guess the assumption that the loader has a networkLoad and the networkLoad has a handle is just as good as the old assumption that the we have a loader with a handle.

That's right. handle() could probably return a reference now though. I'll see if I can make this change.

>> Source/WebKit2/NetworkProcess/NetworkLoadParameters.cpp:44
>> +    , needsCertificateInfo(false)
> 
> If this constructor is needed, you should have webPageID { 0 }; etc. in the header instead of here.

Right, I'll move the initializations inline.

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
>> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)
> 
> This should be an actual constructor.

You mean have a NetworkLoadParameters constructor that takes a NetworkResourceLoadParameters in argument?

[Alex Christensen]

(In reply to comment #6)
> >> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:163
> >> +static NetworkLoadParameters constructNetworkLoadParameters(const NetworkResourceLoadParameters& parameters, const Optional<ResourceRequest>& updatedRequest)
> > 
> > This should be an actual constructor.
> 
> You mean have a NetworkLoadParameters constructor that takes a
> NetworkResourceLoadParameters in argument?
Yes.

[Chris Dumez]

Created attachment 264808 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264808 [details]
Patch

Clearing flags on attachment: 264808

Committed r192038: <http://trac.webkit.org/changeset/192038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change seems to have caused some new crashes:

Crashlog
<https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r192038%20(8097)/fast/inspector-support/matchedrules-crash-log.txt>

Run:
<https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/8097>

[Chris Dumez]

(In reply to comment #11)
> This change seems to have caused some new crashes:
> 
> Crashlog
> <https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> r192038%20(8097)/fast/inspector-support/matchedrules-crash-log.txt>
> 
> Run:
> <https://build.webkit.org/builders/
> Apple%20Yosemite%20Debug%20WK2%20%28Tests%29/builds/8097>

It looks like the SandBoxExtension has a non-zero useCount when it is destroyed and therefore hits an assertion. I am trying to figure out how this is possible. NetworkResourceLoader::cleanup() is called and it should revoke the sandbox extension.

[Chris Dumez]

Created attachment 264826 [details]
Patch

[Chris Dumez]

Comment on attachment 264808 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-211
> -        return;

We used to return early after calling setDefersLoading() on the handle here and I stopped doing this by mistake after my change.

[Chris Dumez]

Comment on attachment 264826 [details]
Patch

Clearing flags on attachment: 264826

Committed r192044: <http://trac.webkit.org/changeset/192044>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.