Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2575523002:
Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch (Closed)
DescriptionPrerender: Confirm ServiceWorkers are invoked for NoState Prefetch.
Confirm that a SW is invoked for files loaded from NoState prefetch, even if a registered SW does not have a process.
The caveats discussed in crbug/465200 still hold regarding the origin for which the SW is called.
BUG=632368
Review-Url: https://codereview.chromium.org/2575523002
Cr-Commit-Position: refs/heads/master@{#442607}
Committed: https://chromium.googlesource.com/chromium/src/+/0aed258863772ab7ba983993a2e1ce934d1caf66
Patch Set 1 #Patch Set 2 : remove debugging code #
Total comments: 8
Patch Set 3 : check sw exist #
Total comments: 1
Patch Set 4 : comments #Patch Set 5 : drop prefetch url request #
Total comments: 1
Patch Set 6 : Check that SW runs rather than disabling #
Total comments: 21
Patch Set 7 : comments #
Total comments: 6
Patch Set 8 : comments #Patch Set 9 : comments #
Total comments: 13
Patch Set 10 : comments/big hammer #
Total comments: 2
Patch Set 11 : Kill SW via RenderProcessHostsb in test rather than through ServiceWorkerVersion #
Total comments: 6
Patch Set 12 : comments #
Messages
Total messages: 51 (8 generated)
mattcary@chromium.org changed reviewers: + droger@chromium.org, pasko@chromium.org
Here is the SW fix I proposed over the email thread. LMK.
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:343: void PrerenderContents::ContinuePrerenderIfNoServiceWorker( Naming nit (optional): there are cases where the prerender continues even if there is a service worker. I would prefer finding a better name. I don't really have good suggestions, but I think MaybeContinuePrerender() or even ContinuePrerender() would be ok. https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; Early return here means that the page will be considered as prefetched, even though it was not. This will impact our metrics for example. Could this code be moved in PrerenderManager::AddPrerender, specifically before the line: prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin);
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:343: void PrerenderContents::ContinuePrerenderIfNoServiceWorker( On 2016/12/13 12:43:01, droger wrote: > Naming nit (optional): there are cases where the prerender continues even if > there is a service worker. I would prefer finding a better name. I don't really > have good suggestions, but I think MaybeContinuePrerender() or even > ContinuePrerender() would be ok. That's a good point. The wrinkle is that this is a service worker-specific method so it can fit with the CheckHasServiceWorker callback, I wasn't sure what the convention was for a case like this (ie, making the context a function should be called in very clear in the name or not). https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; On 2016/12/13 12:43:01, droger wrote: > Early return here means that the page will be considered as prefetched, even > though it was not. This will impact our metrics for example. > > Could this code be moved in PrerenderManager::AddPrerender, specifically before > the line: > > prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin); Mmmm, good point. As far as I can tell, we can't get at the service worker until we have a web contents, which is created when PrerenderContents::StartPrerendering is invoked. Currently the PrerenderContents is created in the PrerenderManager after prefetches_ is updated. It seems cleanest to create-then-cancel, given everything that happens when the PrerenderContents is created. We could remove the URL from the prefetches_ list after we detect the service worker. Would that be enough? There is a race if another navigation happens in between.
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; On 2016/12/13 12:58:36, mattcary wrote: > On 2016/12/13 12:43:01, droger wrote: > > Early return here means that the page will be considered as prefetched, even > > though it was not. This will impact our metrics for example. > > > > Could this code be moved in PrerenderManager::AddPrerender, specifically > before > > the line: > > > > prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin); > > Mmmm, good point. > > As far as I can tell, we can't get at the service worker until we have a web > contents, which is created when PrerenderContents::StartPrerendering is invoked. > Currently the PrerenderContents is created in the PrerenderManager after > prefetches_ is updated. It seems cleanest to create-then-cancel, given > everything that happens when the PrerenderContents is created. > > We could remove the URL from the prefetches_ list after we detect the service > worker. Would that be enough? There is a race if another navigation happens in > between. I don't see any use of WebContents in the code you are adding, are you sure the WebContents is really needed? Otherwise I guess we can probably keep the code as is.
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; On 2016/12/13 13:25:48, droger wrote: > On 2016/12/13 12:58:36, mattcary wrote: > > On 2016/12/13 12:43:01, droger wrote: > > > Early return here means that the page will be considered as prefetched, even > > > though it was not. This will impact our metrics for example. > > > > > > Could this code be moved in PrerenderManager::AddPrerender, specifically > > before > > > the line: > > > > > > prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin); > > > > Mmmm, good point. > > > > As far as I can tell, we can't get at the service worker until we have a web > > contents, which is created when PrerenderContents::StartPrerendering is > invoked. > > Currently the PrerenderContents is created in the PrerenderManager after > > prefetches_ is updated. It seems cleanest to create-then-cancel, given > > everything that happens when the PrerenderContents is created. > > > > We could remove the URL from the prefetches_ list after we detect the service > > worker. Would that be enough? There is a race if another navigation happens in > > between. > > I don't see any use of WebContents in the code you are adding, are you sure the > WebContents is really needed? > > Otherwise I guess we can probably keep the code as is. prerender_contents_ is actually a WebContents (yay for naming! not my fault...) Anyway I just use it to get the StoragePartition. If there's a different way of getting that without the WebContents that would be better, it would allow us to possibly make this check before creating the PrerenderContents.
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; On 2016/12/13 13:30:50, mattcary wrote: > On 2016/12/13 13:25:48, droger wrote: > > On 2016/12/13 12:58:36, mattcary wrote: > > > On 2016/12/13 12:43:01, droger wrote: > > > > Early return here means that the page will be considered as prefetched, > even > > > > though it was not. This will impact our metrics for example. > > > > > > > > Could this code be moved in PrerenderManager::AddPrerender, specifically > > > before > > > > the line: > > > > > > > > prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin); > > > > > > Mmmm, good point. > > > > > > As far as I can tell, we can't get at the service worker until we have a web > > > contents, which is created when PrerenderContents::StartPrerendering is > > invoked. > > > Currently the PrerenderContents is created in the PrerenderManager after > > > prefetches_ is updated. It seems cleanest to create-then-cancel, given > > > everything that happens when the PrerenderContents is created. > > > > > > We could remove the URL from the prefetches_ list after we detect the > service > > > worker. Would that be enough? There is a race if another navigation happens > in > > > between. > > > > I don't see any use of WebContents in the code you are adding, are you sure > the > > WebContents is really needed? > > > > Otherwise I guess we can probably keep the code as is. > > prerender_contents_ is actually a WebContents (yay for naming! not my fault...) > > Anyway I just use it to get the StoragePartition. If there's a different way of > getting that without the WebContents that would be better, it would allow us to > possibly make this check before creating the PrerenderContents. prerender_contents_->GetBrowserContext() is just the profile, which we already have in PrerenderManager. And it seems the SiteInstance stuff can by bypassed using GetStoragePartitionForSite, which just takes a URL. I'm not familiar with this, but GetStoragePartition() calls GetStoragePartitionForSite() internally so that seems sensible.
Regardless of which implementation we chose: Note also that we're changing an assumption: we assumed that the set of prerenderable pages is a subset of prefetchable pages. This now no longer true, and possibly impacts how we interpret metrics as well.
https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2575523002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:347: return; On 2016/12/13 13:36:36, droger wrote: > On 2016/12/13 13:30:50, mattcary wrote: > > On 2016/12/13 13:25:48, droger wrote: > > > On 2016/12/13 12:58:36, mattcary wrote: > > > > On 2016/12/13 12:43:01, droger wrote: > > > > > Early return here means that the page will be considered as prefetched, > > even > > > > > though it was not. This will impact our metrics for example. > > > > > > > > > > Could this code be moved in PrerenderManager::AddPrerender, specifically > > > > before > > > > > the line: > > > > > > > > > > prefetches_.emplace_back(url, GetCurrentTimeTicks(), origin); > > > > > > > > Mmmm, good point. > > > > > > > > As far as I can tell, we can't get at the service worker until we have a > web > > > > contents, which is created when PrerenderContents::StartPrerendering is > > > invoked. > > > > Currently the PrerenderContents is created in the PrerenderManager after > > > > prefetches_ is updated. It seems cleanest to create-then-cancel, given > > > > everything that happens when the PrerenderContents is created. > > > > > > > > We could remove the URL from the prefetches_ list after we detect the > > service > > > > worker. Would that be enough? There is a race if another navigation > happens > > in > > > > between. > > > > > > I don't see any use of WebContents in the code you are adding, are you sure > > the > > > WebContents is really needed? > > > > > > Otherwise I guess we can probably keep the code as is. > > > > prerender_contents_ is actually a WebContents (yay for naming! not my > fault...) > > > > Anyway I just use it to get the StoragePartition. If there's a different way > of > > getting that without the WebContents that would be better, it would allow us > to > > possibly make this check before creating the PrerenderContents. > > prerender_contents_->GetBrowserContext() is just the profile, which we already > have in PrerenderManager. > > And it seems the SiteInstance stuff can by bypassed using > GetStoragePartitionForSite, which just takes a URL. I'm not familiar with this, > but GetStoragePartition() calls GetStoragePartitionForSite() internally so that > seems sensible. ah, good point, will look.
On 2016/12/13 13:38:55, droger wrote: > Regardless of which implementation we chose: > > Note also that we're changing an assumption: we assumed that the set of > prerenderable pages is a subset of prefetchable pages. This now no longer true, > and possibly impacts how we interpret metrics as well. Yes, that's true. That seems to be unavoidable. I don't think it's a good idea to turn of prerendering for SW, as there could be performance impact; on the other hand IIUC there is a correctness issue with prefetching a page with a SW?
https://codereview.chromium.org/2575523002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:527: IN_PROC_BROWSER_TEST_F(NoStatePrefetchBrowserTest, CheckNoSW) { This test here shows that a SW actually exists: I changed the service worker to rewrite the main page with an image url, and running that with this test shows that the preload scanner sees the png. So prefetching with a SW may actually be valid. There are probably corner cases, like if the SW process gets killed, that I'd have to look into to figure out how to check. So I wonder what we want to do. Maybe we don't kill nostate if there's a service worker?
On 2016/12/13 16:08:47, mattcary wrote: > So prefetching with a SW may actually be valid. There are probably corner cases, > like if the SW process gets killed, that I'd have to look into to figure out how > to check. > > So I wonder what we want to do. Maybe we don't kill nostate if there's a service > worker? One issue I can see here is that we did not communicate to devs that we could issue a fetch for SW, so their stats would skew. We could later make it more spec-ed by providing a hint, probably in a form of a request header. I would like to avoid being blocked on having this spec. Also, SW may have side effects besides cache, which we did not review carefully. Why is it a problem to report FINAL_STATUS_NOSTATE_PREFETCH_FINISHED in case there is SW? After all, it was a prefetch, just not a very particularly clever one. There seem to be these cases: 1. SW for the future pageload exists before we spawn the renderer => cancel early without starting a renderer 2. The new renderer will instantiate a SW as part of itself => check in the SW renderer-side impl whether it is SetIsPrerendering-flavored and return early 3. possibly racy: starting a renderer does not expose a SW, but some concurrent navigation can instantiate a SW for the same domain in a different process (need SW experts to help?)
> One issue I can see here is that we did not communicate to devs that we could > issue a fetch for SW, so their stats would skew. We could later make it more > spec-ed by providing a hint, probably in a form of a request header. I would > like to avoid being blocked on having this spec. Also, SW may have side effects > besides cache, which we did not review carefully. > Yes, that's true. A counterpoint is that prerender continues if there is a service worker (IIUC). I'm not disagreeing with you, just mentioning that the choice is not clear. I'm fine going with the safe way and canceling. > Why is it a problem to report FINAL_STATUS_NOSTATE_PREFETCH_FINISHED in case > there is SW? After all, it was a prefetch, just not a very particularly clever > one. > This way we get stats on how frequently this happens. I don't think the status really matters for much else than the histograms? > There seem to be these cases: > 1. SW for the future pageload exists before we spawn the renderer => cancel > early without starting a renderer I'm a little unclear on when the renderer gets started, is it when the WebContents is created (in PrerenderContents::StartPrerendering)? At any rate, the reason why I wait until then to check for the SW is that was how I figured out how to get the StoragePartition and SiteInstance which tells if a SW exists. However, David pointed out that I don't need to wait that long, that I could get those two things earlier. In that case I'll be able to check for the SW earlier which I think will resolve this point. > 2. The new renderer will instantiate a SW as part of itself => check in the SW > renderer-side impl whether it is SetIsPrerendering-flavored and return early > 3. possibly racy: starting a renderer does not expose a SW, but some concurrent > navigation can instantiate a SW for the same domain in a different process (need > SW experts to help?) +1 on talking to SW experts. I believe that the SW check I do looks if one is registered, and so would cover case (2). For case (3), the same situation is a service worker is created in the middle of prefetching (some resources fetched pre-SW and some post-SW). That implies that we need to check for a SW on a ResourceRequest (or equiv) level. So the sketch of the flow would be: (a) check for SW when prefetch starts and cancel if it exists (b) on each resource fetch, if there is a SW and the fetch is for nostate-prefetch, cancel the fetch. An optional thing to do would be to cancel the preload scanner if a SW appears, although because of (b) that would be a performance and not correctness issue. WDYT?
Update: checking for the service worker before creating the Prerender/WebContents seems to be impractical and unnecessary. Impractical, because callers to AddPrerender* expect to have a PrerenderHandle containing a WebContents. In particular, for the PostMessage API, CCT puts an observer on the WebContents to watch for the DOM load at which point it opens up its connection. Putting in an indirection so that we defer creating the WebContents until after we check on a service worker is possible but more work than seems necessary, especially as we plan to move to background tabs for this use case. Unnecessary, because the renderer process isn't created until LoadURL happens, which is after the SW check in the current version of the patch. This does mean that when nostate is canceled due to a service worker, we will have inserted the url into prefetches_ and so potentially count a P-type load when no prefetching has actually been done. I think that's the correct thing to do, though, as we would want to compare with a corresponding prerender that actually went through the service worker correctly.
Thanks, Matt, great investigation! On 2016/12/19 13:31:53, mattcary wrote: > Update: checking for the service worker before creating the > Prerender/WebContents seems to be impractical and unnecessary. > > Impractical, because callers to AddPrerender* expect to have a PrerenderHandle > containing a WebContents. In particular, for the PostMessage API, CCT puts an > observer on the WebContents to watch for the DOM load at which point it opens up > its connection. Putting in an indirection so that we defer creating the > WebContents until after we check on a service worker is possible but more work > than seems necessary, especially as we plan to move to background tabs for this > use case. Totally agreed, creating WebContents for prerender synchronously on UI thread is required. > Unnecessary, because the renderer process isn't created until LoadURL happens, > which is after the SW check in the current version of the patch. This does mean > that when nostate is canceled due to a service worker, we will have inserted the > url into prefetches_ and so potentially count a P-type load when no prefetching > has actually been done. I think that's the correct thing to do, though, as we > would want to compare with a corresponding prerender that actually went through > the service worker correctly. Fingers crossed, we won't have to distinguish this case into the new category. My intuition is bad here, a few thoughts: 1. Most use of SW is probably from Facebook, where NoStatePrefetch is not really useful anyway 2. As one of possible later incremental improvements: learn to discard prefetch, while making sure the SW that starts would stay resident for some time (removes some bottlenecks, right?)
On 2016/12/16 09:00:23, mattcary wrote: > > 2. The new renderer will instantiate a SW as part of itself => check in the SW > > renderer-side impl whether it is SetIsPrerendering-flavored and return early > > 3. possibly racy: starting a renderer does not expose a SW, but some > concurrent > > navigation can instantiate a SW for the same domain in a different process > (need > > SW experts to help?) > +1 on talking to SW experts. > > I believe that the SW check I do looks if one is registered, and so would cover > case (2). For case (3), the same situation is a service worker is created in the > middle of prefetching (some resources fetched pre-SW and some post-SW). That > implies that we need to check for a SW on a ResourceRequest (or equiv) level. So > the sketch of the flow would be: > (a) check for SW when prefetch starts and cancel if it exists > (b) on each resource fetch, if there is a SW and the fetch is for > nostate-prefetch, cancel the fetch. This sounds really nice and simple, +1, thank you!
Description was changed from ========== Prerender: cancel no-state prefetch when a ServiceWorker exists. As a ServiceWorker instance may not exist when no-state prefetch starts loading, prefetch is canceled before beginning if a ServiceWorker is registered for a page. To do this requires adding a thread hop in the path of starting a prefetch, which unfortunately is unavoidable as the relevant ServiceWorker information lives on the IO thead. BUG=632368 ========== to ========== Prerender: cancel no-state prefetch when a ServiceWorker exists. As a ServiceWorker instance may not exist when no-state prefetch starts loading, prefetch is canceled before beginning if a ServiceWorker is registered for a page. To do this requires adding a thread hop in the path of starting a prefetch, which unfortunately is unavoidable as the relevant ServiceWorker information lives on the IO thead. To cover the case when a ServiceWorker is started after the no-state renderer has begun, we also drop all LOAD_PREFETCH requests when a service worker is involved. BUG=632368 ==========
mattcary@chromium.org changed reviewers: + clamy@chromium.org, falken@chromium.org, horo@chromium.org
+clamy, horo, falken Here is the patch related to my ServiceWorker and prefetch thread. The interesting thing here is the ServiceWorkerContextRequestHandler change. I think this is far too heavyweight, but am not sure of a better way to accomplish it. Any comments on whether this is the correct way to go, or even necessary, would be appreciated.
Sorry that I largely missed the email thread. Could you clarify the goal a bit more? I see that NoState prefetch is cancelled if a SW is detected. If NoState prefetch starts and later a SW starts, is the goal to cancel requests initiated by NoState prefetch? If so, I don't think returning null in MaybeCreateJob() does what we want. I think we need to do something like URLRequest::Cancel() or create a job that responds with error. https://codereview.chromium.org/2575523002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_context_request_handler.cc (right): https://codereview.chromium.org/2575523002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_context_request_handler.cc:61: return NULL; Returning null here just means that SW machinery will not intercept the request, and it will go through the usual resource loading/network stack. Also, ServiceWorkerContextRequestHandler is created for requests done from the SW which means either: - the SW calling fetch() - the request for the SW script itself to start the SW Since requests initiated by the SW calling fetch() are *not* intercepted by any SW, they go straight to the normal network stack, hence the return null in line 55. So ServiceWorkerContextRequestHandler only tries to intercept requests for the SW script to start the worker. In this case, if we return null here, the SW script load won't go through the expected path (which would reading or writing to the SW script storage), and will instead go through the network stack. The renderer will get the response for the script and start the worker anyway, which is an unexpected case. In fact I expect this would cause a CHECK later when the browser discovers a service worker is running without going through the expected load path (I think the other null returns past line 55 also would cause this CHECK, it's a long standing bug I'm trying to track down: https://bugs.chromium.org/p/chromium/issues/detail?id=485900. If the goal is to drop the job, I think we'd need to return a job that responds to the request with an error, or cancel the request. Also there is another request handler called ServiceWorkerControlleeRequestHandler, which is for intercepting requests from a "client" (typically a controlled page) that could be intercepted by the SW. I suspect if you want requests involving NoPrefetch to not be intercepted by a SW, you'll need code there too.
At a higher level, when is NoState prefetch triggered? Service worker controlled navigations are gaining their own prefetch-like feature called "navigation preload": https://bugs.chromium.org/p/chromium/issues/detail?id=649558&desc=2
On 2016/12/20 14:55:13, pasko wrote: > Thanks, Matt, great investigation! > > On 2016/12/19 13:31:53, mattcary wrote: > > Update: checking for the service worker before creating the > > Prerender/WebContents seems to be impractical and unnecessary. > > > > Impractical, because callers to AddPrerender* expect to have a PrerenderHandle > > containing a WebContents. In particular, for the PostMessage API, CCT puts an > > observer on the WebContents to watch for the DOM load at which point it opens > up > > its connection. Putting in an indirection so that we defer creating the > > WebContents until after we check on a service worker is possible but more work > > than seems necessary, especially as we plan to move to background tabs for > this > > use case. > > Totally agreed, creating WebContents for prerender synchronously on UI thread is > required. > > > Unnecessary, because the renderer process isn't created until LoadURL happens, > > which is after the SW check in the current version of the patch. This does > mean > > that when nostate is canceled due to a service worker, we will have inserted > the > > url into prefetches_ and so potentially count a P-type load when no > prefetching > > has actually been done. I think that's the correct thing to do, though, as we > > would want to compare with a corresponding prerender that actually went > through > > the service worker correctly. > > Fingers crossed, we won't have to distinguish this case into the new category. > My intuition is bad here, a few thoughts: > 1. Most use of SW is probably from Facebook, where NoStatePrefetch is not really > useful anyway FYI that's become coming less and less true over time. You can see some usage stats at https://uma.googleplex.com/rappor?day_count=90&metric=ServiceWorker.Controlle... (internal link)
On 2016/12/27 01:29:29, falken wrote: > At a higher level, when is NoState prefetch triggered? Service worker controlled > navigations are gaining their own prefetch-like feature called "navigation > preload": https://bugs.chromium.org/p/chromium/issues/detail?id=649558&desc=2 Thanks for the correction, returning a NULL job is definitely not what I want to do. I am very happy to discuss the higher-level question :) Initially, NoState prefetch is going to replace prerender and will be invoked in the same context. I believe it's true that SW handles requests from a prerender normally, so perhaps we actually want NoState prefetch to do the same thing? In that case this whole CL can be dropped. If we do want SW to handle NoState prefetch requests, the next question is if that's happening correctly. NoState uses a renderer process to perform preload scanning, and then the renderer finishes. IIUC, SWs will run in a different renderer process anyway, and they seem to function correctly (at least I've quickly verified that in a browser test context). Can you confirm that? Thx
On 2016/12/27 09:56:28, mattcary wrote: > On 2016/12/27 01:29:29, falken wrote: > > At a higher level, when is NoState prefetch triggered? Service worker > controlled > > navigations are gaining their own prefetch-like feature called "navigation > > preload": https://bugs.chromium.org/p/chromium/issues/detail?id=649558&desc=2 > > Thanks for the correction, returning a NULL job is definitely not what I want to > do. > > I am very happy to discuss the higher-level question :) > > Initially, NoState prefetch is going to replace prerender and will be invoked in > the same context. I believe it's true that SW handles requests from a prerender > normally, so perhaps we actually want NoState prefetch to do the same thing? In > that case this whole CL can be dropped. I'm not sure. Interaction with prefetch/prerender/preload is tracked at: https://bugs.chromium.org/p/chromium/issues/detail?id=465200 but there was never a strong understanding. From the bug comments it sounds like at least prefetch and preload should be working. I'll see if I can investigate more tomorrow before TOK goes on end of year holidays. > > If we do want SW to handle NoState prefetch requests, the next question is if > that's happening correctly. NoState uses a renderer process to perform preload > scanning, and then the renderer finishes. IIUC, SWs will run in a different > renderer process anyway, and they seem to function correctly (at least I've > quickly verified that in a browser test context). Can you confirm that? I can confirm that SWs can run in the same process as the page they control, or a different process. Basically when the browser tries to start a service worker, it looks through all render processes and picks one which matches the SW's origin for security, or else it creates a new render process.
On 2016/12/27 15:09:04, falken wrote: > On 2016/12/27 09:56:28, mattcary wrote: > > On 2016/12/27 01:29:29, falken wrote: > > > At a higher level, when is NoState prefetch triggered? Service worker > > controlled > > > navigations are gaining their own prefetch-like feature called "navigation > > > preload": > https://bugs.chromium.org/p/chromium/issues/detail?id=649558&desc=2 > > > > Thanks for the correction, returning a NULL job is definitely not what I want > to > > do. > > > > I am very happy to discuss the higher-level question :) > > > > Initially, NoState prefetch is going to replace prerender and will be invoked > in > > the same context. I believe it's true that SW handles requests from a > prerender > > normally, so perhaps we actually want NoState prefetch to do the same thing? > In > > that case this whole CL can be dropped. > > I'm not sure. Interaction with prefetch/prerender/preload is tracked at: > https://bugs.chromium.org/p/chromium/issues/detail?id=465200 > > but there was never a strong understanding. From the bug comments it sounds like > at least prefetch and preload should be working. I'll see if I can investigate > more tomorrow before TOK goes on end of year holidays. > > > > > If we do want SW to handle NoState prefetch requests, the next question is if > > that's happening correctly. NoState uses a renderer process to perform preload > > scanning, and then the renderer finishes. IIUC, SWs will run in a different > > renderer process anyway, and they seem to function correctly (at least I've > > quickly verified that in a browser test context). Can you confirm that? > > I can confirm that SWs can run in the same process as the page they control, or > a different process. Basically when the browser tries to start a service worker, > it looks through all render processes and picks one which matches the SW's > origin for security, or else it creates a new render process. Thanks! That's a really useful bug, or at least muddying the waters in a highly relevant way. I'm looking to see if there's anything in standards that sheds any light.
Description was changed from ========== Prerender: cancel no-state prefetch when a ServiceWorker exists. As a ServiceWorker instance may not exist when no-state prefetch starts loading, prefetch is canceled before beginning if a ServiceWorker is registered for a page. To do this requires adding a thread hop in the path of starting a prefetch, which unfortunately is unavoidable as the relevant ServiceWorker information lives on the IO thead. To cover the case when a ServiceWorker is started after the no-state renderer has begun, we also drop all LOAD_PREFETCH requests when a service worker is involved. BUG=632368 ========== to ========== Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch. Confirm that a SW is invoked for files loaded from NoState prefetch, even if a registered SW does not have a process. The caveats discussed in crbug/465200 still hold regarding the origin for which the SW is called. BUG=632368 ==========
After looking over the bug, it seems that things are working as intended currently. The discussion about what "intended" means is still evolving, but that's out of scope for our project. I've changed this CL to add a test to confirm that. Given that this involves tweaking the content public SW API this may be overkill and not necessary; LMK.
+slightlyoff FYI: With nostateprefetch [1] we are going to get the resources through SW 'fetch'. Same happens with Prerender, nothing getting worse here, since nostateprefetch is going to be triggered instead of prerender at the very same moments (at least for v1). In a comment [2] Alex mentioned massive sites moving accounting to SW, which got me slightly worried, worth an FYI .. I thought. Alex: can we avoid blocking nostateprefetch on proper SW speculative fetch API? Step 1 would be to migrate off prerender, step 2 to make sure SW distinguishes regular fetches from prefetches. [1] nostate prefetch proposal https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E... [2] https://github.com/w3c/ServiceWorker/issues/920#issuecomment-230120523 https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:38: namespace { nit: empty line after namespace for function declarations seems to be more consistent with the rest of the code. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:43: } // namespace nit: git grep -B 2 '} // namespace$' https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:531: // Register and launch a SW. Is it easy to add a similar test, but for the case when the SW is not registered yet? https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:543: src_server()->GetURL(MakeAbsolute(kPrefetchPng))) nit: hard to count brackets, formatting does not help. A convenient point to split I think would be 'storage_partition = '. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:547: base::Bind(&ClosureOnUiThread, run_loop.QuitClosure())); why not just run_loop.QuitClosure()? // i.e. without base::Bind and re-posting a task https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:551: // an <img> tage for kPrefetchPng. We verify that the SW ran correctly by s/We verify/Verify/ (we prefer to avoid 'we' in comments) https://codereview.chromium.org/2575523002/diff/100001/chrome/test/data/prere... File chrome/test/data/prerender/service_worker.js (right): https://codereview.chromium.org/2575523002/diff/100001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. nit: s/2012/2017/ https://codereview.chromium.org/2575523002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/2575523002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:500: StopAllServiceWorkersForOriginWithCallback(origin, base::Bind(&Noop)); base::DoNothing https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... content/public/browser/service_worker_context.h:128: // As above, but |callback| is exectued when a running worker is stopped. If nit: s/exectued/executed/ https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. My personal preference would be with _always_ invoking the callback, with status depending on whether there was a SW running. Not my call though, up to content owners.
https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:38: namespace { On 2017/01/02 18:15:36, pasko wrote: > nit: empty line after namespace for function declarations seems to be more > consistent with the rest of the code. Done. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:43: } // namespace On 2017/01/02 18:15:36, pasko wrote: > nit: git grep -B 2 '} // namespace$' Done. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:531: // Register and launch a SW. On 2017/01/02 18:15:36, pasko wrote: > Is it easy to add a similar test, but for the case when the SW is not registered > yet? Yes, in fact the other tests already do that (if prerender/service_worker.js, prerender/prefetch.js would never be loaded). https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:543: src_server()->GetURL(MakeAbsolute(kPrefetchPng))) On 2017/01/02 18:15:36, pasko wrote: > nit: hard to count brackets, formatting does not help. A convenient point to > split I think would be 'storage_partition = '. Done. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:547: base::Bind(&ClosureOnUiThread, run_loop.QuitClosure())); On 2017/01/02 18:15:36, pasko wrote: > why not just run_loop.QuitClosure()? // i.e. without base::Bind and re-posting a > task The SW callback fires from the IO thread. https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:551: // an <img> tage for kPrefetchPng. We verify that the SW ran correctly by On 2017/01/02 18:15:36, pasko wrote: > s/We verify/Verify/ > (we prefer to avoid 'we' in comments) Done, although git grep -i ' we ' | grep cc | head.
On 2017/01/02 18:15:36, pasko wrote: > +slightlyoff FYI: > > With nostateprefetch [1] we are going to get the resources through SW 'fetch'. > Same happens with Prerender, nothing getting worse here, since nostateprefetch > is going to be triggered instead of prerender at the very same moments (at least > for v1). > > In a comment [2] Alex mentioned massive sites moving accounting to SW, which got > me slightly worried, worth an FYI .. I thought. Alex: can we avoid blocking > nostateprefetch on proper SW speculative fetch API? Step 1 would be to migrate > off prerender, step 2 to make sure SW distinguishes regular fetches from > prefetches. > > [1] nostate prefetch proposal > > https://docs.google.com/document/d/16VCYGGWau483IMSxODpg5faZny1FJ6vNK2v-BuM5E... > > [2] https://github.com/w3c/ServiceWorker/issues/920#issuecomment-230120523 > It seems a little dangerous to have a SW sometimes intercept requests and sometimes not. However, presumably if the page is visited the SW will intercept the fetch as usual. So either the SW would intercept the request twice (once on prefetch and once on load), or we'd have one load over the wire (from prefetch) and one intercepted request (during the page load). At any rate probably either option will surprise some people some of the time...
https://codereview.chromium.org/2575523002/diff/100001/chrome/test/data/prere... File chrome/test/data/prerender/service_worker.js (right): https://codereview.chromium.org/2575523002/diff/100001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/02 18:15:36, pasko wrote: > nit: s/2012/2017/ Done. https://codereview.chromium.org/2575523002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.cc (right): https://codereview.chromium.org/2575523002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.cc:500: StopAllServiceWorkersForOriginWithCallback(origin, base::Bind(&Noop)); On 2017/01/02 18:15:36, pasko wrote: > base::DoNothing Ah, thanks, I was looking in callback_helpers.h, silly me. https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... content/public/browser/service_worker_context.h:128: // As above, but |callback| is exectued when a running worker is stopped. If On 2017/01/02 18:15:36, pasko wrote: > nit: s/exectued/executed/ Done. https://codereview.chromium.org/2575523002/diff/100001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. On 2017/01/02 18:15:36, pasko wrote: > My personal preference would be with _always_ invoking the callback, with status > depending on whether there was a SW running. > > Not my call though, up to content owners. Given its current use in a test these semantics work fine. Adding more information for speculative future use makes me nervous for several reasons: I was not really interested in widening the API by adding a public status enumeration (or exposing the existing internal status enum). Also, there seems to be a range of behavior depending on the state of ServiceWorkerVersion---exposing a status will probably necessarily leak the implementation (eg, a ServiceWorkerVersion that exists but which is already stopped, etc).
general approach and chrome/browser/prerender lgtm, thank you https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/100001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:547: base::Bind(&ClosureOnUiThread, run_loop.QuitClosure())); On 2017/01/03 08:43:19, mattcary wrote: > On 2017/01/02 18:15:36, pasko wrote: > > why not just run_loop.QuitClosure()? // i.e. without base::Bind and re-posting > a > > task > > The SW callback fires from the IO thread. Thanks. I did not know that.
https://codereview.chromium.org/2575523002/diff/120001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/120001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:539: sw_counter.WaitForCount(1); You have to wait the service worker to be activated. Otherwise this test will be flaky on slow machines. Use TitleWatcher and navigator.serviceWorker.ready. -- prerender_nostate_prefetch_browsertest.cc -- base::string16 expected_title = base::ASCIIToUTF16("READY"); content::TitleWatcher title_watcher(GetActiveWebContents(), expected_title); ui_test_utils::NavigateToURL( current_browser(), src_server()->GetURL(MakeAbsolute(kServiceWorkerLoader))); EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle()); -- service_worker.html -- <script type="text/javascript"> window.addEventListener('load', function() { navigator.serviceWorker.register('/prerender/service_worker.js') .then(function(registration) { console.log('ServiceWorker registered'); return navigator.serviceWorker.ready; }) .then(function(registration) { document.title = 'READY'; }) .catch(function(err) { console.log(err); }); }); </script> https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. Please write comment that the |callback| is executed on the IO thread. I think the |callback| should be executed when all workers are stopped. And even if there is no worker we should execute |callback|.
https://codereview.chromium.org/2575523002/diff/120001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/120001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:539: sw_counter.WaitForCount(1); On 2017/01/05 05:33:58, horo wrote: > You have to wait the service worker to be activated. > Otherwise this test will be flaky on slow machines. > > Use TitleWatcher and navigator.serviceWorker.ready. > > -- prerender_nostate_prefetch_browsertest.cc -- > > base::string16 expected_title = base::ASCIIToUTF16("READY"); > content::TitleWatcher title_watcher(GetActiveWebContents(), expected_title); > ui_test_utils::NavigateToURL( > current_browser(), > src_server()->GetURL(MakeAbsolute(kServiceWorkerLoader))); > EXPECT_EQ(expected_title, title_watcher.WaitAndGetTitle()); > > > -- service_worker.html -- > > <script type="text/javascript"> > window.addEventListener('load', function() { > navigator.serviceWorker.register('/prerender/service_worker.js') > .then(function(registration) { > console.log('ServiceWorker registered'); > return navigator.serviceWorker.ready; > }) > .then(function(registration) { > document.title = 'READY'; > }) > .catch(function(err) { > console.log(err); > }); > }); > </script> Thanks! That seems much more reliable than just waiting for the js to be read. https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. On 2017/01/05 05:33:58, horo wrote: > Please write comment that the |callback| is executed on the IO thread. > > I think the |callback| should be executed when all workers are stopped. > And even if there is no worker we should execute |callback|. Done. Note that the RunSoon helper defined in service_worker_context_wrapper.cc doesn't check for the callback being null; is that worth adding or should I keep the code as-is?
https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. On 2017/01/05 14:30:43, mattcary wrote: > On 2017/01/05 05:33:58, horo wrote: > > Please write comment that the |callback| is executed on the IO thread. > > > > I think the |callback| should be executed when all workers are stopped. > > And even if there is no worker we should execute |callback|. > > Done. The |callback| should be called only once when the "all" running workers are stopped. Not "each". Could you please change the code using BarrierClosure? > Note that the RunSoon helper defined in service_worker_context_wrapper.cc > doesn't check for the callback being null; is that worth adding or should I keep > the code as-is? I don't think we need the null check.
https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/120001/content/public/browser... content/public/browser/service_worker_context.h:129: // there is no running worker, |callback| will never be executed. On 2017/01/06 02:27:41, horo wrote: > On 2017/01/05 14:30:43, mattcary wrote: > > On 2017/01/05 05:33:58, horo wrote: > > > Please write comment that the |callback| is executed on the IO thread. > > > > > > I think the |callback| should be executed when all workers are stopped. > > > And even if there is no worker we should execute |callback|. > > > > Done. > > The |callback| should be called only once when the "all" running workers are > stopped. Not "each". > Could you please change the code using BarrierClosure? > Ah, I misunderstood. That's a good idea, done. > > Note that the RunSoon helper defined in service_worker_context_wrapper.cc > > doesn't check for the callback being null; is that worth adding or should I > keep > > the code as-is? > > I don't think we need the null check. done
Sorry for coming back so late. I'm a little worried about the new "StopAllWorkersWithCallback" function since the original StopAllWorkers is actually not 100% reliable. It makes more sense as a fire-and-forget thing, since a stopped worker could actually automatically restart soon after the stop callback is invoked. Actually, I think for this test coverage you don't really need to stop the worker. When Chrome decides whether a SW should intercept a request, it doesn't care about whether the SW is already running. So there is not much benefit to stopping it. https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:552: // Checks that a registered ServiceWorker (SW) without a renderer intercepts a nit: Does "without a renderer" mean the same thing as "not currently running"? That'd be more common service worker terminology. https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:563: // Stop the SW renderer process. Ditto, I think rather than "stop the renderer process" this is saying "Stop the SW." https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:565: content::StoragePartition* storage_partation = nit: typo for "partition" https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... File chrome/test/data/prerender/service_worker.js (right): https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:3: // found in the LICENSE file. nit: No (c) after copyright. I'd copy https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... instead of copying an existing file since it may be wrong. https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:20: ['<html><body><img src="/prerender/image.png"/></body></html>']); nit: I don't think this needs to be a Blob. You can just respond with a string. https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.h:111: void StopAllServiceWorkersForOriginWithCallback( These functions are a bit misleading since ServiceWorkerVersion::StopWorker() doesn't necessarily mean the worker is really stopped for good. It could be restarted soon after the callback is invoked in ServiceWorkerVersion::OnStoppedInternal (see the |should_restart| thing). I'd rather not have a function with a callback that seems to indicate "all these workers are really stopped" since we don't yet guarantee that. https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... content/public/browser/service_worker_context.h:128: // As above, but |callback| is executed for each running worker stopped, or if nit: This comment no longer matches the latest patchset (not "each" anymore).
On 2017/01/06 16:17:55, falken wrote: > Sorry for coming back so late. I'm a little worried about the new > "StopAllWorkersWithCallback" function since the original StopAllWorkers is > actually not 100% reliable. It makes more sense as a fire-and-forget thing, > since a stopped worker could actually automatically restart soon after the stop > callback is invoked. > > Actually, I think for this test coverage you don't really need to stop the > worker. When Chrome decides whether a SW should intercept a request, it doesn't > care about whether the SW is already running. So there is not much benefit to > stopping it. > Thanks for the review, I'll get to the comments shortly. Regarding the point you make above: the issue that concerned us was because nostate prefetch starts up a kind of weird renderer that does a truncated parse, it might interfere with SWs getting restarted. Hence this attempt to kill a SW and confirm that everything works. If you think that's unnecessary I can remove that from the test of course. > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:552: // > Checks that a registered ServiceWorker (SW) without a renderer intercepts a > nit: Does "without a renderer" mean the same thing as "not currently running"? > That'd be more common service worker terminology. > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:563: // Stop > the SW renderer process. > Ditto, I think rather than "stop the renderer process" this is saying "Stop the > SW." > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:565: > content::StoragePartition* storage_partation = > nit: typo for "partition" > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > File chrome/test/data/prerender/service_worker.js (right): > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > chrome/test/data/prerender/service_worker.js:3: // found in the LICENSE file. > nit: No (c) after copyright. I'd copy > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > instead of copying an existing file since it may be wrong. > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > chrome/test/data/prerender/service_worker.js:20: ['<html><body><img > src="/prerender/image.png"/></body></html>']); > nit: I don't think this needs to be a Blob. You can just respond with a string. > > https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... > File content/browser/service_worker/service_worker_context_wrapper.h (right): > > https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... > content/browser/service_worker/service_worker_context_wrapper.h:111: void > StopAllServiceWorkersForOriginWithCallback( > These functions are a bit misleading since ServiceWorkerVersion::StopWorker() > doesn't necessarily mean the worker is really stopped for good. It could be > restarted soon after the callback is invoked in > ServiceWorkerVersion::OnStoppedInternal (see the |should_restart| thing). I'd > rather not have a function with a callback that seems to indicate "all these > workers are really stopped" since we don't yet guarantee that. > > https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... > File content/public/browser/service_worker_context.h (right): > > https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... > content/public/browser/service_worker_context.h:128: // As above, but |callback| > is executed for each running worker stopped, or if > nit: This comment no longer matches the latest patchset (not "each" anymore).
On 2017/01/06 16:42:41, mattcary wrote: > On 2017/01/06 16:17:55, falken wrote: > > Sorry for coming back so late. I'm a little worried about the new > > "StopAllWorkersWithCallback" function since the original StopAllWorkers is > > actually not 100% reliable. It makes more sense as a fire-and-forget thing, > > since a stopped worker could actually automatically restart soon after the > stop > > callback is invoked. > > > > Actually, I think for this test coverage you don't really need to stop the > > worker. When Chrome decides whether a SW should intercept a request, it > doesn't > > care about whether the SW is already running. So there is not much benefit to > > stopping it. > > > > Thanks for the review, I'll get to the comments shortly. > > Regarding the point you make above: the issue that concerned us was because > nostate prefetch starts up a kind of weird renderer that does a truncated parse, > it might interfere with SWs getting restarted. Hence this attempt to kill a SW > and confirm that everything works. > > If you think that's unnecessary I can remove that from the test of course. I see, I think that makes sense. Some quick thoughts before I sign off: - Could we just get the RenderProcessHost the worker is in and just kill it with Shutdown? That's a big hammer but would guarantee the worker is stopped. - Maybe StopAllWorkersWithCallback could do some thing where it recursively calls back itself after the barrier is called and only ends up invoking the callback once all "workers to stop" are in STOPPED status so StopWorker didn't need to be called. - Maybe we just need warning comments in StopAllWorkersWithCallback about this caveat. > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > > File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc > (right): > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:552: // > > Checks that a registered ServiceWorker (SW) without a renderer intercepts a > > nit: Does "without a renderer" mean the same thing as "not currently running"? > > That'd be more common service worker terminology. > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:563: // > Stop > > the SW renderer process. > > Ditto, I think rather than "stop the renderer process" this is saying "Stop > the > > SW." > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... > > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:565: > > content::StoragePartition* storage_partation = > > nit: typo for "partition" > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > > File chrome/test/data/prerender/service_worker.js (right): > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > > chrome/test/data/prerender/service_worker.js:3: // found in the LICENSE file. > > nit: No (c) after copyright. I'd copy > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > instead of copying an existing file since it may be wrong. > > > > > https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... > > chrome/test/data/prerender/service_worker.js:20: ['<html><body><img > > src="/prerender/image.png"/></body></html>']); > > nit: I don't think this needs to be a Blob. You can just respond with a > string. > > > > > https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... > > File content/browser/service_worker/service_worker_context_wrapper.h (right): > > > > > https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... > > content/browser/service_worker/service_worker_context_wrapper.h:111: void > > StopAllServiceWorkersForOriginWithCallback( > > These functions are a bit misleading since ServiceWorkerVersion::StopWorker() > > doesn't necessarily mean the worker is really stopped for good. It could be > > restarted soon after the callback is invoked in > > ServiceWorkerVersion::OnStoppedInternal (see the |should_restart| thing). I'd > > rather not have a function with a callback that seems to indicate "all these > > workers are really stopped" since we don't yet guarantee that. > > > > > https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... > > File content/public/browser/service_worker_context.h (right): > > > > > https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... > > content/public/browser/service_worker_context.h:128: // As above, but > |callback| > > is executed for each running worker stopped, or if > > nit: This comment no longer matches the latest patchset (not "each" anymore).
Thanks for your response, I got caught up in sheriffing so couldn't get back to it until today. LMK what you think about this big-hammer approach. Other comments addressed as well. https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:552: // Checks that a registered ServiceWorker (SW) without a renderer intercepts a On 2017/01/06 16:17:55, falken wrote: > nit: Does "without a renderer" mean the same thing as "not currently running"? > That'd be more common service worker terminology. Done. https://codereview.chromium.org/2575523002/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:565: content::StoragePartition* storage_partation = On 2017/01/06 16:17:55, falken wrote: > nit: typo for "partition" Done. https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... File chrome/test/data/prerender/service_worker.js (right): https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:3: // found in the LICENSE file. On 2017/01/06 16:17:55, falken wrote: > nit: No (c) after copyright. I'd copy > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > instead of copying an existing file since it may be wrong. Done. https://codereview.chromium.org/2575523002/diff/160001/chrome/test/data/prere... chrome/test/data/prerender/service_worker.js:20: ['<html><body><img src="/prerender/image.png"/></body></html>']); On 2017/01/06 16:17:55, falken wrote: > nit: I don't think this needs to be a Blob. You can just respond with a string. Done. https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... File content/browser/service_worker/service_worker_context_wrapper.h (right): https://codereview.chromium.org/2575523002/diff/160001/content/browser/servic... content/browser/service_worker/service_worker_context_wrapper.h:111: void StopAllServiceWorkersForOriginWithCallback( On 2017/01/06 16:17:55, falken wrote: > These functions are a bit misleading since ServiceWorkerVersion::StopWorker() > doesn't necessarily mean the worker is really stopped for good. It could be > restarted soon after the callback is invoked in > ServiceWorkerVersion::OnStoppedInternal (see the |should_restart| thing). I'd > rather not have a function with a callback that seems to indicate "all these > workers are really stopped" since we don't yet guarantee that. LMK if the new big-hammer seems better. Also if it should be marked as for testing or whatever. https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... File content/public/browser/service_worker_context.h (right): https://codereview.chromium.org/2575523002/diff/160001/content/public/browser... content/public/browser/service_worker_context.h:128: // As above, but |callback| is executed for each running worker stopped, or if On 2017/01/06 16:17:55, falken wrote: > nit: This comment no longer matches the latest patchset (not "each" anymore). Moot
Actually, yeah I think the big hammer is appropriate here for testing how the "weird renderer" for prefetch might interfere with SW startup, since otherwise the SW would likely just restart in the original renderer it was using. Just a question about if we can avoid adding the code to ServiceWorkerContextWrapper. https://codereview.chromium.org/2575523002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:560: ->KillAllServiceWorkersForOrigin(src_server()->GetURL("/")); Could we move the RenderProcessHost shutdown code to inside this test itself? I.e., use RenderProcessHost::AllHostsIterator and shut down each one. Actually do we just expect there to be one RPH alive now (the one that navigated to the URL above, which the SW should also be running in)? If so, we should just be able to kill that process (it might be worth having an EXPECT that the number of processes == 1 to ensure the test is really killing the process with the SW).
+clamy Camille, could you take a quick look and make sure I'm doing the tab re-opening correctly in the test? Killing all the RenderProcessHosts kills the current tab and I wasn't positive this was the right way to reopen/refresh it. https://codereview.chromium.org/2575523002/diff/180001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/180001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:560: ->KillAllServiceWorkersForOrigin(src_server()->GetURL("/")); On 2017/01/09 15:42:23, falken wrote: > Could we move the RenderProcessHost shutdown code to inside this test itself? > I.e., use RenderProcessHost::AllHostsIterator and shut down each one. Actually > do we just expect there to be one RPH alive now (the one that navigated to the > URL above, which the SW should also be running in)? If so, we should just be > able to kill that process (it might be worth having an EXPECT that the number of > processes == 1 to ensure the test is really killing the process with the SW). Done.
service worker lgtm https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:26: #include "content/public/browser/storage_partition.h" I think lines 25-26 are no longer needed. https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:556: // Stop any SW. Could be worth mentioning that we're killing the render process in order to test whether the renderer for prefetch interferes with SW startup, since otherwise the SW would likely just start in the original render process.
https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:568: // Open a new tab to replace the one closed with all the RenderProcessHosts. Note: if all the processes in a tab have been killed, you don't need to create a new tab to have a new process. Just navigating to a url from the browser should recreate a process for the tab. That said if you really want to create a new tab, the function below should work correctly.
https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:26: #include "content/public/browser/storage_partition.h" On 2017/01/10 13:47:42, falken wrote: > I think lines 25-26 are no longer needed. Done. https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:556: // Stop any SW. On 2017/01/10 13:47:42, falken wrote: > Could be worth mentioning that we're killing the render process in order to test > whether the renderer for prefetch interferes with SW startup, since otherwise > the SW would likely just start in the original render process. Done. https://codereview.chromium.org/2575523002/diff/200001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:568: // Open a new tab to replace the one closed with all the RenderProcessHosts. On 2017/01/10 14:42:53, clamy wrote: > Note: if all the processes in a tab have been killed, you don't need to create a > new tab to have a new process. Just navigating to a url from the browser should > recreate a process for the tab. > > That said if you really want to create a new tab, the function below should work > correctly. I would have thought that as well, but if I don't create a new tab, the test hangs. I presume it's something to do with the in-process browser test setup, but I couldn't confirm.
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, falken@chromium.org Link to the patchset: https://codereview.chromium.org/2575523002/#ps220001 (title: "comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484061364975260,
"parent_rev": "4396f3061c3d2aa3769481a69fa2f7cace69d4c9", "commit_rev":
"0aed258863772ab7ba983993a2e1ce934d1caf66"}
Message was sent while issue was closed.
Description was changed from ========== Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch. Confirm that a SW is invoked for files loaded from NoState prefetch, even if a registered SW does not have a process. The caveats discussed in crbug/465200 still hold regarding the origin for which the SW is called. BUG=632368 ========== to ========== Prerender: Confirm ServiceWorkers are invoked for NoState Prefetch. Confirm that a SW is invoked for files loaded from NoState prefetch, even if a registered SW does not have a process. The caveats discussed in crbug/465200 still hold regarding the origin for which the SW is called. BUG=632368 Review-Url: https://codereview.chromium.org/2575523002 Cr-Commit-Position: refs/heads/master@{#442607} Committed: https://chromium.googlesource.com/chromium/src/+/0aed258863772ab7ba983993a2e1... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/0aed258863772ab7ba983993a2e1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
