Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1294243004:
PlzNavigate: Make ServiceWorker work with PlzNavigate. (Closed)
|
Created:
5 years, 4 months ago by Fabrice (no longer in Chrome) Modified:
5 years ago CC:
chromium-reviews, jsbell+serviceworker_chromium.org, serviceworker-reviews, creis+watch_chromium.org, mlamouri+watch-content_chromium.org, tzik, nasko+codewatch_chromium.org, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, kinuko+serviceworker, kinuko+watch, blink-worker-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Make ServiceWorker work with PlzNavigate.
BUG=440463
Patch Set 1 #Patch Set 2 : Style, placeholders, all that stuff #
Total comments: 31
Patch Set 3 : Review comments #Patch Set 4 : Rebase #
Total comments: 22
Patch Set 5 : Review comments #Patch Set 6 : Rebase #
Total comments: 23
Patch Set 7 : Review comments #
Total comments: 32
Patch Set 8 : Move dummy SWProviderHost to SWRequestHandler #Patch Set 9 : Rebase #
Total comments: 38
Patch Set 10 : Glorious Post-Blink Merge Rebase #
Total comments: 2
Patch Set 11 : New design, new ownership rules, all that #Patch Set 12 : Rebase #
Total comments: 71
Patch Set 13 : Review comments #Patch Set 14 : Missed a spot. #
Total comments: 30
Patch Set 15 : Rebase #Patch Set 16 : Review comments #
Total comments: 53
Patch Set 17 : Rebase #Patch Set 18 : Cleanup + review comments #Patch Set 19 : Cleanup includes and forward decls. #
Total comments: 40
Patch Set 20 : Review comments #
Total comments: 5
Messages
Total messages: 67 (4 generated)
Follow-up from https://codereview.chromium.org/1257553002/ I removed the navigation_provider_id. In PlzNavigate, for browser-initiated navigations, we will SWProviderHost will be initialized with negative IDs, then passed to the renderer who will keep the negative ID for their SWNetworkProvider. This required a lot of shuffling things around and there are some things I am not very happy with, namely the ton of extra arguments in BeginNavigationParams. Suggestions welcome, this is a pretty large patch and we may be able to break it in smaller CLs.
Thanks for working through this one Fabrice? I'm not super ecstatic about all the parameters we need to add to the navigation structs, so let's see if there is a way to minimize them. https://codereview.chromium.org/1294243004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:101: REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL), I don't think we can hardcode REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL here. Let's make it really reflect whether it is the main frame or a subframe. https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:749: navigation_provider_host->CompleteCrossSiteTransfer( There are no cross-site transfers in PlzNavigate. Why do we need this here? https://codereview.chromium.org/1294243004/diff/20001/content/child/web_url_r... File content/child/web_url_request_util.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/child/web_url_r... content/child/web_url_request_util.h:38: // Helper functions to convert enums from the blink type to thecontent type. nit: s/thecontent/the content/ https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:120: FetchRequestMode fetch_request_mode, Can we skip adding some of these? For example, FetchRequestMode doesn't really make sense in the case of navigation requests. This aren't fetch() calls. Same with the credentials mode and redirect mode. https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:123: RequestContextType request_context_type, This one can also be only one of two values, right? Looking through the enum, only _FRAME and _IFRAME make sense. https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:436: FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), -1); Please use a symbolic constant, not the literal integer. https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:568: FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), -1); Ditto. https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2528: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; Can we DCHECK here that this is not the top-level RenderFrame? https://codereview.chromium.org/1294243004/diff/20001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/test/test_rende... content/test/test_render_frame_host.cc:335: REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL); This can be called for subframes too, let's not hardcode things that we know about.
Thanks for taking over this!! https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:739: if (provider_id < kInvalidServiceWorkerProviderId) { nit: Could we have a small helper function to check this somewhere in common/ and consistently use it to make the intention clearer? (e.g. ServiceWorkerUtils::IsBrowserAssignedProviderId()) https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:749: navigation_provider_host->CompleteCrossSiteTransfer( On 2015/08/20 16:49:50, nasko wrote: > There are no cross-site transfers in PlzNavigate. Why do we need this here? This method is used to complete the NavigationProviderHost initialization. I think we want to create a new method for this, which DCHECKs on provider_id and process_id to see if the provider host's created for PlzNavigate and calls CompleteCrossSiteTransfer internally (or calls the same internal method as CompleteCrossSiteTransfer). https://codereview.chromium.org/1294243004/diff/20001/content/child/service_w... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/child/service_w... content/child/service_worker/service_worker_network_provider.cc:52: int browser_provider_id) { provider_id_(kInvalidServiceWorkerProviderId) here, remove NOTREACHED on line 64, and just DCHECK_NE(provider_id_, kInvalidServiceWorkerProviderId) on line 68?
Thanks! A few comments on the navigation part of the patch. I second Nasko's suggestion not to put too much parameters in BeginNavigationParams. https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:138: // True if the ServiceWorker should be skipped. In which cases is skip_service_worker set to true? https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4828: BeginNavigationParams( If we remove parameters such as FetchCredentialsMode etc... from BeginNavigationParams as nasko suggested, can we DCHECK here that we are getting the values we expect?
Thanks for the comments everyone! New patch, less arguments in NavigationParams, different problems! https://codereview.chromium.org/1294243004/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:101: REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL), On 2015/08/20 16:49:50, nasko wrote: > I don't think we can hardcode REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL here. Let's > make it really reflect whether it is the main frame or a subframe. Done. https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:739: if (provider_id < kInvalidServiceWorkerProviderId) { On 2015/08/21 14:00:44, kinuko wrote: > nit: Could we have a small helper function to check this somewhere in common/ > and consistently use it to make the intention clearer? > > (e.g. ServiceWorkerUtils::IsBrowserAssignedProviderId()) Done. https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:749: navigation_provider_host->CompleteCrossSiteTransfer( On 2015/08/21 14:00:44, kinuko wrote: > On 2015/08/20 16:49:50, nasko wrote: > > There are no cross-site transfers in PlzNavigate. Why do we need this here? > > This method is used to complete the NavigationProviderHost initialization. I > think we want to create a new method for this, which DCHECKs on provider_id and > process_id to see if the provider host's created for PlzNavigate and calls > CompleteCrossSiteTransfer internally (or calls the same internal method as > CompleteCrossSiteTransfer). Agreed, bad name for PlzNavigate. I'm not sure we also need all the stuff done in CompleteCrossSiteTransfer, I trimmed it down a bit. Done. https://codereview.chromium.org/1294243004/diff/20001/content/child/service_w... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/child/service_w... content/child/service_worker/service_worker_network_provider.cc:52: int browser_provider_id) { On 2015/08/21 14:00:44, kinuko wrote: > provider_id_(kInvalidServiceWorkerProviderId) here, > remove NOTREACHED on line 64, and just > DCHECK_NE(provider_id_, kInvalidServiceWorkerProviderId) on line 68? This won't work because GenerateProviderIdForType can return -1 if type is SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME. I refactored the code around to add a DCHECK. https://codereview.chromium.org/1294243004/diff/20001/content/child/web_url_r... File content/child/web_url_request_util.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/child/web_url_r... content/child/web_url_request_util.h:38: // Helper functions to convert enums from the blink type to thecontent type. On 2015/08/20 16:49:50, nasko wrote: > nit: s/thecontent/the content/ Done. https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:120: FetchRequestMode fetch_request_mode, On 2015/08/20 16:49:50, nasko wrote: > Can we skip adding some of these? For example, FetchRequestMode doesn't really > make sense in the case of navigation requests. This aren't fetch() calls. Same > with the credentials mode and redirect mode. I removed them and DCHEK'd them per clamy's suggestion. https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:123: RequestContextType request_context_type, On 2015/08/20 16:49:50, nasko wrote: > This one can also be only one of two values, right? Looking through the enum, > only _FRAME and _IFRAME make sense. I did some quick and dirty testing, these are the values I managed to get: REQUEST_CONTEXT_TYPE_FORM REQUEST_CONTEXT_TYPE_HYPERLINK REQUEST_CONTEXT_TYPE_LOCATION https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:138: // True if the ServiceWorker should be skipped. On 2015/08/24 12:27:35, clamy wrote: > In which cases is skip_service_worker set to true? There are a few cases: -The request was fallbacked through the network, it should not be sent to the ServiceWorker again. -NPAPI does not use ServiceWorker (though that is going away soon) -It is also set to true in Blink in many places. I'm not sure what they all correspond to. I think some are for reload requests, while others are for fallbacks. kinuko@, can you confirm these are the only use cases? Also, what kind of reloads should trigger skipping the ServiceWorker? https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:436: FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), -1); On 2015/08/20 16:49:50, nasko wrote: > Please use a symbolic constant, not the literal integer. Done. https://codereview.chromium.org/1294243004/diff/20001/content/public/test/ren... content/public/test/render_view_test.cc:568: FrameMsg_UILoadMetricsReportType::NO_REPORT, GURL(), GURL(), -1); On 2015/08/20 16:49:50, nasko wrote: > Ditto. Done. https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2528: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; On 2015/08/20 16:49:50, nasko wrote: > Can we DCHECK here that this is not the top-level RenderFrame? Done. https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:4828: BeginNavigationParams( On 2015/08/24 12:27:35, clamy wrote: > If we remove parameters such as FetchCredentialsMode etc... from > BeginNavigationParams as nasko suggested, can we DCHECK here that we are getting > the values we expect? Done. https://codereview.chromium.org/1294243004/diff/20001/content/test/test_rende... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/test/test_rende... content/test/test_render_frame_host.cc:335: REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL); On 2015/08/20 16:49:51, nasko wrote: > This can be called for subframes too, let's not hardcode things that we know > about. Done.
https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:138: // True if the ServiceWorker should be skipped. On 2015/08/26 13:23:24, Fabrice wrote: > On 2015/08/24 12:27:35, clamy wrote: > > In which cases is skip_service_worker set to true? > > There are a few cases: > -The request was fallbacked through the network, it should not be sent to the > ServiceWorker again. > -NPAPI does not use ServiceWorker (though that is going away soon) > -It is also set to true in Blink in many places. I'm not sure what they all > correspond to. I think some are for reload requests, while others are for > fallbacks. kinuko@, can you confirm these are the only use cases? Also, what > kind of reloads should trigger skipping the ServiceWorker? I think your understanding is correct. Fallback handling needs to go back and forth between processes in some cases (e.g. redirect, CORS fallback etc) and that's making the current flag handling code less understandable. (We're willing to cleanup these)
https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2528: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; On 2015/08/20 16:49:50, nasko (out until Sept 1st) wrote: > Can we DCHECK here that this is not the top-level RenderFrame? I added a DCHECK on is_subframe_, but some tests did not like it: SitePerProcessBrowserTest.DynamicSandboxFlagsRemoteToLocal SitePerProcessBrowserTest.DynamicSandboxFlags SitePerProcessBrowserTest.SandboxFlagsReplication Is it the right check to do here?
Thanks! A few comments on my part. https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_LOCATION, I'm not sure this is the best context type for a browser-initiated navigation. I think we've been using LINK for this purpose. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.h:65: NavigationURLLoaderImplCore* loader); I'm a bit concerned about providing a raw pointer to the NavigationURLLoaderImplCore, since the URLRequest will outlive it (it gets destroyed when the request is ready to commit). I think transitioning to a weak pointer would avoid possible edge cases here. https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... content/common/navigation_params.h:139: // load, or a sub objects load. Is this really needed? You already know if you are loading a main frame or a subframe on the browser side (and you _should not_ trust the information from the renderer if you have it on the browser side). It seems weird that a sub object load results in a navigation. Does it happen in practice? If it does, this seems to me to be a bug taht we should fix. https://codereview.chromium.org/1294243004/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:4873: FetchRedirectMode::FOLLOW_MODE); Can we also precise the different possibilities for the request context type? As mentioned in navigation_params.h, I think we may be able to get away from sending the frame type.
Thanks for taking care of this. I'd like to check: do we need PlzNavigate specific ServiceWorker tests? Are there tests already in place that go over renderer vs browser initiated navigations? https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_LOCATION, On 2015/08/27 11:53:45, clamy wrote: > I'm not sure this is the best context type for a browser-initiated navigation. I > think we've been using LINK for this purpose. If you do this change here remember that there are other places where this same value was used as default that should be changed as well. https://codereview.chromium.org/1294243004/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.h:60: void SetServiceWorkerProviderHost(scoped_ptr<ServiceWorkerProviderHost>); nit: new line here. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:107: } If-blocks existing solely for CHECK-ing something are generally frowned upon. I recommend changing this to: CHECK_IMPLIES(render_process_id == kVirtualProcessIDForBrowserRequest, base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kEnableBrowserSideNavigation)); https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:564: void ServiceWorkerProviderHost::CompleteBrowserInitialized(int process_id, nit: add previous line with: // PlzNavigate https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:62: static int GetNextBrowserProviderID(); You might want to also mention here that values start at -2 and go further negative to differentiate from renderer provided values. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:72: // case of redirect to HTTPS. Re-add the first line of this comment. https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... content/common/navigation_params.h:136: bool skip_service_worker; If we REALLY want to avoid adding more data fields here we could try to eliminate skip_service_worker by overloading request_context_type with its meaning: if it is set to REQUEST_CONTEXT_TYPE_UNSPECIFIED we could assume skip_service_worker is true (if that value is not normally used). To make this a little less cryptic we should also rename request_context_type to something like request_context_type_for_service_worker so that the connection is made clearer. But I'm not sure this is possible nor if it will be too confusing.
https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... content/common/navigation_params.h:136: bool skip_service_worker; On 2015/08/27 14:26:43, carlosk wrote: > If we REALLY want to avoid adding more data fields here we could try to > eliminate skip_service_worker by overloading request_context_type with its > meaning: if it is set to REQUEST_CONTEXT_TYPE_UNSPECIFIED we could assume > skip_service_worker is true (if that value is not normally used). > > To make this a little less cryptic we should also rename request_context_type to > something like request_context_type_for_service_worker so that the connection is > made clearer. > > But I'm not sure this is possible nor if it will be too confusing. Request context type is an enum part of the public content API. We should nto be modifying it. On top of that, I think it may be codified by various specs.
On 2015/08/27 15:19:03, clamy wrote: > https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... > File content/common/navigation_params.h (right): > > https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... > content/common/navigation_params.h:136: bool skip_service_worker; > On 2015/08/27 14:26:43, carlosk wrote: > > If we REALLY want to avoid adding more data fields here we could try to > > eliminate skip_service_worker by overloading request_context_type with its > > meaning: if it is set to REQUEST_CONTEXT_TYPE_UNSPECIFIED we could assume > > skip_service_worker is true (if that value is not normally used). > > > > To make this a little less cryptic we should also rename request_context_type > to > > something like request_context_type_for_service_worker so that the connection > is > > made clearer. > > > > But I'm not sure this is possible nor if it will be too confusing. > > Request context type is an enum part of the public content API. We should nto be > modifying it. On top of that, I think it may be codified by various specs. Notice this doesn't modify it as REQUEST_CONTEXT_TYPE_UNSPECIFIED is an existing value. But indeed this is only doable if that value is not a possible value for our requests.
Still, I don't like it: - it seems to me that it is cleverness over clarity, which we should avoid. - it is a point of failure should the REQUEST_CONTEXT_TYPE_UNSPECIFIED be used in the future. My comment about parameters in IPC is not that we should not add any parameters. It is that we should not add any parameter that either (A) has a single possible value for navigations or (B) which value is already known on the browser side. If it's not the case (such as with skip_service_worker) I'm fine with adding it.
On 2015/08/28 10:05:49, clamy wrote: > Still, I don't like it: > - it seems to me that it is cleverness over clarity, which we should avoid. > - it is a point of failure should the REQUEST_CONTEXT_TYPE_UNSPECIFIED be used > in the future. > > My comment about parameters in IPC is not that we should not add any parameters. > It is that we should not add any parameter that either (A) has a single possible > value for navigations or (B) which value is already known on the browser side. > If it's not the case (such as with skip_service_worker) I'm fine with adding it. Acknowledged and agreed.
Thanks all! I removed Nasko's DCHECK in render_frame_impl between patches 4 and 5 since the tryjobs did not like it. But maybe I was checking for the wrong thing? More answers below. https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_LOCATION, On 2015/08/27 14:26:42, carlosk wrote: > On 2015/08/27 11:53:45, clamy wrote: > > I'm not sure this is the best context type for a browser-initiated navigation. > I > > think we've been using LINK for this purpose. > > If you do this change here remember that there are other places where this same > value was used as default that should be changed as well. I left LOCATION for now, I'd like some more people input since we seemingly use both types. Maybe we should have a static method somewhere to return a default for browser-initiated navigations? https://codereview.chromium.org/1294243004/diff/60001/content/browser/loader/... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/loader/... content/browser/loader/navigation_url_loader_impl_core.h:60: void SetServiceWorkerProviderHost(scoped_ptr<ServiceWorkerProviderHost>); On 2015/08/27 14:26:42, carlosk wrote: > nit: new line here. Spacing between getters and setters in headers is so inconsistent across the code base, I am not sure what is the expected way to do it. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:107: } On 2015/08/27 14:26:43, carlosk wrote: > If-blocks existing solely for CHECK-ing something are generally frowned upon. I > recommend changing this to: > > CHECK_IMPLIES(render_process_id == kVirtualProcessIDForBrowserRequest, > base::CommandLine::ForCurrentProcess()->HasSwitch( > switches::kEnableBrowserSideNavigation)); Done. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.cc:564: void ServiceWorkerProviderHost::CompleteBrowserInitialized(int process_id, On 2015/08/27 14:26:43, carlosk wrote: > nit: add previous line with: // PlzNavigate Done. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_provider_host.h:62: static int GetNextBrowserProviderID(); On 2015/08/27 14:26:43, carlosk wrote: > You might want to also mention here that values start at -2 and go further > negative to differentiate from renderer provided values. Does that really matter? It looks like an implementation detail rather than something the caller should know about. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.cc:72: // case of redirect to HTTPS. On 2015/08/27 14:26:43, carlosk wrote: > Re-add the first line of this comment. Bad auto-merge :( Done. https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... File content/browser/service_worker/service_worker_request_handler.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/browser/service... content/browser/service_worker/service_worker_request_handler.h:65: NavigationURLLoaderImplCore* loader); On 2015/08/27 11:53:45, clamy wrote: > I'm a bit concerned about providing a raw pointer to the > NavigationURLLoaderImplCore, since the URLRequest will outlive it (it gets > destroyed when the request is ready to commit). I think transitioning to a weak > pointer would avoid possible edge cases here. The loader is only used in this fashion in this method: loader->SetServiceWorkerProviderHost(scoped_provider_host.Pass()); I don't think there is a risk of "losing" the pointer in the meantime. Or I am misunderstanding your point. https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... content/common/navigation_params.h:136: bool skip_service_worker; On 2015/08/27 14:26:43, carlosk wrote: > If we REALLY want to avoid adding more data fields here we could try to > eliminate skip_service_worker by overloading request_context_type with its > meaning: if it is set to REQUEST_CONTEXT_TYPE_UNSPECIFIED we could assume > skip_service_worker is true (if that value is not normally used). > > To make this a little less cryptic we should also rename request_context_type to > something like request_context_type_for_service_worker so that the connection is > made clearer. > > But I'm not sure this is possible nor if it will be too confusing. That's not possible. ServiceWorker has its own, sometimes complex, logic when it comes to setting skip_service_worker to true. https://codereview.chromium.org/1294243004/diff/60001/content/common/navigati... content/common/navigation_params.h:139: // load, or a sub objects load. On 2015/08/27 11:53:45, clamy wrote: > Is this really needed? You already know if you are loading a main frame or a > subframe on the browser side (and you _should not_ trust the information from > the renderer if you have it on the browser side). It seems weird that a sub > object load results in a navigation. Does it happen in practice? If it does, > this seems to me to be a bug taht we should fix. I removed the frame type. https://codereview.chromium.org/1294243004/diff/60001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/60001/content/renderer/render... content/renderer/render_frame_impl.cc:4873: FetchRedirectMode::FOLLOW_MODE); On 2015/08/27 11:53:45, clamy wrote: > Can we also precise the different possibilities for the request context type? As > mentioned in navigation_params.h, I think we may be able to get away from > sending the frame type. As we discussed offline, I don't think we can DCHECK on a list of what is acceptable for a request context type here.
Thanks! A few more comments on my part and it should be good for me. https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:14: #include "content/browser/service_worker/service_worker_provider_host.h" nit: remove unnecessary include. https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_LOCATION), Same comment on the request context type. https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:57: ServiceWorkerProviderHost* service_worker_provider_host() { I think this method is const. https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_unittest.cc:183: REQUEST_CONTEXT_TYPE_LOCATION); nit: I would again initialize the context type to link here (CommonNavigationParams default value for the transition type is LINK as well). https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2008: GetChromeBlobStorageContextForResourceContext(resource_context)); nit: extra line needed. https://codereview.chromium.org/1294243004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:90: DCHECK(ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id)); nit: add a DCHECK(loader)? https://codereview.chromium.org/1294243004/diff/100001/content/child/web_url_... File content/child/web_url_loader_impl.cc (left): https://codereview.chromium.org/1294243004/diff/100001/content/child/web_url_... content/child/web_url_loader_impl.cc:183: FetchRequestMode GetFetchRequestMode(const WebURLRequest& request) { Since we are moving the conversion out of this file, should we also move the asserts so that they remain together? https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... content/common/navigation_params.h:14: #include "content/common/service_worker/service_worker_types.h" nit; remove unnecessary include. https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... content/common/navigation_params.h:17: #include "content/public/common/request_context_frame_type.h" nit: remove unnecessary include. https://codereview.chromium.org/1294243004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4869: RequestContextFrameType frame_type = I think you should avoid initializing a parameter that is just used for DCHECKS. In fact, we could also DCHECK that the context we get is also what we expect, ie: DCHECK_IMPLIES(!frame_->parent(), GetRequestContextFrameTypeForWebURLRequest(*request) == REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL); (and the same for subframes). https://codereview.chromium.org/1294243004/diff/100001/content/test/test_rend... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/test/test_rend... content/test/test_render_frame_host.cc:333: REQUEST_CONTEXT_TYPE_LOCATION); Regarding the type, 3 lines below we set the transition type to PAGE_TRANSITION_LINK. I think it would be more coherent to have the type as REQUEST_CONTEXT_TYPE_HYPERLINK. (LOCATION would mean that it is a client-side redirect which is treated differently in the PageTransitions).
Thanks! Responses in-line. https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:14: #include "content/browser/service_worker/service_worker_provider_host.h" On 2015/08/31 12:09:26, clamy wrote: > nit: remove unnecessary include. It's needed for ServiceWorkerProviderHost::GetNextBrowserProviderID() https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_LOCATION), On 2015/08/31 12:09:27, clamy wrote: > Same comment on the request context type. Done. https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:57: ServiceWorkerProviderHost* service_worker_provider_host() { On 2015/08/31 12:09:27, clamy wrote: > I think this method is const. Done. https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/navigation_url_loader_unittest.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/navigation_url_loader_unittest.cc:183: REQUEST_CONTEXT_TYPE_LOCATION); Done. https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2008: GetChromeBlobStorageContextForResourceContext(resource_context)); On 2015/08/31 12:09:27, clamy wrote: > nit: extra line needed. You mean a new line? Done. https://codereview.chromium.org/1294243004/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:90: DCHECK(ServiceWorkerUtils::IsBrowserAssignedProviderId(provider_id)); On 2015/08/31 12:09:27, clamy wrote: > nit: add a DCHECK(loader)? Good idea. Done. https://codereview.chromium.org/1294243004/diff/100001/content/child/web_url_... File content/child/web_url_loader_impl.cc (left): https://codereview.chromium.org/1294243004/diff/100001/content/child/web_url_... content/child/web_url_loader_impl.cc:183: FetchRequestMode GetFetchRequestMode(const WebURLRequest& request) { On 2015/08/31 12:09:27, clamy wrote: > Since we are moving the conversion out of this file, should we also move the > asserts so that they remain together? That sounds reasonable to me and I think they belong in WebUrlRequestUtil better anyway. I moved them, nasko@, WDYT? https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... content/common/navigation_params.h:14: #include "content/common/service_worker/service_worker_types.h" On 2015/08/31 12:09:27, clamy wrote: > nit; remove unnecessary include. Done. https://codereview.chromium.org/1294243004/diff/100001/content/common/navigat... content/common/navigation_params.h:17: #include "content/public/common/request_context_frame_type.h" On 2015/08/31 12:09:27, clamy wrote: > nit: remove unnecessary include. Done. https://codereview.chromium.org/1294243004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:4869: RequestContextFrameType frame_type = On 2015/08/31 12:09:27, clamy wrote: > I think you should avoid initializing a parameter that is just used for DCHECKS. > In fact, we could also DCHECK that the context we get is also what we expect, > ie: > DCHECK_IMPLIES(!frame_->parent(), > GetRequestContextFrameTypeForWebURLRequest(*request) == > REQUEST_CONTEXT_FRAME_TYPE_TOP_LEVEL); > (and the same for subframes). Done. https://codereview.chromium.org/1294243004/diff/100001/content/test/test_rend... File content/test/test_render_frame_host.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/test/test_rend... content/test/test_render_frame_host.cc:333: REQUEST_CONTEXT_TYPE_LOCATION); On 2015/08/31 12:09:27, clamy wrote: > Regarding the type, 3 lines below we set the transition type to > PAGE_TRANSITION_LINK. I think it would be more coherent to have the type as > REQUEST_CONTEXT_TYPE_HYPERLINK. (LOCATION would mean that it is a client-side > redirect which is treated differently in the PageTransitions). Done.
Thanks! Lgtm provided service worker people are happy. https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:14: #include "content/browser/service_worker/service_worker_provider_host.h" On 2015/08/31 14:00:35, Fabrice wrote: > On 2015/08/31 12:09:26, clamy wrote: > > nit: remove unnecessary include. > > It's needed for ServiceWorkerProviderHost::GetNextBrowserProviderID() Acknowledged. https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:75: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; nit: maybe declare service worker member together?
Mainly one question based on the non-serviceworker code. I'd like someone from the SW team to approve this CL before I put my stamp on it. https://codereview.chromium.org/1294243004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_HYPERLINK), Is hyperlink the most appropriate here? Browser initiated navigation is likely an omnibox action or something similar, which isn't a hyperlink. What is the TYPE_LOCATION corresponding to? https://codereview.chromium.org/1294243004/diff/120001/content/common/service... File content/common/service_worker/service_worker_utils.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/service... content/common/service_worker/service_worker_utils.h:44: // Returns true of the _provider_id_ was assigned by the browser process. nit: |provider_id| https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4874: REQUEST_CONTEXT_FRAME_TYPE_NESTED); Thanks for moving those out of the struct!
This is looking pretty good but I think I see a possible leak, see inline comments for details. https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/20001/content/browser/service... content/browser/service_worker/service_worker_dispatcher_host.cc:744: GetContext()->TakeBrowserProviderHost(provider_id)); If anything should prevent navigation from proceeding in the child process, the untaken host is leaked. This object probably should be owned by objects specific to this navigation/request until OnProviderCreated() time when the SW system can take over ownership. https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/20001/content/common/navigati... content/common/navigation_params.h:53: int service_worker_provider_id); browser_initiated_nav_id https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:94: service_worker_context_->RegisterBrowserProviderHost( If anything should prevent context->TakeBrowserProviderHost() from eventually being called, the provider host will be leaked. The comment below about timing related to UI thread task processing suggests that some things could prevent the Take method from happening? https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:734: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( Should this be treated as a bad_message instead of a CHECK? A buggy or compromised renderer could provide garbage for this id and I don't think the browser should crash as a result. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:741: } That CHECK(CommandLine) might make more sense on this line, but it's at the top of CompleteBrowserIntialized() so maybe its not needed in here at all? https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:95: loader->SetServiceWorkerProviderHost(scoped_provider_host.Pass()); Assuming the same URLRequest instance is used from start to end of a navigation (which seems to be the case?), here's a suggestion that might help to avoid the leak that can occur in the Register/Take scheme currently in use in the CL. Instead of passing ownership of the newly creatd |host| the |loader| object (which later passes ownership to the sw_context_core where it can be leaked), pass ownership to the |handler| instance created below. The lifetime of that object is tied to the lifetime of the |request| so if anything should prevent the taking over of ownership of the host, it won't get leaked. Notice the scoped_ptr<ServiceWorkerProviderHost> host_for_cross_site_transfer_ data member. It exist to avoid similar leakage of host objects for that other case. This data member could be renamed and used in this case too. ServiceWorkerRequestHandler::GetHandler(request) can be used to easily get at the handler instance and a Take method could be added to take ownership of any host it's holding on to. https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:61: provider_id_ = browser_provider_id; Is it possible for provider_type to be SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME in this case? https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:83: kInvalidServiceWorkerProviderId) {} maybe invoke GenerateProviderIdForType(provider_type) here to reduce the branching logic needed in the real ctor https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... content/common/navigation_params.h:51: int service_worker_provider_id); I still think it could be conceptually cleaner and useful for this value to be coined as a unique "navigation_id" rather than a sw specific value. That way the sw and appcache systems can each derive a unique <foo>HostIds based on this value. As a practical terms, its hardly a difference, mostly just a matter what body of code owns the static sequence generator. https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2578: service_worker_provider_id = extra_data->service_worker_provider_id(); does this only pick up browser initiated ids for PlzNavigate?
I think transitioning to using a navigation id as michaeln@ suggested would be nice. https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... content/common/navigation_params.h:51: int service_worker_provider_id); On 2015/09/01 01:15:23, michaeln wrote: > I still think it could be conceptually cleaner and useful for this value to be > coined as a unique "navigation_id" rather than a sw specific value. That way the > sw and appcache systems can each derive a unique <foo>HostIds based on this > value. As a practical terms, its hardly a difference, mostly just a matter what > body of code owns the static sequence generator. I also like this idea as a unique navigation id will be useful in other places as well. Now that we have the NavigationHandle in place this should be a simple addition. I don't expect this to make this change more complex than it already is and avoiding conflicts of this id value with the renderer generated provider ids should be simple enough.
Apologies for the delay, moving the dummy SWPH proved to be a bit more complicated than I first thought. https://codereview.chromium.org/1294243004/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:97: REQUEST_CONTEXT_TYPE_HYPERLINK), On 2015/08/31 23:58:17, nasko (slow to review) wrote: > Is hyperlink the most appropriate here? Browser initiated navigation is likely > an omnibox action or something similar, which isn't a hyperlink. What is the > TYPE_LOCATION corresponding to? I had it set to LOCATION but clamy@ insisted it should be HYPERLINK. I will let you two fight over it. https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:94: service_worker_context_->RegisterBrowserProviderHost( On 2015/09/01 01:15:23, michaeln wrote: > If anything should prevent context->TakeBrowserProviderHost() from eventually > being called, the provider host will be leaked. The comment below about timing > related to UI thread task processing suggests that some things could prevent the > Take method from happening? Acknowledged. https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:75: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2015/08/31 16:58:48, clamy wrote: > nit: maybe declare service worker member together? Done. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:734: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/09/01 01:15:23, michaeln wrote: > Should this be treated as a bad_message instead of a CHECK? A buggy or > compromised renderer could provide garbage for this id and I don't think the > browser should crash as a result. That's a good point. How about changing it to a DCHECK here instead? We'd still have a way to catch it on tests. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:741: } On 2015/09/01 01:15:23, michaeln wrote: > That CHECK(CommandLine) might make more sense on this line, but it's at the top > of CompleteBrowserIntialized() so maybe its not needed in here at all? Acknowledged. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:95: loader->SetServiceWorkerProviderHost(scoped_provider_host.Pass()); On 2015/09/01 01:15:23, michaeln wrote: > Assuming the same URLRequest instance is used from start to end of a navigation > (which seems to be the case?), here's a suggestion that might help to avoid the > leak that can occur in the Register/Take scheme currently in use in the CL. > > Instead of passing ownership of the newly creatd |host| the |loader| object > (which later passes ownership to the sw_context_core where it can be leaked), > pass ownership to the |handler| instance created below. The lifetime of that > object is tied to the lifetime of the |request| so if anything should prevent > the taking over of ownership of the host, it won't get leaked. > > Notice the scoped_ptr<ServiceWorkerProviderHost> host_for_cross_site_transfer_ > data member. It exist to avoid similar leakage of host objects for that other > case. This data member could be renamed and used in this case too. > ServiceWorkerRequestHandler::GetHandler(request) can be used to easily get at > the handler instance and a Take method could be added to take ownership of any > host it's holding on to. > I added the SWPH to the |handler| in SWPH::CreateRequestHandler. I had to leave a reference to the |handler| in the |loader| for easy access but it is a weak reference to it so there should not be any ownership issue. WDYT? https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:61: provider_id_ = browser_provider_id; On 2015/09/01 01:15:23, michaeln wrote: > Is it possible for provider_type to be > SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME in this case? DCHECK'd https://codereview.chromium.org/1294243004/diff/120001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:83: kInvalidServiceWorkerProviderId) {} On 2015/09/01 01:15:23, michaeln wrote: > maybe invoke GenerateProviderIdForType(provider_type) here to reduce the > branching logic needed in the real ctor We still have some sanity DCHECKs, I don't think it would change much of the real ctor logic. https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... content/common/navigation_params.h:51: int service_worker_provider_id); On 2015/09/01 01:15:23, michaeln wrote: > I still think it could be conceptually cleaner and useful for this value to be > coined as a unique "navigation_id" rather than a sw specific value. That way the > sw and appcache systems can each derive a unique <foo>HostIds based on this > value. As a practical terms, its hardly a difference, mostly just a matter what > body of code owns the static sequence generator. This struct is used both ways, this argument has a slightly different meaning depending on where the navigation was initiated. For a browser-initiated navigation, this refers to the value with which the dummy SWPH was initialized. For a renderer-initiated navigation, this refers to the SWPH that is supposed to handle the request. So, unless we create a new SWNP/SWPH couple for every navigation, that is not going to work. Even that approach has issues, since navigation_id would need to be created browser-side and we would need a mechanism to have a temporary id-less SWNP for renderer-initiated navigations. However, this still sounds like a good idea and it would work really well with PlzNavigate. But I think it would lead to a lot of code complexity to implement a dual ID system as it is (one for current navigation, one for PlzNavigate). Maybe we could revisit once we have fully switched to PlzNavigate? https://codereview.chromium.org/1294243004/diff/120001/content/common/service... File content/common/service_worker/service_worker_utils.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/service... content/common/service_worker/service_worker_utils.h:44: // Returns true of the _provider_id_ was assigned by the browser process. On 2015/08/31 23:58:17, nasko wrote: > nit: |provider_id| Done. Fixed the typo too (s/of/if/) https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:2578: service_worker_provider_id = extra_data->service_worker_provider_id(); On 2015/09/01 01:15:23, michaeln wrote: > does this only pick up browser initiated ids for PlzNavigate? The value is initialized to -1, so it doesn't really matter since that is going to cause the "classic" logic to be executed in the ctor. https://codereview.chromium.org/1294243004/diff/120001/content/renderer/rende... content/renderer/render_frame_impl.cc:4874: REQUEST_CONTEXT_FRAME_TYPE_NESTED); On 2015/08/31 23:58:17, nasko (slow to review) wrote: > Thanks for moving those out of the struct! Welcome!
Quick update, since this patch is starting to get kinda large, I am breaking it into smaller CLs. I have a separate patch for WebURL: https://codereview.chromium.org/1317833005/ And a second patch for adding parameters on top of it: https://codereview.chromium.org/1315023004/
On 2015/09/07 17:02:48, Fabrice wrote: > Quick update, since this patch is starting to get kinda large, I am breaking it > into smaller CLs. > I have a separate patch for WebURL: https://codereview.chromium.org/1317833005/ > And a second patch for adding parameters on top of it: > https://codereview.chromium.org/1315023004/ The latter CL doesn't have any reviewers. Is it supposed to be looked at? Also, there are some unresolved questions in this CL. michaeln, could you take another look at the updated version of this CL?
https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:734: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/09/07 12:09:48, Fabrice wrote: > On 2015/09/01 01:15:23, michaeln wrote: > > Should this be treated as a bad_message instead of a CHECK? A buggy or > > compromised renderer could provide garbage for this id and I don't think the > > browser should crash as a result. > > That's a good point. How about changing it to a DCHECK here instead? We'd still > have a way to catch it on tests. If we know that this can be hit from compromised renderer, then using CHECK is the wrong thing to do. It is indeed a great fit for bad_message.
https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:734: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/09/14 21:28:32, nasko (slow to review) wrote: > On 2015/09/07 12:09:48, Fabrice wrote: > > On 2015/09/01 01:15:23, michaeln wrote: > > > Should this be treated as a bad_message instead of a CHECK? A buggy or > > > compromised renderer could provide garbage for this id and I don't think the > > > browser should crash as a result. > > > > That's a good point. How about changing it to a DCHECK here instead? We'd > still > > have a way to catch it on tests. > > If we know that this can be hit from compromised renderer, then using CHECK is > the wrong thing to do. It is indeed a great fit for bad_message. Note that historically, we have put CHECKs in PlzNavigate specific IPC handlers in RFH. I think those can be triggered by a compromised renderer as well.
On 2015/09/14 21:35:00, clamy wrote: > https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... > File content/browser/service_worker/service_worker_dispatcher_host.cc (right): > > https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... > content/browser/service_worker/service_worker_dispatcher_host.cc:734: > CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( > On 2015/09/14 21:28:32, nasko (slow to review) wrote: > > On 2015/09/07 12:09:48, Fabrice wrote: > > > On 2015/09/01 01:15:23, michaeln wrote: > > > > Should this be treated as a bad_message instead of a CHECK? A buggy or > > > > compromised renderer could provide garbage for this id and I don't think > the > > > > browser should crash as a result. > > > > > > That's a good point. How about changing it to a DCHECK here instead? We'd > > still > > > have a way to catch it on tests. > > > > If we know that this can be hit from compromised renderer, then using CHECK is > > the wrong thing to do. It is indeed a great fit for bad_message. > > Note that historically, we have put CHECKs in PlzNavigate specific IPC handlers > in RFH. I think those can be triggered by a compromised renderer as well. It is my fault then for not pointing those out. CHECK - ok for cases where no compromised renderer can force us to reach. bad_message for code reachable from compromised renderer and DCHECK if deemed needed.
Thanks! No new patch yet, just an answer to the comment about the command line check. I will rebase on top of https://codereview.chromium.org/1315023004/ after it has landed. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_dispatcher_host.cc:734: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/09/14 21:28:32, nasko (slow to review) wrote: > On 2015/09/07 12:09:48, Fabrice wrote: > > On 2015/09/01 01:15:23, michaeln wrote: > > > Should this be treated as a bad_message instead of a CHECK? A buggy or > > > compromised renderer could provide garbage for this id and I don't think the > > > browser should crash as a result. > > > > That's a good point. How about changing it to a DCHECK here instead? We'd > still > > have a way to catch it on tests. > > If we know that this can be hit from compromised renderer, then using CHECK is > the wrong thing to do. It is indeed a great fit for bad_message. I do not know if having a test at all is relevant then. Essentially, we would be testing that if the provider_id received is negative, then PlzNavigate must be enabled. This would only work during the transition period. Either way, the second test on line 737 already takes care of that, indirectly. I will remove this CHECK in a future patch.
I think i need to patch it in and enable the flag to better see what's going on? I don't understand the purpose of the changes to MakeCommonNavigationParams(). https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:169: void RegisterBrowserProviderHost( As mentioned in other comments, having this class maintain a collection of rawptrs to provider hosts in play for plz navigations would probably help (OnProviderCreated find the darn thing given its id). ServiceWorkerProviderHost* GetNavigationProviderHost() AddNavigationProviderHost(ServiceWorkerProviderHost*) RemoveNavigationProviderHost(ServiceWorkerProviderHost*) note: This central class maintains rawptrs to other 'live' object types too for similar reasons, to make them easily reachable given just an id. See the GetLive<<>> and AddLive<<>> methods below. https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:295: BrowserProviderHostMap browser_provider_host_map_; i'd vote to use a std::map<int, T*> for consistency https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/120001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:95: loader->SetServiceWorkerProviderHost(scoped_provider_host.Pass()); On 2015/09/07 12:09:48, Fabrice wrote: > On 2015/09/01 01:15:23, michaeln wrote: > > Assuming the same URLRequest instance is used from start to end of a > navigation > > (which seems to be the case?), here's a suggestion that might help to avoid > the > > leak that can occur in the Register/Take scheme currently in use in the CL. > > > > Instead of passing ownership of the newly creatd |host| the |loader| object > > (which later passes ownership to the sw_context_core where it can be leaked), > > pass ownership to the |handler| instance created below. The lifetime of that > > object is tied to the lifetime of the |request| so if anything should prevent > > the taking over of ownership of the host, it won't get leaked. > > > > Notice the scoped_ptr<ServiceWorkerProviderHost> host_for_cross_site_transfer_ > > data member. It exist to avoid similar leakage of host objects for that other > > case. This data member could be renamed and used in this case too. > > ServiceWorkerRequestHandler::GetHandler(request) can be used to easily get at > > the handler instance and a Take method could be added to take ownership of any > > host it's holding on to. > > > > I added the SWPH to the |handler| in SWPH::CreateRequestHandler. > I had to leave a reference to the |handler| in the |loader| for easy access but > it is a weak reference to it so there should not be any ownership issue. WDYT? I think once initalization is done, the swsystem should be able to manage ownership xfer internally and the loader won't need the ptr at all. As coded, ownership is being xferred twice (from handler to core/intermediary to core/final) How's this for a strategy instead? * the |handler| takes ownership of the pre-created host during initialziation * the |core| has a map of rawptrs to all pre-created hosts (so OnProviderCreated can find them). * owenership is xferred to the |core| inside of OnProviderCreated (that involves setting the process_id_ to a real value and removing it from the map of pre-created hosts). * the |handler| conditionally deletes the |host| iff it wasn't picked up by OnProviderCreated and also removes it from the map of pre-created hosts. https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/120001/content/common/navigat... content/common/navigation_params.h:51: int service_worker_provider_id); On 2015/09/07 12:09:48, Fabrice wrote: > On 2015/09/01 01:15:23, michaeln wrote: > > I still think it could be conceptually cleaner and useful for this value to be > > coined as a unique "navigation_id" rather than a sw specific value. That way > the > > sw and appcache systems can each derive a unique <foo>HostIds based on this > > value. As a practical terms, its hardly a difference, mostly just a matter > what > > body of code owns the static sequence generator. > > This struct is used both ways, this argument has a slightly different meaning > depending on where the navigation was initiated. > For a browser-initiated navigation, this refers to the value with which the > dummy SWPH was initialized. I see the browser-initiated stuff at the callsite to SWPH::GetNextXXXX(). > For a renderer-initiated navigation, this refers to the SWPH that is supposed to > handle the request. Why does the browser-side nav logic have to know of the id in this case? I see the provider_id being set in MakeCommonNavigationParams in the renderer, but don't follow what happens thereafter and why that value is needed? > However, this still sounds like a good idea and it would work really well with > PlzNavigate. But I think it would lead to a lot of code complexity to implement > a dual ID system as it is (one for current navigation, one for PlzNavigate). > Maybe we could revisit once we have fully switched to PlzNavigate? > (one for current navigation, one for PlzNavigate) Would it make sense to coin a "browser_initiated_nav_id" only for browser initiated navs? https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:99: service_worker_context_->RegisterBrowserProviderHost( This seems out of place here? All it's doing is xfering ownership from the URLRequest+SWReqHandler to the ContextCore? See other comments about not needed to do that at all. https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:75: ServiceWorkerRequestHandler* service_worker_handler_; Is this rawptr needed? It can be looked up as needed given the URLRequest ptr. A NavigationResourceHandler helper method might make sense to connect the dots. ServiceWorkerRequestHandle* NavigationResourceHandler::GetServiceWorkerRequestHandler() { ServiceWorkerRequestHandle::GetHandler(request()); } I think that'd help with code grokability compared to a rawptr. edit: Hmmm... past init time, the loader code may not need to get back to the handler. See other comments about how this probably isn't needed. https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2075: ServiceWorkerRequestHandler::InitializeHandler( I think it'd be worth creating a new Init method for this case in particular instead of swiss-army-knifing the existing method. Considerably more needs to happen now in addition to creating a handler and associating it with the request object. ServiceWorkerRequestHandler::InitializeForNavigationRequest - pre-create a providerhost instance with the no-process-id - stash a ptr to it in the |core|->AddNavigationProviderHost(p) - create and init the handler (akin to what InitializeHandler does today) - assign ownership of the newly created providerhost to the handler https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:232: ServiceWorkerProviderHost* ServiceWorkerContextCore::CreateBrowserProviderHost( Probably this helper method isn't really needed, i think the function will only be called from within the bowels of sw code (the Handler::InitForNavigationRequest() method). https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:170: scoped_ptr<ServiceWorkerProviderHost> TakeBrowserProviderHost( Since the URLRequest + SWRequestHandler scope the lifetime of the 'host' until ownership can be properly attributed to the target renderer, this central ServiceWorkerContextCore class doesn't need to. I think i see why you have it this way, so SWDispatchHost::OnProviderCreated() has a way to find the pre-created ServiceWorkerProviderHost instance. See comments about having a map of rawptrs in the |core|. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:70: static base::StaticAtomicSequenceNumber sequence; Use of a thread safe construct here can be misleading. There's no need for thread-safetey here, maybe use a simple static int here in place of the StaticAtomicSequenceNumber. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:376: handler->SetBrowserProviderHost( This is an odd hidden artifact? See comments about makeing a distinct ServiceWorkerRequestHandler::InitializeForNavigationRequest() method and hoisting this xfer of ownership into that method. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:71: NavigationURLLoaderImplCore* loader) { it'd be nice to not have this dependency https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:87: // Initialize a dummy SWProviderHost for browser-initiated navigations. "Dummy" might be misleading. It's a real 'host'. It gets constructed when a navigation is initiated. That happens differently for brower vs renderer intiated navs. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:124: loader->SetServiceWorkerHandler(handler.get()); This method has quirky artifacts now depending on how its called. I'd like to make the differences in behavior more obvious. The differences in behavior are pretty significant... * sometimes this method creates a SWProviderHost vs sometimes not * sometimes this method assigns ownership of the host to the 'handler' vs sometimes not Making these hidden behaviors more explicit could help down the road when its time to delete code for the non plznavigate way of doing navigations and also help readers understand what's going on and why. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:152: switches::kEnableBrowserSideNavigation)); just checking, but are these pre-existing methods only reachable with the plznavigate flag is set? https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.h:102: // navigations. These public methods may not be needed if the xfer can be handled as handwaved at? https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.h:125: scoped_ptr<ServiceWorkerProviderHost> temporary_host_; its good to annotate with // PlzNavigate, picking a name that suggest this has something todo with that would be even better, maybe navigation_host_ ? https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:521: extra_data->service_worker_provider_id()); If i understand correctly, i think this is called in preperation of asking the browser to do the navigation. Is that right? * How does the value provided here get used downstream in the browser process? * Can the browser decide to offload the nav to a different process? https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2583: if (original_request.extraData()) { I think this new bit of code is specific to PlzNavigate, if thats right please annotate it with the // PlzNavigate comment too. I think the extraData() only applies to PlzNavigate initiated navigations, is that right? I really appreciate the use of the PlzNavigate comment to annotate where the two different modes of doing navigations differ. Otherwise we have mysteries like "why is extra_data sometimes null and other times not and does it matter" that nearly nobody knows the answer to.
I've patched it in to take a closer took. In my local build, so far I've removed the DCHECK mentioned below. The comments below are based on watching what happens prior to fixing the ipc traits. I'll come back with other observations about what happens with the ipc traits fixed. I'm only looking at the case when navigation is supposed to resume in the originating process for now. https://codereview.chromium.org/1294243004/diff/160001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/common/navigat... content/common/navigation_params.h:97: int service_worker_provider_id; This needs to be added to the IPC traits for this class in frame_messages.h too. https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:521: extra_data->service_worker_provider_id()); Ok, this id is definitely not useful. The object it identifies will be deleted prior to startLoad() returning and any other msg being processed by the loop. According to nate this is by desigin. The original policy/provisional loader+swprovider is deleted (per the snippet below) and a new loader+swprovider gets created if/when the browser process decides to delegate the navigation back to the this child process. snipped from FrameLoader::startLoad(...) // stopAllLoaders can detach the LocalFrame, so protect it. RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get()); if ((!m_policyDocumentLoader->shouldContinueForNavigationPolicy(...) { detachDocumentLoader(m_policyDocumentLoader); return; } The MakeCommonNavigationParams() function is called inside of m_policyDocumentLoader->shouldContinueForNavigationPolicy. The documentLoader and its associated ServiceWorkerNetworkProvider are deleted inside of detachDocumentLoader(). Eventually we get an ipc to RenderFrameImpl::OnCommitNavigation(). As currently coded in the cl, the value of CommonNavigationParams.service_worker_provider_id in that message is kInvalidServiceWorkerProviderId. Yet the stream override url has an actual blob:url in it. I'll look more closely at the corresponding control flow in the browser process? https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:4860: FetchRedirectMode::FOLLOW_MODE); I've patched it in and am seeing these DCHECKs get hit on every navigation? Is there something more I need to do in addition to running with --enable-browser-side-navigation to test things?
This cl is tough because it requires a solid working knowledge of multiple areas of difficult to master expertise. It might make sense for someone more sw-savvy (me) to carry the ball a little bit further on this cl and then to hand it off back to someone more plznavigate-savvy (and then hand it off to somebody more blink/loader-nav savvy)? https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:583: route_id_ = frame_id; Oh, we also need to set dispatcher_host_ in here. This method probably needs to perform all the functions of CompleteCrossSiteTransfer. These two methods should probably be merged. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:124: loader->SetServiceWorkerHandler(handler.get()); bah, handler.get() will always be null here given the handler.release() on the previous line
Or better yet, divide and conquer to get the details right. I'd be great if we could decompose this into smaller pieces of work where the different pieces are more localized to a single area of expertise. That calls for clean abstractions and interfaces that can be reasoned about at a high level and that line of thinking brings me back to crisply defining a navigation as a thing. Given that, I think the interaction between plznavigating and sworkering can be explained as... 1) In the browser process, the navigation stack initializes a swhandler + swproviderhost for a particular browser-initiated navigation inside of ResourceDispatcherHostImpl::BeginNavigationRequest. The new method we could use is ServiceWorkerRequestHandler::InitializeForNavigation(browser_initiated_navid, ...). The sw system can take it from there. 2) In the renderer process, the navigation stack creates a corresponding swprovider for that particular browser-initiated navigation in RenderFrameImpl::didCreateDataSource. The new method we could use there is a new ctor: ServiceWorkerNetworkProvider(browser_initiated_navid, ...). The sw system can take it from there. How the navstack deal with getting that navid to the right place at the right time is one ball of wax and what happens on the other side of those sw apis is another. Ikd the details of plznavigation wrt browser initiated vs renderer initiated and how they coexist or not, but its easy to coin identifiers that can distinguish between the two: navid = std::pair<pid, seqnum-in-process> // for the browser process pid = -1
https://codereview.chromium.org/1294243004/diff/180001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/180001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:56: ResourceDispatcherHostImpl::Get()->BeginNavigationRequest( Upon return from this function, the newly created swproviderhost should be retrievable via the sw constructs setup underneath this function call. Where is the commitnav message sent, the id value of the host needs to make it from here to there?
I have been working on a new design, but have encountered some issue. Keeping ownership of the SWProviderHost in the SWRequestHandler does not work because the URLRequest is destroyed too early. So by the time OnProviderCreated is called, the raw pointer in ServiceWorkerContextCore is no longer valid. I think we are going to need to keep ownership of the SWProviderHosts elsewhere. The URLRequest is detroyed from content::ResourceDispatcherHostImpl::RemovePendingLoader(), which is called when OnReadCompleted happens. This is way before the renderer actually creates its ServiceNetworkNetworkProvider, resulting in sending the ServiceWorkerHostMsg_ProviderCreated IPC to the browser.
On 2015/09/25 19:40:09, Fabrice wrote: > I have been working on a new design, but have encountered some issue. > > Keeping ownership of the SWProviderHost in the SWRequestHandler does not work > because the URLRequest is destroyed too early. So by the time OnProviderCreated > is called, the raw pointer in ServiceWorkerContextCore is no longer valid. I > think we are going to need to keep ownership of the SWProviderHosts elsewhere. > The URLRequest is detroyed from > content::ResourceDispatcherHostImpl::RemovePendingLoader(), which is called when > OnReadCompleted happens. This is way before the renderer actually creates its > ServiceNetworkNetworkProvider, resulting in sending the > ServiceWorkerHostMsg_ProviderCreated IPC to the browser. Ooops, i was under the mistaken impression that the URLRequest created in ResourceDispatcherHostImpl::BeginNavigationRequest survived the entire navigation, at least until the dest renderer picked up the ball. If thats not the case, we definitely will need to find another owner for the 'host'. Something that survives as least until the dest renderer constructs the doc. Is there any plznav object in the browser process that could provide a home? What are the lifeimtes of NavigationURLLoaderImpl & NavigationURLLoaderImplCore? How is ownership of the underlying Stream handled?
On 2015/09/25 21:02:23, michaeln wrote: > On 2015/09/25 19:40:09, Fabrice wrote: > > I have been working on a new design, but have encountered some issue. > > > > Keeping ownership of the SWProviderHost in the SWRequestHandler does not work > > because the URLRequest is destroyed too early. So by the time > OnProviderCreated > > is called, the raw pointer in ServiceWorkerContextCore is no longer valid. I > > think we are going to need to keep ownership of the SWProviderHosts elsewhere. > > The URLRequest is detroyed from > > content::ResourceDispatcherHostImpl::RemovePendingLoader(), which is called > when > > OnReadCompleted happens. This is way before the renderer actually creates its > > ServiceNetworkNetworkProvider, resulting in sending the > > ServiceWorkerHostMsg_ProviderCreated IPC to the browser. > > Ooops, i was under the mistaken impression that the URLRequest created in > ResourceDispatcherHostImpl::BeginNavigationRequest survived the entire > navigation, at least until the dest renderer picked up the ball. > > If thats not the case, we definitely will need to find another owner for > the 'host'. Something that survives as least until the dest renderer > constructs the doc. > > Is there any plznav object in the browser process that could provide > a home? What are the lifeimtes of NavigationURLLoaderImpl & > NavigationURLLoaderImplCore? > > How is ownership of the underlying Stream handled? The NavigationURLLoaderImpl* are all destroyed at commit time, so they are not usable. For the Stream, the RenderFrameHost keeps a StreamHandle, which prevents it from being destroyed. I don't think ServiceWorker has a similar UI thread object we could keep in the RenderFrameHost. I guess the Stream itself could take ownership of the SWProviderHost, but that would be a bit weird.
So the idea we had is that we should have a UI thread handle similar to the StreamHandle that we send to the UI thread and that is held there until the navigation has committed. For example, it can be held by the NavigationHandle. Additionally, the thread handle can have the ServiceWorkerProvider id, so that we properly update the RequestNavigationParams that we send to the renderer.
On 2015/09/25 22:04:11, clamy wrote: > So the idea we had is that we should have a UI thread handle similar to the > StreamHandle that we send to the UI thread and that is held there until the > navigation has committed. For example, it can be held by the NavigationHandle. Great, the NavigationHandle lifetime works. > Additionally, the thread handle can have the ServiceWorkerProvider id, so that > we properly update the RequestNavigationParams that we send to the renderer. what is "the thread handle", do you mean 'navhandle'?
No I meant the SWHandle, sorry for the confusion.
The new SW provided class is a ServiceWorkerNavigationHandle. The nav stack has
to
ensure its lifetime extends at least beyond commitTime in the renderer.
Here's an outline of a lifetime and ownership xfer strategy.
* NavigationHandle owns a ServiceWorkerNavigationHandle.
* ServiceWorkerNavigationHandle must be created on the IO thread and deleted on
the IO thread.
A ServiceWorkerProviderHost is pre-created in its constructor. The object is
conditionally
deleted in the destructor iff the pre-created host was not utilized to complete
a navigation.
* ServiceWorkerNavigationHandle has a threadsafe getter for the provider_id and
internally
keeps a ptr to the SWPH, there is no public getter for the provider_ptr.
* The core has a map of rawptrs to all pre-created hosts so InitForNavRequest
and OnProviderCreated can find them.
* ServiceWorkerRequestHandler::InitForNavRequest(sw_nav_handle, ...) so it can
access
the id and pre-created host during initialization.
* OnProviderCreated does a lot. Fully initialize the host for the right process,
xfers
ownership to the core and removes it from the cores pre-created map.
// Nav stack creates and owns.
class ServiceWorkerNavigationHandle {
public:
// The ctor and dtor are for use on the IO thread only.
ServiceWorkerNavigationHandle(core); { creates the host and gives a rawptr to
the core }
~ServiceWorkerNavigationHandle(); { if host is in the pre-create map, delete
it and clean up }
// May be used on any thread.
int provider_id();
private:
int provider_id_;
scoped_ptr<SWPH> pre_created_host_;
static int next_provider_id_; // negative ids
}
Additional ServiceWorkerRequestHandler methods {
// For use by the ResourceDispatcherHostImpl::BeginNavigationRequest
static InitializeForNavigationRequest(const ServiceWorkerNavigationHandle&,
...)
}
Additional ServiceWorkerContextCore methods {
// To access the map of rawptrs
ServiceWorkerProviderHost* GetNavigationProviderHost(provider_id)
AddNavigationProviderHost(ServiceWorkerProviderHost*);
RemoveNavigationProviderHost(ServiceWorkerProviderHost*);
}
Just a heads-up. I tried to design as proposed here, but encountered some threading issues. Mainly, the NavigationHandle is a UI class, and we'd have to create the SWNavigationHandle from the IO thread and attach it to the NavigationHandleImpl, which is bad. So, new design, as discussed off-line with clamy@. We will create the SWPH in the NavigationHandleImpl ctor and put it in the SWContextCore map. We'll keep a reference to the provider ID in the NavigationHandleImpl. In the NavigationHandleImpl dtor, we will post a task to the IO thread to take care of destroying the SWPH in the SWContextCore if needed.
That sounds pretty similar, except there is no ServiceWorkerNavigationHandle to encapsulate some of the details. Other than the ServiceWorkerContextWrapper, the other service worker classes should only be used on the IO thread. It's not safe to construct a ServiceWorkerProviderHost on the UI thread. If there is no convenient place to to IO thread init work related to NavHandles, we could make the ServiceWorkerNavigationHandle UI thread constructable/destructible and have it internally do the task posting to the IO thread for interacting with other SW classes. It would need to be constructed with a reference to the ServiceWorkerContextWrapper to safely reach the core on the IO thread.
Yeah if we need a safe 'handle' to work with SWProviderHost on UI thread adding a wrapper class like something michael proposes could encapsulate threading part and that'd probably make the core code simpler. On Wed, Sep 30, 2015 at 6:52 AM, <michaeln@chromium.org> wrote: > That sounds pretty similar, except there is no > ServiceWorkerNavigationHandle to > encapsulate some of the details. > > Other than the ServiceWorkerContextWrapper, the other service worker > classes > should only be used on the IO thread. It's not safe to construct a > ServiceWorkerProviderHost on the UI thread. > > If there is no convenient place to to IO thread init work related to > NavHandles, > we could make the ServiceWorkerNavigationHandle UI thread > constructable/destructible and have it internally do the task posting to > the IO > thread for interacting with other SW classes. It would need to be > constructed > with a reference to the ServiceWorkerContextWrapper to safely reach the > core on > the IO thread. > > https://codereview.chromium.org/1294243004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The idea was to generate the ServiceWorkerProviderHost ID on the UI thread and keep it in the NavigationhandleImpl. When the request is sent to the IO thread by the NavigationURlLoaderCore, we will create the ServiceWorkerProviderHost based on the id sent by the UI thread (but yes the construction takes place on the IO thread). It is then placed on a map. When a started request is stopped on the UI thread, a task is posted to the IO thread to destroy the SWPH if it still in the map. Since all browser-created ids will be created in the UI thread, this should be safe from a threading point of view.
Thanks everyone! I'm sorry this took so long, but this should look much better now. I did not add a new class in the end, the ContextCore keeps a map of pointers to the SWProviderHost. It is then removed if necessary in the NavigationHandleImpl dtor. PTAL. https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:99: service_worker_context_->RegisterBrowserProviderHost( On 2015/09/16 00:56:41, michaeln wrote: > This seems out of place here? All it's doing is xfering ownership from the > URLRequest+SWReqHandler to the ContextCore? See other comments about not needed > to do that at all. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.h:75: ServiceWorkerRequestHandler* service_worker_handler_; On 2015/09/16 00:56:42, michaeln wrote: > Is this rawptr needed? It can be looked up as needed given the URLRequest ptr. A > NavigationResourceHandler helper method might make sense to connect the dots. > > ServiceWorkerRequestHandle* > NavigationResourceHandler::GetServiceWorkerRequestHandler() { > ServiceWorkerRequestHandle::GetHandler(request()); > } > > I think that'd help with code grokability compared to a rawptr. > > edit: Hmmm... past init time, the loader code may not need to get back to the > handler. See other comments about how this probably isn't needed. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2075: ServiceWorkerRequestHandler::InitializeHandler( On 2015/09/16 00:56:42, michaeln wrote: > I think it'd be worth creating a new Init method for this case in particular > instead of swiss-army-knifing the existing method. Considerably more needs to > happen now in addition to creating a handler and associating it with the request > object. > > ServiceWorkerRequestHandler::InitializeForNavigationRequest > - pre-create a providerhost instance with the no-process-id > - stash a ptr to it in the |core|->AddNavigationProviderHost(p) > - create and init the handler (akin to what InitializeHandler does today) > - assign ownership of the newly created providerhost to the handler > > Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:232: ServiceWorkerProviderHost* ServiceWorkerContextCore::CreateBrowserProviderHost( On 2015/09/16 00:56:42, michaeln wrote: > Probably this helper method isn't really needed, i think the function will only > be called from within the bowels of sw code (the > Handler::InitForNavigationRequest() method). Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:170: scoped_ptr<ServiceWorkerProviderHost> TakeBrowserProviderHost( On 2015/09/16 00:56:42, michaeln wrote: > Since the URLRequest + SWRequestHandler scope the lifetime of the 'host' until > ownership can be properly attributed to the target renderer, this central > ServiceWorkerContextCore class doesn't need to. > > I think i see why you have it this way, so SWDispatchHost::OnProviderCreated() > has a way to find the pre-created ServiceWorkerProviderHost instance. See > comments about having a map of rawptrs in the |core|. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:70: static base::StaticAtomicSequenceNumber sequence; On 2015/09/16 00:56:42, michaeln wrote: > Use of a thread safe construct here can be misleading. There's no need for > thread-safetey here, maybe use a simple static int here in place of the > StaticAtomicSequenceNumber. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:376: handler->SetBrowserProviderHost( On 2015/09/16 00:56:42, michaeln wrote: > This is an odd hidden artifact? See comments about makeing a distinct > ServiceWorkerRequestHandler::InitializeForNavigationRequest() method and > hoisting this xfer of ownership into that method. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:583: route_id_ = frame_id; On 2015/09/19 00:14:40, michaeln wrote: > Oh, we also need to set dispatcher_host_ in here. This method probably needs to > perform all the functions of CompleteCrossSiteTransfer. These two methods should > probably be merged. Added everything that was missing. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:71: NavigationURLLoaderImplCore* loader) { On 2015/09/16 00:56:42, michaeln wrote: > it'd be nice to not have this dependency Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:87: // Initialize a dummy SWProviderHost for browser-initiated navigations. On 2015/09/16 00:56:42, michaeln wrote: > "Dummy" might be misleading. It's a real 'host'. It gets constructed when a > navigation is initiated. That happens differently for brower vs renderer > intiated navs. Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:124: loader->SetServiceWorkerHandler(handler.get()); On 2015/09/19 00:14:40, michaeln wrote: > bah, handler.get() will always be null here given the handler.release() on the > previous line I added a new method with different semantics so this is no longer relevant. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:152: switches::kEnableBrowserSideNavigation)); On 2015/09/16 00:56:42, michaeln wrote: > just checking, but are these pre-existing methods only reachable with the > plznavigate flag is set? These methods are only reachable when the flag is not set (aka current navigation). There is no cross-site transfer semantics in the PlzNavigate world. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.h:102: // navigations. On 2015/09/16 00:56:42, michaeln wrote: > These public methods may not be needed if the xfer can be handled as handwaved > at? Done. https://codereview.chromium.org/1294243004/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.h:125: scoped_ptr<ServiceWorkerProviderHost> temporary_host_; On 2015/09/16 00:56:42, michaeln wrote: > its good to annotate with // PlzNavigate, picking a name that suggest this has > something todo with that would be even better, maybe navigation_host_ ? This has been removed. https://codereview.chromium.org/1294243004/diff/160001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/160001/content/common/navigat... content/common/navigation_params.h:97: int service_worker_provider_id; On 2015/09/18 23:03:56, michaeln wrote: > This needs to be added to the IPC traits for this class in frame_messages.h too. Done. https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:521: extra_data->service_worker_provider_id()); On 2015/09/18 23:03:56, michaeln wrote: > Ok, this id is definitely not useful. > > The object it identifies will be deleted prior to startLoad() returning and any > other msg being processed by the loop. According to nate this is by desigin. The > original policy/provisional loader+swprovider is deleted (per the snippet below) > and a new loader+swprovider gets created if/when the browser process decides to > delegate the navigation back to the this child process. > > snipped from FrameLoader::startLoad(...) > > // stopAllLoaders can detach the LocalFrame, so protect it. > RefPtrWillBeRawPtr<LocalFrame> protect(m_frame.get()); > if ((!m_policyDocumentLoader->shouldContinueForNavigationPolicy(...) { > detachDocumentLoader(m_policyDocumentLoader); > return; > } > > The MakeCommonNavigationParams() function is called inside of > m_policyDocumentLoader->shouldContinueForNavigationPolicy. The documentLoader > and its associated ServiceWorkerNetworkProvider are deleted inside of > detachDocumentLoader(). > > Eventually we get an ipc to RenderFrameImpl::OnCommitNavigation(). As currently > coded in the cl, the value of CommonNavigationParams.service_worker_provider_id > in that message is kInvalidServiceWorkerProviderId. Yet the stream override url > has an actual blob:url in it. > > I'll look more closely at the corresponding control flow in the browser process? This has been removed. https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:2583: if (original_request.extraData()) { On 2015/09/16 00:56:42, michaeln wrote: > I think this new bit of code is specific to PlzNavigate, if thats right please > annotate it with the // PlzNavigate comment too. I think the extraData() only > applies to PlzNavigate initiated navigations, is that right? > > I really appreciate the use of the PlzNavigate comment to annotate where the two > different modes of doing navigations differ. Otherwise we have mysteries like > "why is extra_data sometimes null and other times not and does it matter" that > nearly nobody knows the answer to. Done. There's a bit of duplication here now but I find it easier to understand this way since the tests are different for PlzNavigate anyway. https://codereview.chromium.org/1294243004/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:4860: FetchRedirectMode::FOLLOW_MODE); On 2015/09/18 23:03:56, michaeln wrote: > I've patched it in and am seeing these DCHECKs get hit on every navigation? > > Is there something more I need to do in addition to running with > --enable-browser-side-navigation to test things? That's because the values for navigation requests have changed in the meantime. I fixed them. https://codereview.chromium.org/1294243004/diff/180001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl_core.cc (right): https://codereview.chromium.org/1294243004/diff/180001/content/browser/loader... content/browser/loader/navigation_url_loader_impl_core.cc:56: ResourceDispatcherHostImpl::Get()->BeginNavigationRequest( On 2015/09/23 19:18:17, michaeln wrote: > Upon return from this function, the newly created swproviderhost should be > retrievable via the sw constructs setup underneath this function call. Where is > the commitnav message sent, the id value of the host needs to make it from here > to there? This is now completely different.
This is looking pretty good! I think its close. I haven't looked at tests yet. Please think about what additional testing we could/should have for this? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/frame_host/navigation_handle_impl.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/frame_host/navigation_handle_impl.cc:53: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( picking the partition based on the url is actually not correct, there's a bug open related to this. maybe put a comment referring to crbug/513539 https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/frame_host/navigation_handle_impl.h (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/frame_host/navigation_handle_impl.h:23: class ServiceWorkerContext; i don't think this forward is needed https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/frame_host/navigation_handle_impl.h:181: // The SWProvider ID used for navigations. for non-plznav navigations, this should probably be a kInvalid number https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/frame_host/navigator_delegate.h (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; This method seems out of place on this interface. Most of the method here here have todo with the navstack telling some outside agency what's happening. This method is not like that. https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/frame_host/render_frame_host_impl.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/frame_host/render_frame_host_impl.cc:917: ServiceWorkerProviderHost::GetNextNavigationProviderId()); Is this OnDidCommitProvisionalLoad method used in the oldnav case and not the plznav case? Has the navigation actually already happened in this case and the renderer is just telling the browser about it after the fact? If so, the we shouldn't need to generate provider id because we won't be creating a host after the fact, and can use a kInvalid constant here. https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/loader/navigation_url_loader_impl.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/loader/navigation_url_loader_impl.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( another usage of BrowserContext::GetStoragePartitionForSite that needs to be poked at eventually https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/loader/resource_dispatcher_host_impl.cc:1963: ServiceWorkerContextWrapper* service_worker_context) { interesting failure mode potential, what if service_worker_content != loader->service_worker_context ??? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/loader/resource_dispatcher_host_impl.cc:2101: // TODO: sync loads should set skip_service_worker to true. can there be a sync plznav load? if not probably should remove the comment https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/loader/resource_dispatcher_host_impl.h:281: ServiceWorkerContextWrapper* service_worker_context); its more common to pass refptrs into methods as const scoped_refptr<T>& https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/service_worker/service_worker_provider_host.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_provider_host.cc:71: int ServiceWorkerProviderHost::GetNextNavigationProviderId() { please DCHECK_CURRENTLY_ON(BrowserThread::UI), since its unusual for sw code to run on the UI thread i'd like to call attention to that https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_provider_host.cc:72: if (base::CommandLine::ForCurrentProcess()->HasSwitch( Does this need to be a runtime check? I don't think this method should be getting called unless this switch is set. If that's right, can you change this to a DCHECK instead. https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_provider_host.cc:539: switches::kEnableBrowserSideNavigation)); ah, i didn't notice the ! char! Ok, thanx for pointing out the CrossSiteXferring and PlzNavigating are mutually exclusive. Is there reason to CHECK this or would a DCHECK to document it and catch programmer errors be enuf? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_provider_host.cc:574: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( same question about CHECK vs DCHECK? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_provider_host.cc:587: for (const GURL& pattern : associated_patterns_) CompleteCrossSiteTransfer and CompleteNavigationInitialized contain a lot of the same code, can you factor out the common bits and reuse it https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... File content/browser/service_worker/service_worker_request_handler.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:11: #include "content/browser/loader/navigation_url_loader_impl_core.h" are the navstack includes still needed? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:23: #include "ipc/ipc_message.h" is this needed? what brought this include in? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:70: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( CHECK vs DCHECK? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:91: context_wrapper->context()->AddNavigationProviderHost(provider_host); I really don't like the passing around of int types as a way of expressing ownership. It's hard to follow. Somebody trying to read this code a few years from now will appreciate clarifying how this works in code. It'd be good to make the xfer of ownership more obvious. Upon return from this function there is no scoped_ptr to the object anywhere so who exactly owns it is not at all clear. The navstack is resposible for taking care to delete it eventually by id but that is not at all obvious. InitializeForNavigation returns nothing but as a hidden artifact it allocates an object that the caller is reponsible for managing. The method the caller must use when its done is a method on a completely different class. And the ids are coined by yet another class. I'd like to consolidate and clean that up. It's not difficult to make it more obvious. One way clarify could be to have a wrapper 'handle' class coordinate its construction and conditional destruction so its all inside of one small .cc file inside instead of being scattered about. That wrapper could also help to avoid things like generating an id without ever generating a corresponding object. Or retaining the id value after the object it refers to has been deleted. https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:147: if (skip_service_worker) { please refctor and share common code between these two init methods https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/browser... content/browser/service_worker/service_worker_request_handler.cc:193: CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch( ditto question about CHECK vs DCHECK https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/child/s... File content/child/service_worker/service_worker_network_provider.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/child/s... content/child/service_worker/service_worker_network_provider.cc:32: return kInvalidServiceWorkerProviderId; This early return prevent the creation of a 'host' in the browser process which makes it impossible for sandboxed frames to load via serviceworkers. That is intentional. How are sandboxed frames excluded from using serviceworkers in the plznav era? https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/rendere... File content/renderer/render_frame_impl.cc (right): https://chromiumcodereview.appspot.com/1294243004/diff/220001/content/rendere... content/renderer/render_frame_impl.cc:2589: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; Are the effectiveSandboxFlags() sent as part of the FrameHostMsg_BeginNavigation message? I don't thing they are. In this case we need to skip the serviceworker. There's no need to create a providerhost in the browser process. (see related comments in SWNPH.c)
Thanks! I think the design is better in this patch. There are still a few issues, see my comments below. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:66: // PlzNavigate nit: I would put this comment above the if condition (same in other places where this happens). Also precise what type of Host we're talking about by using the full class name. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:65: int service_worker_provider_id); I do not like having the service_worker_provider_id be a parameter to this function, that is used both by PlzNavigate and non-PlzNavigate code. I think we could consider the following options: - having the id be generated in the constructor of NavigationHandleImpl, and then put it in the RequestNavigationParams. - add a boolean to the request params to check whether they are constructed in the browser or the renderer. If they are constructed in the browser, then generate a SWid, otherwise the SWid is kInvalid... In that case, when the handle is created, we can set its swid to the one in the request params. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:181: // The SWProvider ID used for navigations. nit: use the full class name for SWProvider. Also, as michaeln@ suggested, mention its value for the non-PlzNavigate case. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:125: RequestNavigationParams request_params; We should switch to instantiating the RequestParams here by using the full constructor, so that we do not have the risk of missing a newly added parameter. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:129: ServiceWorkerProviderHost::GetNextNavigationProviderId(); It seems very weird to me that we should be using a static method of an IO-thread class. Is that even thread safe? In any case, I think there could be other ways to generate the id (see my comments in NavigationHandle). https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_request_info.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request_info.h:37: const RequestNavigationParams request_params; I think we can avoid copying a whole data struct for 1 boolean parameter. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/01 01:42:15, michaeln wrote: > This method seems out of place on this interface. Most of the method here here > have todo with the navstack telling some outside agency what's happening. This > method is not like that. Agreed. At the end of the day, NavigatorDelegate is about the Navigator informing the WebContents of the navigation events happening. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:917: ServiceWorkerProviderHost::GetNextNavigationProviderId()); On 2015/10/01 01:42:15, michaeln wrote: > Is this OnDidCommitProvisionalLoad method used in the oldnav case and not the > plznav case? Has the navigation actually already happened in this case and the > renderer is just telling the browser about it after the fact? > > If so, the we shouldn't need to generate provider id because we won't be > creating a host after the fact, and can use a kInvalid constant here. The case here is a synchronous navigation. That means the document has not and will not be changed, same for the SWProvider. Therefore, we should not attempt to create a new id for this navigation. https://codereview.chromium.org/1294243004/diff/220001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/common/navigat... content/common/navigation_params.h:272: // SWProvider ID for navigations. Use the full class name. https://codereview.chromium.org/1294243004/diff/220001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/public/browser... content/public/browser/navigation_handle.h:101: int service_worker_provider_id); This _should not_ be exposed outside of content, hence it should not be in the public interface. https://codereview.chromium.org/1294243004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:2585: ServiceWorkerProviderType provider_type = There's a lot of repeated code here. How about: bool browser_side_navigation = base::CommandLine... if (browser_side_navigation && !content_initiated) return; int service_worker_provider_id = browser_side_navigation ? ... : ...; .... I think that would be clearer.
Thanks for the quick review! More comments inline, but this is the summary of remaining points (if I didn't forget anything): -The extra method in NavigationDelegate, I welcome any alternative to access the BrowserContext. -The extra argument in NavigationDelegate ctor. My first attempt was to initialize the SWProviderID here, but NavigationRequestInfo was initialized before that, resulting in the wrong value being passed to the ServiceWorkerDispatcherHost. -I changed the new methods in ServiceWorkerContextCore to take and return soped_ptr. Does that clarify the ownership semantics? -I did some changes in render_frame_impl.cc to fix the failing tests. I am not sure this is the best way to proceed. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:53: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( On 2015/10/01 01:42:15, michaeln wrote: > picking the partition based on the url is actually not correct, there's a bug > open related to this. maybe put a comment referring to crbug/513539 Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:66: // PlzNavigate On 2015/10/01 12:41:26, clamy wrote: > nit: I would put this comment above the if condition (same in other places where > this happens). > Also precise what type of Host we're talking about by using the full class name. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:23: class ServiceWorkerContext; On 2015/10/01 01:42:15, michaeln wrote: > i don't think this forward is needed Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:65: int service_worker_provider_id); On 2015/10/01 12:41:26, clamy wrote: > I do not like having the service_worker_provider_id be a parameter to this > function, that is used both by PlzNavigate and non-PlzNavigate code. I think we > could consider the following options: > - having the id be generated in the constructor of NavigationHandleImpl, and > then put it in the RequestNavigationParams. That was my first idea too, but the NavigationRequestInfo is created before the NavigationHandleImpl, meaning it is too late to add the information to the RequestNavigationParams after NavigationHandleImpl ctor is called. > - add a boolean to the request params to check whether they are constructed in > the browser or the renderer. If they are constructed in the browser, then > generate a SWid, otherwise the SWid is kInvalid... In that case, when the handle > is created, we can set its swid to the one in the request params. Same problem as above, it is too late to create the ID in this ctor and pass it back to NavigationRequestInfo. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:181: // The SWProvider ID used for navigations. On 2015/10/01 12:41:26, clamy wrote: > nit: use the full class name for SWProvider. Also, as michaeln@ suggested, > mention its value for the non-PlzNavigate case. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:125: RequestNavigationParams request_params; On 2015/10/01 12:41:27, clamy wrote: > We should switch to instantiating the RequestParams here by using the full > constructor, so that we do not have the risk of missing a newly added parameter. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:129: ServiceWorkerProviderHost::GetNextNavigationProviderId(); On 2015/10/01 12:41:27, clamy wrote: > It seems very weird to me that we should be using a static method of an > IO-thread class. Is that even thread safe? In any case, I think there could be > other ways to generate the id (see my comments in NavigationHandle). Since it is only called here, I moved it in the anonymous namespace in this file instead. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_request_info.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_request_info.h:37: const RequestNavigationParams request_params; On 2015/10/01 12:41:27, clamy wrote: > I think we can avoid copying a whole data struct for 1 boolean parameter. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/01 12:41:27, clamy wrote: > On 2015/10/01 01:42:15, michaeln wrote: > > This method seems out of place on this interface. Most of the method here here > > have todo with the navstack telling some outside agency what's happening. This > > method is not like that. > > Agreed. At the end of the day, NavigatorDelegate is about the Navigator > informing the WebContents of the navigation events happening. This was the only way I found to access the BrowserContext from the NavigationHandleImpl that did not result in an access violation somewhere. This class is implemented as WebContentsImpl, but the presubmit prevent including the WebContentsImpl header from frame_host (for good reasons), and I cannot cast the NavigatorDelegate as a WebContents either. I'd be happy to find another way of handling this. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:917: ServiceWorkerProviderHost::GetNextNavigationProviderId()); On 2015/10/01 12:41:27, clamy wrote: > On 2015/10/01 01:42:15, michaeln wrote: > > Is this OnDidCommitProvisionalLoad method used in the oldnav case and not the > > plznav case? Has the navigation actually already happened in this case and the > > renderer is just telling the browser about it after the fact? > > > > If so, the we shouldn't need to generate provider id because we won't be > > creating a host after the fact, and can use a kInvalid constant here. > > The case here is a synchronous navigation. That means the document has not and > will not be changed, same for the SWProvider. Therefore, we should not attempt > to create a new id for this navigation. Besides, synchronous navigations should not use the ServiceWorker. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... content/browser/loader/navigation_url_loader_impl.cc:28: StoragePartition* partition = BrowserContext::GetStoragePartitionForSite( On 2015/10/01 01:42:15, michaeln wrote: > another usage of BrowserContext::GetStoragePartitionForSite that needs to be > poked at eventually Added the bug reference. https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1963: ServiceWorkerContextWrapper* service_worker_context) { On 2015/10/01 01:42:15, michaeln wrote: > interesting failure mode potential, what if > > service_worker_content != loader->service_worker_context > > ??? I'm not sure I follow? This is called from the loader with this exact argument. Do you want to add an accessor from the loader instead? https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2101: // TODO: sync loads should set skip_service_worker to true. On 2015/10/01 01:42:15, michaeln wrote: > can there be a sync plznav load? if not probably should remove the comment This is now taken care of, see comments in render_frame_host_impl.cc https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:281: ServiceWorkerContextWrapper* service_worker_context); On 2015/10/01 01:42:16, michaeln wrote: > its more common to pass refptrs into methods as > const scoped_refptr<T>& Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:71: int ServiceWorkerProviderHost::GetNextNavigationProviderId() { On 2015/10/01 01:42:16, michaeln wrote: > please DCHECK_CURRENTLY_ON(BrowserThread::UI), since its unusual for sw code to > run on the UI thread i'd like to call attention to that I moved it so there should no longer be any confusion here. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:72: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 01:42:16, michaeln wrote: > Does this need to be a runtime check? I don't think this method should be > getting called unless this switch is set. If that's right, can you change this > to a DCHECK instead. It's called in both cases actually, when initializing RequestNavigationParams. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:539: switches::kEnableBrowserSideNavigation)); On 2015/10/01 01:42:16, michaeln wrote: > ah, i didn't notice the ! char! > > Ok, thanx for pointing out the CrossSiteXferring and PlzNavigating are mutually > exclusive. Is there reason to CHECK this or would a DCHECK to document it and > catch programmer errors be enuf? I think we should leave a CHECK here as stated above. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:574: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 01:42:16, michaeln wrote: > same question about CHECK vs DCHECK? If we get here, it means the browser received a negative provider ID from the renderer (could have been compromised) and was able to retrieve a SWProviderHost with that ID from the map that should only be used with PlzNavigate. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:587: for (const GURL& pattern : associated_patterns_) On 2015/10/01 01:42:16, michaeln wrote: > CompleteCrossSiteTransfer and CompleteNavigationInitialized contain a lot of the > same code, can you factor out the common bits and reuse it Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:11: #include "content/browser/loader/navigation_url_loader_impl_core.h" On 2015/10/01 01:42:16, michaeln wrote: > are the navstack includes still needed? No, that's leftovers from earlier attempts, removed. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:23: #include "ipc/ipc_message.h" On 2015/10/01 01:42:16, michaeln wrote: > is this needed? what brought this include in? MSG_ROUTING_NONE https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:70: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 01:42:16, michaeln wrote: > CHECK vs DCHECK? We use CHECK for PlzNavigate checks in the browser. If current navigation end up calling this method, something went very very wrong down the road. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:91: context_wrapper->context()->AddNavigationProviderHost(provider_host); On 2015/10/01 01:42:16, michaeln wrote: > I really don't like the passing around of int types as a way of expressing > ownership. It's hard to follow. Somebody trying to read this code a few years > from now will appreciate clarifying how this works in code. > > It'd be good to make the xfer of ownership more obvious. Upon return from this > function there is no scoped_ptr to the object anywhere so who exactly owns it is > not at all clear. The navstack is resposible for taking care to delete it > eventually by id but that is not at all obvious. InitializeForNavigation returns > nothing but as a hidden artifact it allocates an object that the caller is > reponsible for managing. The method the caller must use when its done is a > method on a completely different class. And the ids are coined by yet another > class. > > I'd like to consolidate and clean that up. > > It's not difficult to make it more obvious. One way clarify could be to have a > wrapper 'handle' class coordinate its construction and conditional destruction > so its all inside of one small .cc file inside instead of being scattered about. > That wrapper could also help to avoid things like generating an id without ever > generating a corresponding object. Or retaining the id value after the object it > refers to has been deleted. > I changed the API in ServiceWorkerContextCore to use scoped_ptr instead. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:147: if (skip_service_worker) { On 2015/10/01 01:42:16, michaeln wrote: > please refctor and share common code between these two init methods This is kinda awkward due to the large number of arguments passed around but done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:193: CHECK(!base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 01:42:16, michaeln wrote: > ditto question about CHECK vs DCHECK There is no cross-site transfer mechanics in PlzNavigate. Once again if this ends up being called, we have a big problem on our hands and it is much safer to crash. https://codereview.chromium.org/1294243004/diff/220001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:32: return kInvalidServiceWorkerProviderId; On 2015/10/01 01:42:16, michaeln wrote: > This early return prevent the creation of a 'host' in the browser process which > makes it impossible for sandboxed frames to load via serviceworkers. That is > intentional. > > How are sandboxed frames excluded from using serviceworkers in the plznav era? This is now taken care of by checking the flags in the FrameTreeNode in the browser. https://codereview.chromium.org/1294243004/diff/220001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/common/navigat... content/common/navigation_params.h:272: // SWProvider ID for navigations. On 2015/10/01 12:41:27, clamy wrote: > Use the full class name. Done. https://codereview.chromium.org/1294243004/diff/220001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/public/browser... content/public/browser/navigation_handle.h:101: int service_worker_provider_id); On 2015/10/01 12:41:27, clamy wrote: > This _should not_ be exposed outside of content, hence it should not be in the > public interface. I removed it, since this is for unit tests, the value should not matter anyway. https://codereview.chromium.org/1294243004/diff/220001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:2585: ServiceWorkerProviderType provider_type = On 2015/10/01 12:41:27, clamy wrote: > There's a lot of repeated code here. How about: > bool browser_side_navigation = base::CommandLine... > if (browser_side_navigation && !content_initiated) > return; > int service_worker_provider_id = browser_side_navigation ? ... : ...; > .... > > I think that would be clearer. I reworked the whole part here. https://codereview.chromium.org/1294243004/diff/220001/content/renderer/rende... content/renderer/render_frame_impl.cc:2589: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; On 2015/10/01 01:42:16, michaeln wrote: > Are the effectiveSandboxFlags() sent as part of the FrameHostMsg_BeginNavigation > message? I don't thing they are. In this case we need to skip the serviceworker. > There's no need to create a providerhost in the browser process. (see related > comments in SWNPH.c) That's my mistake. I'm setting the provider_id to -1 in the browser for sandboxed frames.
The CQ bit was checked by fdegans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1294243004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1294243004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:65: int service_worker_provider_id); On 2015/10/01 18:29:55, Fabrice wrote: > On 2015/10/01 12:41:26, clamy wrote: > > I do not like having the service_worker_provider_id be a parameter to this > > function, that is used both by PlzNavigate and non-PlzNavigate code. I think > > That was my first idea too, but the NavigationRequestInfo is created before the > NavigationHandleImpl, meaning it is too late to add the information to the > RequestNavigationParams after NavigationHandleImpl ctor is called. I think your talking about the construction sequences below and how the handle is created after NavigitaionRequest (whose constructor creates the NavigationRequestInfo struct). Is that right? Also am I right in thinking that these are the only two callsites where we need to precreate a swprovider host? void NavigatorImpl::OnBeginNavigation ... frame_tree_node->CreatedNavigationRequest( NavigationRequest::CreateRendererInitiated( frame_tree_node, common_params, begin_params, body, controller_->GetLastCommittedEntryIndex(), controller_->GetEntryCount())); NavigationRequest* navigation_request = frame_tree_node->navigation_request(); navigation_request->CreateNavigationHandle(delegate_); void NavigatorImpl::RequestNavigation .... frame_tree_node->CreatedNavigationRequest( NavigationRequest::CreateBrowserInitiated( frame_tree_node, dest_url, dest_referrer, frame_entry, entry, navigation_type, is_same_document_history_load, navigation_start, controller_)); NavigationRequest* navigation_request = frame_tree_node->navigation_request(); navigation_request->CreateNavigationHandle(delegate_); It looks pretty straightforward to mildy alter this construction sequence. You could pass the delegate_ into the factory methods and have the handle be created during NavigationRequest construction. You could probably get rid of the CreateNavigationHandle method and retain the const getter and TakeNavigationHandle method. Then the NavigationRequest could internally construct the NavigationHandle which could internally construct the ServiceWorkerNavigationHandle. Or the NavigationRequest::CreateXxxxInitiated methods could take a scoped_ptr<ServiceWorkerNavigationHandle> as input. I think something like that would be nice cleanup. void NavigatorImpl::OnBeginNavigation ... frame_tree_node->CreatedNavigationRequest( NavigationRequest::CreateRendererInitiated( frame_tree_node, common_params, begin_params, body, controller_->GetLastCommittedEntryIndex(), controller_->GetEntryCount(), delegate_, make_scoped_ptr(new ServiceWorkerNavigationHandle(swcontext_wrapper)); void NavigatorImpl::RequestNavigation .... frame_tree_node->CreatedNavigationRequest( NavigationRequest::CreateBrowserInitiated( frame_tree_node, dest_url, dest_referrer, frame_entry, entry, navigation_type, is_same_document_history_load, navigation_start, controller_, delegate_, make_scoped_ptr(new ServiceWorkerNavigationHandle(swcontext_wrapper)))); https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:539: switches::kEnableBrowserSideNavigation)); On 2015/10/01 18:29:55, Fabrice wrote: > On 2015/10/01 01:42:16, michaeln wrote: > > ah, i didn't notice the ! char! > > > > Ok, thanx for pointing out the CrossSiteXferring and PlzNavigating are > mutually > > exclusive. Is there reason to CHECK this or would a DCHECK to document it and > > catch programmer errors be enuf? > > I think we should leave a CHECK here as stated above. If this check is to defend against bad ipc inputs, please hoist the test higher up in the stack as mentioned below. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:574: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 18:29:55, Fabrice wrote: > On 2015/10/01 01:42:16, michaeln wrote: > > same question about CHECK vs DCHECK? > > If we get here, it means the browser received a negative provider ID from the > renderer (could have been compromised) and was able to retrieve a SWProviderHost > with that ID from the map that should only be used with PlzNavigate. Please use BadMessageRecieved for that and put the test closer to where messages are first received. It's good to put checks of that nature where the inputs from the renderer are first received. Also we don't want to kill the browser process in cases like that. We want to kill the borken renderer. See usages of BadMessageRecieved for examples of that. Notice that all (modulo one special case) uses of that function under service_worker/ are in the ServiceWorkerDispatcherHost class. That is the ipc message dispatching class which first handles a message from potentially busted renderer. If your other usages of CHECK are to defend against bad ipc inputs, please shuffle them around to where messages are initially received and convert them to BadMessageReceived. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; The NavigationController interface has a GetBrowserContext() method and the Navigator has a navigation_controller_ data member. We should probably use that to 'navigate' up the the browser context object. Just curious. I see FrameTreeNodes have a reference to a Navigator. What is the scope of a single Navigator(Impl) instance? How many nodes does a NavigatorImpl servce concurrently and over its lifetime? From the looks of the NavigationMetricsData, it looks like it could be a 1 to 1 relationship? https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:165: scoped_ptr<ServiceWorkerProviderHost> provider_host); passing a scoped_ptr here is misleading because the core does not actually take ownership of the object, it will never delete the object on its own https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:58: static int kVirtualProcessIDForBrowserRequest; please move this const to service_worker_types.h and really make it a const https://codereview.chromium.org/1294243004/diff/260001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:57: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( plz vett ipc params higher up in the stack https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2579: bool browser_side_navigation = this block of code is still tricky to follow and easily error prone? https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2582: if ((browser_side_navigation && content_initiated) || We end up w/o having created a ServiceWorkerNetworkProvider in this case which i think may be a bug, i think is current sources we always return from this method with a non-null provider. We should retain that behavior. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2584: ServiceWorkerNetworkProvider::FromDocumentState( i think u could put this up top (at line) as an early return condition independent of plznav mode. if a provider exists we don't want to create a new one <period>. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2601: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; The provider_type is only used by the ServiceWorkerNetworkProvider class to possibly compute a kInvalid id. Since this class now has so much logic dedicated to dealing with that id value, for the plznav case, lets hoist all of the logic into this class and keep the ServiceWorkerNetworkProvider ctor simple. ServiceWorkerNetworkProvider(route_id, provider_id) { : provider_id_(provider_id) { if (provider_id_ == kInvalid) return; context_ = new ServiceWorkerProviderContext(provider_id_); if (!ChildThreadImpl::current()) return; ChildThreadImpl::current()->Send(...);l } And retain the existing ctor for use by non-plz nav case. I think it can be implemented in terms of the other. ServiceWorkerNetworkProvider(route_id, provider_type) { : ServiceWorkerNetworkProvider(route_id, GenerateIdForType(type)) {} https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2604: if (!browser_side_navigation) { this could be an else clause
https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:65: int service_worker_provider_id); On 2015/10/01 23:11:11, michaeln wrote: > On 2015/10/01 18:29:55, Fabrice wrote: > > On 2015/10/01 12:41:26, clamy wrote: > > > I do not like having the service_worker_provider_id be a parameter to this > > > function, that is used both by PlzNavigate and non-PlzNavigate code. I think > > > > That was my first idea too, but the NavigationRequestInfo is created before > the > > NavigationHandleImpl, meaning it is too late to add the information to the > > RequestNavigationParams after NavigationHandleImpl ctor is called. > > I think your talking about the construction sequences below and how the handle > is created after NavigitaionRequest (whose constructor creates the > NavigationRequestInfo struct). Is that right? Also am I right in thinking that > these are the only two callsites where we need to precreate a swprovider host? > > void NavigatorImpl::OnBeginNavigation > ... > frame_tree_node->CreatedNavigationRequest( > NavigationRequest::CreateRendererInitiated( > frame_tree_node, common_params, begin_params, body, > controller_->GetLastCommittedEntryIndex(), > controller_->GetEntryCount())); > NavigationRequest* navigation_request = frame_tree_node->navigation_request(); > navigation_request->CreateNavigationHandle(delegate_); > > > void NavigatorImpl::RequestNavigation > .... > frame_tree_node->CreatedNavigationRequest( > NavigationRequest::CreateBrowserInitiated( > frame_tree_node, dest_url, dest_referrer, frame_entry, entry, > navigation_type, is_same_document_history_load, navigation_start, > controller_)); > NavigationRequest* navigation_request = frame_tree_node->navigation_request(); > navigation_request->CreateNavigationHandle(delegate_); > > > It looks pretty straightforward to mildy alter this construction sequence. You > could pass the delegate_ into the factory methods and have the handle be created > during NavigationRequest construction. You could probably get rid of the > CreateNavigationHandle method and retain the const getter and > TakeNavigationHandle method. Then the NavigationRequest could internally > construct the NavigationHandle which could internally construct the > ServiceWorkerNavigationHandle. Or the NavigationRequest::CreateXxxxInitiated > methods could take a scoped_ptr<ServiceWorkerNavigationHandle> as input. > > I think something like that would be nice cleanup. > > void NavigatorImpl::OnBeginNavigation > ... > frame_tree_node->CreatedNavigationRequest( > NavigationRequest::CreateRendererInitiated( > frame_tree_node, common_params, begin_params, body, > controller_->GetLastCommittedEntryIndex(), > controller_->GetEntryCount(), delegate_, > make_scoped_ptr(new ServiceWorkerNavigationHandle(swcontext_wrapper)); > > > void NavigatorImpl::RequestNavigation > .... > frame_tree_node->CreatedNavigationRequest( > NavigationRequest::CreateBrowserInitiated( > frame_tree_node, dest_url, dest_referrer, frame_entry, entry, > navigation_type, is_same_document_history_load, navigation_start, > controller_, delegate_, > make_scoped_ptr(new > ServiceWorkerNavigationHandle(swcontext_wrapper)))); > While it does look straightforward to alter the construction sequence, there is a reason the NavigationHandle is not constructed in the NavigationRequest in the first place. The construction and destruction of the NavigationHandle trigger calls to WCO::DidStartNavigation and WCO::DidFinishNavigation. In order not to confuse observers, we want the call to DidFinishNavigation to arrive first for the old navigation and then the call to DidStartNavigation for the second navigation. This means destroying the old NavigationHandle before creating the new one. Since the NavigationRequest that owns the old handle is destroyed in frame_tree_node->CreatedNavigationRequest, if the new request already contains the new handle, we break this ordering. We considered creating the NavigationRequest inside the FrameTreeNode (after the old request has been destroyed), but ruled that out since it's bringing too much navigation knowledge that FrameTreeNode is not meant to handle. I'm also thinking that we will want to merge NavigationRequest and NavigationHandle into a single class once PlzNavigate is enabled by default, hence why it was left this way. Thinking on the problem some more, I think we should initialize the SWProviderID neither in the constructor of RequestNavigationParams nor NavigationHandle. The SWProviderID should only be initialized when the NavigationRequest is sent to the network stack, when we are sure we will create a SWProviderHost. Otherwise, I'm worried we may introduce some bugs with navigations that commit without making a network request. In order to do so, the id will be set in the RequestNavigationParams and the NavigationHandle when NavigationRequest::BeginNavigation determined that a network request needed to be made. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/01 18:29:55, Fabrice wrote: > On 2015/10/01 12:41:27, clamy wrote: > > On 2015/10/01 01:42:15, michaeln wrote: > > > This method seems out of place on this interface. Most of the method here > here > > > have todo with the navstack telling some outside agency what's happening. > This > > > method is not like that. > > > > Agreed. At the end of the day, NavigatorDelegate is about the Navigator > > informing the WebContents of the navigation events happening. > > This was the only way I found to access the BrowserContext from the > NavigationHandleImpl that did not result in an access violation somewhere. > This class is implemented as WebContentsImpl, but the presubmit prevent > including the WebContentsImpl header from frame_host (for good reasons), and I > cannot cast the NavigatorDelegate as a WebContents either. > I'd be happy to find another way of handling this. It'd be reasonable to add a pointer to the FTN in the NavigationHandle. From there you can get the NavigationController that will give you the BrowserContext. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:161: int service_worker_provider_id) const; See my comment on the previous patch set on why we should not be setting the service_worker_provider_id at construction time but when we know the request should be sent to the network. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:64: int service_worker_provider_id); See the comment in the previous patch set on why we should only set the id when we know the navigation will make a network request. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:182: // Set to kInvalidServiceWorkerProviderId for current navigation. I don't understand the last comment. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:58: // PlzNavigate No need for PlzNavigate prefix, this is a PlzNavigate only class. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:69: if (base::CommandLine::ForCurrentProcess()->HasSwitch( This is a PlzNavigate only class, so no need to handle the current architecture. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/01 23:11:12, michaeln wrote: > The NavigationController interface has a GetBrowserContext() method and the > Navigator has a navigation_controller_ data member. We should probably use that > to 'navigate' up the the browser context object. > > Just curious. I see FrameTreeNodes have a reference to a Navigator. What is the > scope of a single Navigator(Impl) instance? How many nodes does a NavigatorImpl > servce concurrently and over its lifetime? From the looks of the > NavigationMetricsData, it looks like it could be a 1 to 1 relationship? > There is one NavigatorImpl per WebContents, and its lifetime is the lifetime of the WebContents (same for the NavigationController). It services all the FTN of the FrameTree. In this case, getting the BrowserContext through the NavigationController is the right thing to do. In order to get to the NavigationController, we should just keep a pointer to the FTN in the NavigationHandle (a raw pointer is safe). In fact, it can replace the delegate (since we can also get the Navigator and it's delegate from the FTN).
Thanks! New patch, new comments, new answers. I changed the map in SWContextCore to handle linked_ptr instead so the core now explicitly takes ownership. Bunch of arguments changed here and there. the service_worker_provider_id is not initialized later so the NavigationHandle ctor remains simple. https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:65: int service_worker_provider_id); On 2015/10/02 10:34:44, clamy wrote: > On 2015/10/01 23:11:11, michaeln wrote: > > On 2015/10/01 18:29:55, Fabrice wrote: > > > On 2015/10/01 12:41:26, clamy wrote: > > > > I do not like having the service_worker_provider_id be a parameter to this > > > > function, that is used both by PlzNavigate and non-PlzNavigate code. I > think > > > > > > That was my first idea too, but the NavigationRequestInfo is created before > > the > > > NavigationHandleImpl, meaning it is too late to add the information to the > > > RequestNavigationParams after NavigationHandleImpl ctor is called. > > > > I think your talking about the construction sequences below and how the handle > > is created after NavigitaionRequest (whose constructor creates the > > NavigationRequestInfo struct). Is that right? Also am I right in thinking that > > these are the only two callsites where we need to precreate a swprovider host? > > > > void NavigatorImpl::OnBeginNavigation > > ... > > frame_tree_node->CreatedNavigationRequest( > > NavigationRequest::CreateRendererInitiated( > > frame_tree_node, common_params, begin_params, body, > > controller_->GetLastCommittedEntryIndex(), > > controller_->GetEntryCount())); > > NavigationRequest* navigation_request = > frame_tree_node->navigation_request(); > > navigation_request->CreateNavigationHandle(delegate_); > > > > > > void NavigatorImpl::RequestNavigation > > .... > > frame_tree_node->CreatedNavigationRequest( > > NavigationRequest::CreateBrowserInitiated( > > frame_tree_node, dest_url, dest_referrer, frame_entry, entry, > > navigation_type, is_same_document_history_load, navigation_start, > > controller_)); > > NavigationRequest* navigation_request = > frame_tree_node->navigation_request(); > > navigation_request->CreateNavigationHandle(delegate_); > > > > > > It looks pretty straightforward to mildy alter this construction sequence. You > > could pass the delegate_ into the factory methods and have the handle be > created > > during NavigationRequest construction. You could probably get rid of the > > CreateNavigationHandle method and retain the const getter and > > TakeNavigationHandle method. Then the NavigationRequest could internally > > construct the NavigationHandle which could internally construct the > > ServiceWorkerNavigationHandle. Or the NavigationRequest::CreateXxxxInitiated > > methods could take a scoped_ptr<ServiceWorkerNavigationHandle> as input. > > > > I think something like that would be nice cleanup. > > > > void NavigatorImpl::OnBeginNavigation > > ... > > frame_tree_node->CreatedNavigationRequest( > > NavigationRequest::CreateRendererInitiated( > > frame_tree_node, common_params, begin_params, body, > > controller_->GetLastCommittedEntryIndex(), > > controller_->GetEntryCount(), delegate_, > > make_scoped_ptr(new > ServiceWorkerNavigationHandle(swcontext_wrapper)); > > > > > > void NavigatorImpl::RequestNavigation > > .... > > frame_tree_node->CreatedNavigationRequest( > > NavigationRequest::CreateBrowserInitiated( > > frame_tree_node, dest_url, dest_referrer, frame_entry, entry, > > navigation_type, is_same_document_history_load, navigation_start, > > controller_, delegate_, > > make_scoped_ptr(new > > ServiceWorkerNavigationHandle(swcontext_wrapper)))); > > > > While it does look straightforward to alter the construction sequence, there is > a reason the NavigationHandle is not constructed in the NavigationRequest in the > first place. The construction and destruction of the NavigationHandle trigger > calls to WCO::DidStartNavigation and WCO::DidFinishNavigation. In order not to > confuse observers, we want the call to DidFinishNavigation to arrive first for > the old navigation and then the call to DidStartNavigation for the second > navigation. This means destroying the old NavigationHandle before creating the > new one. Since the NavigationRequest that owns the old handle is destroyed in > frame_tree_node->CreatedNavigationRequest, if the new request already contains > the new handle, we break this ordering. > > We considered creating the NavigationRequest inside the FrameTreeNode (after the > old request has been destroyed), but ruled that out since it's bringing too much > navigation knowledge that FrameTreeNode is not meant to handle. I'm also > thinking that we will want to merge NavigationRequest and NavigationHandle into > a single class once PlzNavigate is enabled by default, hence why it was left > this way. > > Thinking on the problem some more, I think we should initialize the SWProviderID > neither in the constructor of RequestNavigationParams nor NavigationHandle. The > SWProviderID should only be initialized when the NavigationRequest is sent to > the network stack, when we are sure we will create a SWProviderHost. Otherwise, > I'm worried we may introduce some bugs with navigations that commit without > making a network request. > > In order to do so, the id will be set in the RequestNavigationParams and the > NavigationHandle when NavigationRequest::BeginNavigation determined that a > network request needed to be made. That works thanks! https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/02 10:34:44, clamy wrote: > On 2015/10/01 18:29:55, Fabrice wrote: > > On 2015/10/01 12:41:27, clamy wrote: > > > On 2015/10/01 01:42:15, michaeln wrote: > > > > This method seems out of place on this interface. Most of the method here > > here > > > > have todo with the navstack telling some outside agency what's happening. > > This > > > > method is not like that. > > > > > > Agreed. At the end of the day, NavigatorDelegate is about the Navigator > > > informing the WebContents of the navigation events happening. > > > > This was the only way I found to access the BrowserContext from the > > NavigationHandleImpl that did not result in an access violation somewhere. > > This class is implemented as WebContentsImpl, but the presubmit prevent > > including the WebContentsImpl header from frame_host (for good reasons), and I > > cannot cast the NavigatorDelegate as a WebContents either. > > I'd be happy to find another way of handling this. > > It'd be reasonable to add a pointer to the FTN in the NavigationHandle. From > there you can get the NavigationController that will give you the > BrowserContext. Done. https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/220001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:539: switches::kEnableBrowserSideNavigation)); On 2015/10/01 23:11:12, michaeln wrote: > On 2015/10/01 18:29:55, Fabrice wrote: > > On 2015/10/01 01:42:16, michaeln wrote: > > > ah, i didn't notice the ! char! > > > > > > Ok, thanx for pointing out the CrossSiteXferring and PlzNavigating are > > mutually > > > exclusive. Is there reason to CHECK this or would a DCHECK to document it > and > > > catch programmer errors be enuf? > > > > I think we should leave a CHECK here as stated above. > > If this check is to defend against bad ipc inputs, please hoist the test higher > up in the stack as mentioned below. Looking at the calling stack, this is actually taken care of in ResourceDispatcherHostImpl::BeginRequest. Removing it here. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_entry_impl.h:161: int service_worker_provider_id) const; On 2015/10/02 10:34:44, clamy wrote: > See my comment on the previous patch set on why we should not be setting the > service_worker_provider_id at construction time but when we know the request > should be sent to the network. Done. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:64: int service_worker_provider_id); On 2015/10/02 10:34:44, clamy wrote: > See the comment in the previous patch set on why we should only set the id when > we know the navigation will make a network request. Done. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:182: // Set to kInvalidServiceWorkerProviderId for current navigation. On 2015/10/02 10:34:44, clamy wrote: > I don't understand the last comment. I clarified it is unused. Since it is unused, it doesn't matter what value it is set to. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:58: // PlzNavigate On 2015/10/02 10:34:44, clamy wrote: > No need for PlzNavigate prefix, this is a PlzNavigate only class. Done. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:69: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/02 10:34:44, clamy wrote: > This is a PlzNavigate only class, so no need to handle the current architecture. Done. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/02 10:34:44, clamy wrote: > On 2015/10/01 23:11:12, michaeln wrote: > > The NavigationController interface has a GetBrowserContext() method and the > > Navigator has a navigation_controller_ data member. We should probably use > that > > to 'navigate' up the the browser context object. > > > > Just curious. I see FrameTreeNodes have a reference to a Navigator. What is > the > > scope of a single Navigator(Impl) instance? How many nodes does a > NavigatorImpl > > servce concurrently and over its lifetime? From the looks of the > > NavigationMetricsData, it looks like it could be a 1 to 1 relationship? > > > > There is one NavigatorImpl per WebContents, and its lifetime is the lifetime of > the WebContents (same for the NavigationController). It services all the FTN of > the FrameTree. In this case, getting the BrowserContext through the > NavigationController is the right thing to do. In order to get to the > NavigationController, we should just keep a pointer to the FTN in the > NavigationHandle (a raw pointer is safe). In fact, it can replace the delegate > (since we can also get the Navigator and it's delegate from the FTN). Changed the argument to use a FrameTreeNode so this is removed. Thanks! https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:165: scoped_ptr<ServiceWorkerProviderHost> provider_host); On 2015/10/01 23:11:12, michaeln wrote: > passing a scoped_ptr here is misleading because the core does not actually take > ownership of the object, it will never delete the object on its own I changed the implementation to use a linked_ptr in the map rather than raw pointer. linked_ptr is essentially a scoped_ptr usable with STL. So now the core does have ownership and will delete the object on destruction. WDYT? There are DCHECK in place in the linked_ptr implementation to ensure release() is called by the one and only owner of the object. https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:58: static int kVirtualProcessIDForBrowserRequest; On 2015/10/01 23:11:12, michaeln wrote: > please move this const to service_worker_types.h and really make it a const Done. https://codereview.chromium.org/1294243004/diff/260001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:57: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/01 23:11:12, michaeln wrote: > plz vett ipc params higher up in the stack Done. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2579: bool browser_side_navigation = On 2015/10/01 23:11:12, michaeln wrote: > this block of code is still tricky to follow and easily error prone? Refactored a bit. WDYT? https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2582: if ((browser_side_navigation && content_initiated) || On 2015/10/01 23:11:12, michaeln wrote: > We end up w/o having created a ServiceWorkerNetworkProvider in this case which i > think may be a bug, i think is current sources we always return from this method > with a non-null provider. We should retain that behavior. In PlzNavigate, this methods ends up being called twice. The first time, we haven't received the RequestNavigationParams yet so we cannot initialize anything even if we wanted to. That's why I'm testing for content_initiated. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2584: ServiceWorkerNetworkProvider::FromDocumentState( On 2015/10/01 23:11:12, michaeln wrote: > i think u could put this up top (at line) as an early return condition > independent of plznav mode. if a provider exists we don't want to create a new > one <period>. I am not 100% sure about that since we end up creating a new SWPH for renderer-initiated navigations. Do we need to take care of cleaning that up somewhere when a renderer intializes a new navigation? Either way, I changed that in the next patch. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2601: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; On 2015/10/01 23:11:12, michaeln wrote: > The provider_type is only used by the ServiceWorkerNetworkProvider class to > possibly compute a kInvalid id. Since this class now has so much logic dedicated > to dealing with that id value, for the plznav case, lets hoist all of the logic > into this class and keep the ServiceWorkerNetworkProvider ctor simple. > > ServiceWorkerNetworkProvider(route_id, provider_id) { > : provider_id_(provider_id) { > if (provider_id_ == kInvalid) > return; > context_ = new ServiceWorkerProviderContext(provider_id_); > if (!ChildThreadImpl::current()) > return; > ChildThreadImpl::current()->Send(...);l > } > > And retain the existing ctor for use by non-plz nav case. I think it can be > implemented in terms of the other. > > ServiceWorkerNetworkProvider(route_id, provider_type) { > : ServiceWorkerNetworkProvider(route_id, GenerateIdForType(type)) {} > Thanks, I moved the logic up here. Done. https://codereview.chromium.org/1294243004/diff/260001/content/renderer/rende... content/renderer/render_frame_impl.cc:2604: if (!browser_side_navigation) { On 2015/10/01 23:11:12, michaeln wrote: > this could be an else clause ... Yes. I feel silly.
I think this time it was much easier to follow. Good job! A bunch of mostly nits and suggestions. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:53: // Picking the partition based on the URL is incorrect. Is this supposed to be a TODO? nit: Empty line before the comment. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (left): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:145: const bool is_main_frame, Why did we lose the const here? https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:186: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; Can you add a comment on this member? https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:62: // Returns kInvalidServiceWorkerProviderId if sandbox_flags indicates a nit: Reflow this comment as a single sentence: Returns the next ServiceWorkerProviderHost ID for navigations, kInvalidServiceWorkerProviderId if the frame is sandboxed. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:67: if (sandboxed_frame) Why use a full boolean variable instead of checking the condition directly here? https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:155: false); // should_clear_history_list nit: align this comment vertically with the rest above. https://codereview.chromium.org/1294243004/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2091: ChromeBlobStorageContext* blob_context = This variable now will shadow the one on line 2028. Let's use different name, especially since they seem to be different types. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:283: // Transfer ownership to a linked_ptr. nit: Redundant comment. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:285: DCHECK(!ContainsKey(navigation_provider_host_map_, host_link->provider_id())); Why DCHECK on ContainsKey instead of using the insert method on the map and check the return value? This way we avoid doing two searches in the map, even in debug builds. Example of what I mean: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:296: // Transfer ownership to a scoped_ptr. nit: Another redundant comment. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:171: // Noop if the ProviderHost with id |provider_id| is not present. nit: This comment is an implementation detail that isn't relevant to its callers. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:546: DCHECK_NE(MSG_ROUTING_NONE, frame_id); Routing id or FTN id? https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:209: int route_id, What object is this routing id for? https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:301: int frame_id, nit: frame_routing_id? https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:50: int service_worker_provider_id) nit: parameter name mismatch between .cc and .h https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... File content/child/service_worker/service_worker_network_provider.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... content/child/service_worker/service_worker_network_provider.h:41: // service_worker_provider_id is initialized by the browser for navigations. service_worker_provider_id is not to be found anywhere else in this file. Can you update the comment? https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... File content/common/navigation_params.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... content/common/navigation_params.cc:65: request_context_type(REQUEST_CONTEXT_TYPE_HYPERLINK) {} Did we confirm the current code uses hyperlink or location? https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... content/common/navigation_params.h:273: // This is unused for current navigation. Can you clarify what is meant by "current"? Is it PlzNavigation code vs existing navigation code? Or is it current vs pending navigation? https://codereview.chromium.org/1294243004/diff/300001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/public/browser... content/public/browser/navigation_handle.h:100: RenderFrameHost* render_frame_host); Does it make sense to separate this test change into a new CL to make this one smaller? https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2591: return; nit: Empty line after the return and before the next if statement. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2592: if (browser_side_navigation && content_initiated) Can you include a comment why we are returning here. It isn't completely intuitive. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2606: // The browser sets service_worker_provider_id to -1 for sandboxed frames. Let's use the symbolic name, not the constant -1. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2610: DCHECK(ServiceWorkerUtils::IsBrowserAssignedProviderId( More than one line body requires {}. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:4942: DCHECK_EQ(FETCH_REQUEST_MODE_SAME_ORIGIN, Let's put a comment above all of the DCHECKs to explain why we are performing them - aka we assume those values on the browser side, so they better match here.
Thanks! This is getting a closer. A few comments in addition to the ones on the other patchset. Also, if you get a chance to upload a rebase on the other patch, that would make the review easier. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:183: // This is unused for current navigation. nit: rephrase as "This parameter is not used in the current navigation architecture, where it will always be kInvalidServiceWorkerProviderId." https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:186: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Can you add a comment on this member? I'd rather you store the FrameTreeNode (see comments in the other patchset). We will need to store it anyway, since we eventually want to expose its id to the WebNavigationAPI. This way, you don't have to store this member, since you can get the BrowserContext at anytime. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.h:167: // Note: |common_params_| and |begin_params_| are not const as they can be Update this comment to explain why the request params are not const as well. https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... content/common/navigation_params.h:273: // This is unused for current navigation. On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Can you clarify what is meant by "current"? Is it PlzNavigation code vs existing > navigation code? Or is it current vs pending navigation? +1. Something along the lines of "This parameter is not used in the current navigation architecture, where it will always be equal to kInvalidServiceWorkerProviderId." is a bit clearer.
Thanks! Rebased, cleaned up and fixed comments following the 2 spin off patches. PTAL https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:53: // Picking the partition based on the URL is incorrect. On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Is this supposed to be a TODO? > > nit: Empty line before the comment. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (left): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:145: const bool is_main_frame, On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Why did we lose the const here? It was inconsistent. This has been removed in the other patch anyway. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:183: // This is unused for current navigation. On 2015/10/05 12:18:16, clamy wrote: > nit: rephrase as "This parameter is not used in the current navigation > architecture, where it will always be kInvalidServiceWorkerProviderId." Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:186: scoped_refptr<ServiceWorkerContextWrapper> service_worker_context_; On 2015/10/05 12:18:16, clamy wrote: > On 2015/10/02 22:09:45, nasko (slow to review) wrote: > > Can you add a comment on this member? > > I'd rather you store the FrameTreeNode (see comments in the other patchset). We > will need to store it anyway, since we eventually want to expose its id to the > WebNavigationAPI. This way, you don't have to store this member, since you can > get the BrowserContext at anytime. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:62: // Returns kInvalidServiceWorkerProviderId if sandbox_flags indicates a On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: Reflow this comment as a single sentence: Returns the next > ServiceWorkerProviderHost ID for navigations, kInvalidServiceWorkerProviderId if > the frame is sandboxed. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:67: if (sandboxed_frame) On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Why use a full boolean variable instead of checking the condition directly here? Thought it was a bit clearer but I changed it. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:155: false); // should_clear_history_list On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: align this comment vertically with the rest above. Done in the other patch. https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... File content/browser/frame_host/navigation_request.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/frame_... content/browser/frame_host/navigation_request.h:167: // Note: |common_params_| and |begin_params_| are not const as they can be On 2015/10/05 12:18:16, clamy wrote: > Update this comment to explain why the request params are not const as well. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2091: ChromeBlobStorageContext* blob_context = On 2015/10/02 22:09:45, nasko (slow to review) wrote: > This variable now will shadow the one on line 2028. Let's use different name, > especially since they seem to be different types. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:285: DCHECK(!ContainsKey(navigation_provider_host_map_, host_link->provider_id())); On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Why DCHECK on ContainsKey instead of using the insert method on the map and > check the return value? This way we avoid doing two searches in the map, even in > debug builds. Example of what I mean: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r... I thought insert would replace the content but it turns out that no. Thanks! https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.cc:296: // Transfer ownership to a scoped_ptr. On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: Another redundant comment. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:171: // Noop if the ProviderHost with id |provider_id| is not present. On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: This comment is an implementation detail that isn't relevant to its > callers. Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:546: DCHECK_NE(MSG_ROUTING_NONE, frame_id); On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Routing id or FTN id? Done. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:209: int route_id, On 2015/10/02 22:09:45, nasko (slow to review) wrote: > What object is this routing id for? Changed it to frame_routing_id but the other methods here use frame_id. https://codereview.chromium.org/1294243004/diff/300001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.h:301: int frame_id, On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: frame_routing_id? Done. https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:50: int service_worker_provider_id) On 2015/10/02 22:09:45, nasko (slow to review) wrote: > nit: parameter name mismatch between .cc and .h Done. https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... File content/child/service_worker/service_worker_network_provider.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/child/service_... content/child/service_worker/service_worker_network_provider.h:41: // service_worker_provider_id is initialized by the browser for navigations. On 2015/10/02 22:09:45, nasko (slow to review) wrote: > service_worker_provider_id is not to be found anywhere else in this file. Can > you update the comment? Done. https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... File content/common/navigation_params.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... content/common/navigation_params.cc:65: request_context_type(REQUEST_CONTEXT_TYPE_HYPERLINK) {} On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Did we confirm the current code uses hyperlink or location? Done in the other patch. https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... File content/common/navigation_params.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/common/navigat... content/common/navigation_params.h:273: // This is unused for current navigation. On 2015/10/05 12:18:17, clamy wrote: > On 2015/10/02 22:09:45, nasko (slow to review) wrote: > > Can you clarify what is meant by "current"? Is it PlzNavigation code vs > existing > > navigation code? Or is it current vs pending navigation? > > +1. Something along the lines of "This parameter is not used in the current > navigation architecture, where it will always be equal to > kInvalidServiceWorkerProviderId." is a bit clearer. Done in the other patch. https://codereview.chromium.org/1294243004/diff/300001/content/public/browser... File content/public/browser/navigation_handle.h (right): https://codereview.chromium.org/1294243004/diff/300001/content/public/browser... content/public/browser/navigation_handle.h:100: RenderFrameHost* render_frame_host); On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Does it make sense to separate this test change into a new CL to make this one > smaller? Done. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2591: return; On 2015/10/02 22:09:46, nasko (slow to review) wrote: > nit: Empty line after the return and before the next if statement. Done. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2592: if (browser_side_navigation && content_initiated) On 2015/10/02 22:09:46, nasko (slow to review) wrote: > Can you include a comment why we are returning here. It isn't completely > intuitive. Done. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2606: // The browser sets service_worker_provider_id to -1 for sandboxed frames. On 2015/10/02 22:09:46, nasko (slow to review) wrote: > Let's use the symbolic name, not the constant -1. Done. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:2610: DCHECK(ServiceWorkerUtils::IsBrowserAssignedProviderId( On 2015/10/02 22:09:45, nasko (slow to review) wrote: > More than one line body requires {}. Done. https://codereview.chromium.org/1294243004/diff/300001/content/renderer/rende... content/renderer/render_frame_impl.cc:4942: DCHECK_EQ(FETCH_REQUEST_MODE_SAME_ORIGIN, On 2015/10/02 22:09:45, nasko (slow to review) wrote: > Let's put a comment above all of the DCHECKs to explain why we are performing > them - aka we assume those values on the browser side, so they better match > here. Done.
I think the logic is in pretty good shape now. Somebody on the serviceworker team proper should probably take a final look too. I know i've said it already a few times, but might as well say it again. I do think a 'handle' class to encapsulate the details of the pre-created host would be an improvement and would avoid the need for the odd RemoveNavProviderHost method on the ContextWrapper class. I don't understand the rationale behind the CHECKs in the browser process? What about testing? The changes in ContextCore and RequestHandler look unit testable. And the nav specific stuff probably could be tested in the context of RenderViewImplTest's. https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... File content/browser/frame_host/navigator_delegate.h (right): https://codereview.chromium.org/1294243004/diff/260001/content/browser/frame_... content/browser/frame_host/navigator_delegate.h:136: virtual BrowserContext* GetBrowserContext() const; On 2015/10/02 10:34:44, clamy wrote: > On 2015/10/01 23:11:12, michaeln wrote: > > The NavigationController interface has a GetBrowserContext() method and the > > Navigator has a navigation_controller_ data member. We should probably use > that > > to 'navigate' up the the browser context object. > > > > Just curious. I see FrameTreeNodes have a reference to a Navigator. What is > the > > scope of a single Navigator(Impl) instance? How many nodes does a > NavigatorImpl > > servce concurrently and over its lifetime? From the looks of the > > NavigationMetricsData, it looks like it could be a 1 to 1 relationship? > > > > There is one NavigatorImpl per WebContents, and its lifetime is the lifetime of > the WebContents (same for the NavigationController). It services all the FTN of > the FrameTree. Thnx for the explanation, the NavigationMetricsData threw me because it obviously only applies to a single navigation. I see now that data only applies to a navigation of the topmost frame. > In this case, getting the BrowserContext through the > NavigationController is the right thing to do. In order to get to the > NavigationController, we should just keep a pointer to the FTN in the > NavigationHandle (a raw pointer is safe). In fact, it can replace the delegate > (since we can also get the Navigator and it's delegate from the FTN). https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:52: switches::kEnableBrowserSideNavigation) && nit: would we ever expect a valid id value when the switch is not set? i think we probably only need to test if != kinvalid here and if so DCHECK the flag is set https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:234: navigation_handle_->set_service_worker_provider_id( Nice, deferring this swspecific init work until this point is definitely an improvement. This could be a great place to create a wrapper class that encapsulates the serviceworker specific bits of what happens on the browser-side and to take ownership of it. https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2093: chrome_blob_context->context()->GetBlobDataFromPublicURL( you could directly use the existing local here blob_context->>GetBlobDataFromPublicURL() https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:165: void AddNavigationProviderHost( Calling Add() and then never calling Take() or Rmv() should be considered a leak but typical leak detection won't identify it as such. Another plug for wrapper class :) https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:631: context_core_->RemoveNavigationProviderHost(provider_id); contenxt_core_ is a weakptr, so early return if null https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:531: FinalizeInitialization(new_process_id, new_frame_id, new_dispatcher_host); thnx for factoring out the common code https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:540: switches::kEnableBrowserSideNavigation)); I think this idea with the CHECK is to defend against bad ipcs, if that's right, please move to the OnProviderCreated method and convert it to a BadMessageReceived() call. Otherwise having a CHECK here provides a vector for a compromised renderer to crash the entire browser. I don't think having vectors like that is a desireable so I don't understand the motivation for using CHECKs in this fashion? https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:103: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( Is this to defend against a bad ipc? https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:66: GenerateProviderIdForType(provider_type)) {} thank you for simplifying these ctors https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... File content/child/service_worker/service_worker_network_provider.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... content/child/service_worker/service_worker_network_provider.h:44: int browse_provider_id); might want a 3rd empty ctor for the noop case ServiceWorkerNetworkProvider() : provider_id_(kInvalidXxxxx) {} https://codereview.chromium.org/1294243004/diff/360001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/common/service... content/common/service_worker/service_worker_types.h:40: static const int kVirtualProcessIDForBrowserRequest = -2; We should probably use ChildProcessHost::kInvalidUniqueID for this purpose instead of defining new const with a value of -2. Otherwise we're relying on -2 being an invalid value for a child process host id, i'm not sure if that always holds true? https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2586: // exists). please keep this comment about when/why a provider would already exist, its the sort of hidden behavior that should be documented https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2596: if (browser_side_navigation && content_initiated) There are a serveral callsites where a non-null provider is expected for a frame's datasource. I think we should maintain that invariant instead of fixing the callsites to allow otherwise. You mentioned elsewhere that this is called twice for plznav and the first time thru we don't have the RequestNavigationParams. If i understand properly, those two calls are for two different WebDataSource instances. The first instance is the provisional data source that's created (and deleted) prior to FrameHostMsg_BeginNavigation being sent to the browser process. The second instance is created much later, when the renderer is processing the FrameMsg_CommitNavigation message. If thats right, for the 'first' instances, lets create and attach an empty provider, new ServiceWorkerNetworkProvider() so its reliably non-null. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2613: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; we could use the empty ctor here too https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:3381: if (provider) provided the existing invariant is maintained, this change isn't needed https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:3388: if (provider) { ditto
Thanks! Lgtm with nits. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:58: // TODO: Picking the partition based on the URL is incorrect. nit: name in TODO. You can put mine if you want. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:179: // This parameter is not used in the current navigationa architecture, where nit: s/navigationa/navigation https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... content/browser/loader/navigation_url_loader_impl.cc:27: // TODO: Picking the partition based on the URL is incorrect. nit: name in TODO. You can put mine if you want.
Thanks! I added comments to clarify what happens with navigation ServiceWorkerProviderHosts. I changed the render_frame_impl logic again too. Regarding extra tests, this patch fixes 19 content_browsertests and 7 browser_tests in PlzNavigate so I think we have good test coverage already. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:52: switches::kEnableBrowserSideNavigation) && On 2015/10/07 01:16:44, michaeln wrote: > nit: would we ever expect a valid id value when the switch is not set? i think > we probably only need to test if != kinvalid here and if so DCHECK the flag is > set Sounds good. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.cc:58: // TODO: Picking the partition based on the URL is incorrect. On 2015/10/07 07:53:30, clamy wrote: > nit: name in TODO. You can put mine if you want. Done. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:179: // This parameter is not used in the current navigationa architecture, where On 2015/10/07 07:53:30, clamy wrote: > nit: s/navigationa/navigation Donea. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:234: navigation_handle_->set_service_worker_provider_id( On 2015/10/07 01:16:44, michaeln wrote: > Nice, deferring this swspecific init work until this point is definitely an > improvement. > > This could be a great place to create a wrapper class that encapsulates the > serviceworker specific bits of what happens on the browser-side and to take > ownership of it. One of the issues is that we are currently on the IO thread and the dtor for this new class will be called on the UI thread. We'd be passing a whole lot of information to a new class while adding little in the way of clarification since the dtor for the SWPH would end being called from another file anyway. However, I can add comments to clarify the lifetime of the SWPH for navigations at creation and destruction time and who will be responsible for that. https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... File content/browser/loader/navigation_url_loader_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... content/browser/loader/navigation_url_loader_impl.cc:27: // TODO: Picking the partition based on the URL is incorrect. On 2015/10/07 07:53:30, clamy wrote: > nit: name in TODO. You can put mine if you want. Thanks for volunteering! https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2093: chrome_blob_context->context()->GetBlobDataFromPublicURL( On 2015/10/07 01:16:44, michaeln wrote: > you could directly use the existing local here > blob_context->>GetBlobDataFromPublicURL() It seems like we should have been using the same thing in the first place anyway. I changed it. https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_context_core.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_context_core.h:165: void AddNavigationProviderHost( On 2015/10/07 01:16:44, michaeln wrote: > Calling Add() and then never calling Take() or Rmv() should be considered a leak > but typical leak detection won't identify it as such. Another plug for wrapper > class :) I added comments explaining where and why the Take() or Remove() call happen. https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:631: context_core_->RemoveNavigationProviderHost(provider_id); On 2015/10/07 01:16:44, michaeln wrote: > contenxt_core_ is a weakptr, so early return if null Done. https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:531: FinalizeInitialization(new_process_id, new_frame_id, new_dispatcher_host); On 2015/10/07 01:16:44, michaeln wrote: > thnx for factoring out the common code Welcome! https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:540: switches::kEnableBrowserSideNavigation)); On 2015/10/07 01:16:44, michaeln wrote: > I think this idea with the CHECK is to defend against bad ipcs, if that's right, > please move to the OnProviderCreated method and convert it to a > BadMessageReceived() call. Otherwise having a CHECK here provides a vector for a > compromised renderer to crash the entire browser. I don't think having vectors > like that is a desireable so I don't understand the motivation for using CHECKs > in this fashion? BadMessageReceived is already in the caller function. If we manage to still get in this method, this means that somehow, the browser created a corresponding SWPH and has been compromised. Or worse, there is a stack corruption. This is really more of a sanity check. But it also ensures that no one will attempt to reuse that method for current navigation architecture in the future. Or would at least think twice about calling it. https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_request_handler.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_request_handler.cc:103: CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/07 01:16:44, michaeln wrote: > Is this to defend against a bad ipc? No, this is another sanity check. The caller already has a similar check. This also prevents people from getting the idea to use this method for current navigation. https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... File content/child/service_worker/service_worker_network_provider.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... content/child/service_worker/service_worker_network_provider.cc:66: GenerateProviderIdForType(provider_type)) {} On 2015/10/07 01:16:44, michaeln wrote: > thank you for simplifying these ctors Welcome! https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... File content/child/service_worker/service_worker_network_provider.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/child/service_... content/child/service_worker/service_worker_network_provider.h:44: int browse_provider_id); On 2015/10/07 01:16:44, michaeln wrote: > might want a 3rd empty ctor for the noop case > > ServiceWorkerNetworkProvider() : provider_id_(kInvalidXxxxx) {} Done. https://codereview.chromium.org/1294243004/diff/360001/content/common/service... File content/common/service_worker/service_worker_types.h (right): https://codereview.chromium.org/1294243004/diff/360001/content/common/service... content/common/service_worker/service_worker_types.h:40: static const int kVirtualProcessIDForBrowserRequest = -2; On 2015/10/07 01:16:44, michaeln wrote: > We should probably use ChildProcessHost::kInvalidUniqueID for this purpose > instead of defining new const with a value of -2. Otherwise we're relying on -2 > being an invalid value for a child process host id, i'm not sure if that always > holds true? child process host ids cannot be negative, afaik. I changed it as it does not make as much sense as it used to in previous patches to use a different value. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (left): https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2586: // exists). On 2015/10/07 01:16:44, michaeln wrote: > please keep this comment about when/why a provider would already exist, its the > sort of hidden behavior that should be documented Done. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2596: if (browser_side_navigation && content_initiated) On 2015/10/07 01:16:44, michaeln wrote: > There are a serveral callsites where a non-null provider is expected for a > frame's datasource. I think we should maintain that invariant instead of fixing > the callsites to allow otherwise. > > You mentioned elsewhere that this is called twice for plznav and the first time > thru we don't have the RequestNavigationParams. If i understand properly, those > two calls are for two different WebDataSource instances. The first instance is > the provisional data source that's created (and deleted) prior to > FrameHostMsg_BeginNavigation being sent to the browser process. The second > instance is created much later, when the renderer is processing the > FrameMsg_CommitNavigation message. > > If thats right, for the 'first' instances, lets create and attach an empty > provider, new ServiceWorkerNetworkProvider() so its reliably non-null. Done. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:2613: provider_type = SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME; On 2015/10/07 01:16:44, michaeln wrote: > we could use the empty ctor here too Done. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:3381: if (provider) On 2015/10/07 01:16:44, michaeln wrote: > provided the existing invariant is maintained, this change isn't needed I DCHECK'd it instead. SIGSEGV crashes tend to not be very verbose. https://codereview.chromium.org/1294243004/diff/360001/content/renderer/rende... content/renderer/render_frame_impl.cc:3388: if (provider) { On 2015/10/07 01:16:44, michaeln wrote: > ditto Done.
michaeln@chromium.org changed required reviewers: + kinuko@chromium.org
Someobody from the core sw team should give the stamp of approval and whether they'd like to see additional tests or a crisper interface between the navsystem and swsystems and if we should commit as is or have somebody else pick it up or follow up? I put a star by kinuko's name for that. I feel like i'm playing bad cop :( Sorry for playing bad cop :( Thanx Fabrice for getting the CL into really nice shape. > Regarding extra tests, this patch fixes 19 content_browsertests and 7 > browser_tests in PlzNavigate so I think we have good test coverage already. None of them actually verify the lifetime and ownshership xfer logic works since an effective leak would not actually be detected as a leak. Unittests are much better suited to that than system tests. I can appreciate needing to be done with the CL and to move on to your new gig and that being the reason to punt tests. https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:234: navigation_handle_->set_service_worker_provider_id( On 2015/10/07 12:59:43, Fabrice wrote: > On 2015/10/07 01:16:44, michaeln wrote: > > Nice, deferring this swspecific init work until this point is definitely an > > improvement. > > > > This could be a great place to create a wrapper class that encapsulates the > > serviceworker specific bits of what happens on the browser-side and to take > > ownership of it. > > One of the issues is that we are currently on the IO thread and the dtor for > this new class will be called on the UI thread. We'd be passing a whole lot of > information to a new class while adding little in the way of clarification since > the dtor for the SWPH would end being called from another file anyway. > However, I can add comments to clarify the lifetime of the SWPH for navigations > at creation and destruction time and who will be responsible for that. Hmmm, but thats not right? First, this method runs on the UI thread (right?). And the only input needed for the wrapper class would be the ServiceWorkerContextWrapper and maybe the sandbox_flags. Here's a public interface that I think covers it. And the NavigationHandle having a scoped_ptr<> to one of these would actually clarify ownership at a glance. class ServiceWorkerNavigationHandle { public: ServiceWorkerNavigationHandle(context_wrapper, sandbox_flags); ~ServiceWorkerNavigationHandle(); int provider_host_id(); ServiceWorkerContextWrapper* context_wrapper(); }; Internally that class would * produce the magic id value * construct the host object and initialize it properly on the io thread * conditionally destruct the host if needed It might be more little complicated to put that class together than it is to have the logic be diffuse as in the existing CL. I understand you want to be done with the CL. https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... File content/browser/service_worker/service_worker_provider_host.cc (right): https://codereview.chromium.org/1294243004/diff/360001/content/browser/servic... content/browser/service_worker/service_worker_provider_host.cc:540: switches::kEnableBrowserSideNavigation)); > BadMessageReceived is already in the caller function. If we manage to still get > in this method, this means that somehow, the browser created a corresponding > SWPH and has been compromised. Or worse, there is a stack corruption. > This is really more of a sanity check. But it also ensures that no one will > attempt to reuse that method for current navigation architecture in the future. > Or would at least think twice about calling it. 1. To defend against memory corruption or a compromised browser 2. For documentation purposes. Thnx for the explanation. In that case, I think you should switch these to DCHECKs, at least for those in the /service_worker directory. The DCHECK covers the latter case. We don't use CHECKs as sanity checks because doing so pervasively would add code bloat without adding real value. Search the content/browser codebase and you'll see there aren't very many of them, espcecially if you look only in the service_worker subdirectory. https://codereview.chromium.org/1294243004/diff/380001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/1294243004/diff/380001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:631: if (context_core_) nit: for consistency, it'd be good to phrase this as an early return, if (!context_core_) return. There are a couple dozen existing weakptr tests of this form and none of the other. https://codereview.chromium.org/1294243004/diff/380001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1294243004/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:2597: if (browser_side_navigation && content_initiated) { thnx for restructuring this code, the different cases are a lot easier to follow now https://codereview.chromium.org/1294243004/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:2599: // Initialize an empty ServiceWorkerNetworkProvider. nit: this comment isn't needed, it states the obvious A comment that explains how there are two calls to this methods for two different objects, one prior to sending the message requesting the browser to navigate, and another after when committing the navigation, would help. https://codereview.chromium.org/1294243004/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:2604: // Retrieve the service_worker_provider_id from the RequestNavigationParams. nit: also stating the obvious https://codereview.chromium.org/1294243004/diff/380001/content/renderer/rende... content/renderer/render_frame_impl.cc:2628: routing_id_, SERVICE_WORKER_PROVIDER_FOR_SANDBOXED_FRAME)); nit: could use the empty ctor here too so its clear that what you get on line 2164 matches what you get here
Was just chatting with kinuko about this one. I'll pick it up and dust it off and re'up it. > Someobody from the core sw team should give the stamp of approval and whether > they'd like to see additional tests or a crisper interface between the navsystem > and swsystems and if we should commit as is or have somebody else pick it up or > follow up? I put a star by kinuko's name for that.
Actually I'm working on an updated version of this patch. I uncovered an issue in the latest patchset which caused ~400 content_browsertests to fail with PlzNavigate enabled. This prompted me to redesign some of the patch and I should have something up tonight.
And I have a new CL up at https://codereview.chromium.org/1399363004/.
On 2015/10/14 14:08:52, clamy wrote: > And I have a new CL up at https://codereview.chromium.org/1399363004/. fantastique, merci beaucoup! |
