Summary: | [WK2] Extract networking code out of NetworkResourceLoader class to improve reusability | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | achristensen, andersca, commit-queue, koivisto, ryanhaddad | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | 150937 | ||||||||||||||||
Bug Blocks: | 150856 | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2015-11-04 10:40:46 PST
Created attachment 264799 [details]
WIP Patch
Created attachment 264800 [details]
WIP Patch
Created attachment 264801 [details]
WIP Patch
Created attachment 264802 [details]
Patch
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. 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? (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. Created attachment 264808 [details]
Patch
Comment on attachment 264808 [details] Patch Clearing flags on attachment: 264808 Committed r192038: <http://trac.webkit.org/changeset/192038> All reviewed patches have been landed. Closing bug. 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> (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. Created attachment 264826 [details]
Patch
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. Comment on attachment 264826 [details] Patch Clearing flags on attachment: 264826 Committed r192044: <http://trac.webkit.org/changeset/192044> All reviewed patches have been landed. Closing bug. |