Introduce chrome://flags/#network-prediction-options-for-service-worker

Introduce chrome://flags/#network-prediction-options-for-service-worker

This CL introduces the following network predictor options for service worker controlled pages.

- PreconnectOnly (default): Execute the preconnection on navigations.
- StartServiceWorkerAndPreconnect: Start the service worker and execute the
  preconnection in parallel on navigations.
- StartServiceWorkerAndDeferPreconnect: Start the service worker on
  navigations, and defer the preconnection until the service worker startup.
- StartServiceWorkerOnly: Start the service worker on navigations and do not
  execute the preconnection.

BUG=727544

[horo, 2017-05-31 09:59:48]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 09:59:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 10:51:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 10:51:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-05-31 13:11:48]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 13:12:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 15:17:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 15:17:57]

Dry run: This issue passed the CQ dry run.

[horo, 2017-05-31 16:20:23]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 16:20:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2017-05-31 16:20:47]

Patchset #2 (id:20001) has been deleted

[horo, 2017-05-31 16:20:50]

Patchset #1 (id:1) has been deleted

[horo, 2017-05-31 16:23:49]

Description was changed from

==========
NetworkPredictionOptionsForServiceWorker

BUG=
==========

to

==========
Introduce chrome://flags/#network-prediction-options-for-service-worker

This CL introduces the following network predictor options for service worker
controlled pages.

- PreconnectOnly (default): Execute the preconnection on navigations.
- StartServiceWorkerAndPreconnect: Start the service worker and execute the
  preconnection in parallel on navigations.
- StartServiceWorkerAndDeferPreconnect: Start the service worker on
  navigations, and defer the preconnection until the service worker startup.
- StartServiceWorkerOnly: Start the service worker on navigations and do not
  execute the preconnection.

BUG=727544
==========

[horo, 2017-05-31 16:25:08]

horo@chromium.org changed reviewers:
+ falken@chromium.org

[horo, 2017-05-31 16:25:09]

falken@
Could you please review this?

[commit-bot: I haz the power, 2017-05-31 18:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 18:12:00]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-01 05:31:28]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-06-01 05:31:29]

https://codereview.chromium.org/2916533002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/2916533002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:125: if (!success ||
GetNetworkPredictionOptionsForServiceWorker() ==
Wouldn't this trigger PreconnectUrlWithPredictor twice if option ==
AND_PRECONNECT and lookup fails?

[horo, 2017-06-01 05:52:39]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[horo, 2017-06-01 05:52:55]

Thank you!

https://codereview.chromium.org/2916533002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/2916533002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:125: if (!success ||
GetNetworkPredictionOptionsForServiceWorker() ==
On 2017/06/01 05:31:28, kinuko wrote:
> Wouldn't this trigger PreconnectUrlWithPredictor twice if option ==
> AND_PRECONNECT and lookup fails?

Done.

[commit-bot: I haz the power, 2017-06-01 05:52:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2017-06-01 06:54:17]

We should also write a design doc for this.

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:119: void
PredictorTabHelper::DidStartServiceWorkerForNavigaionPreconnect(
Navigation

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:130:
START_SERVICE_WORKER_AND_DEFER_PRECONNECT) {
I feel a switch would be more readable here, since it we're already mentioning 2
of 4 of the cases, and more robust to changes to the enum later.

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:135: void
PredictorTabHelper::PreconnectUrlWithPredictor(const GURL& url) {
I'm wondering why did the name change to WithPredictor? It seems PreconnectUrl
was always done by (with?) the predictor.

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:891:
registration->pattern(), render_process_id);
Have you tested this with PlzNavigate? Is it still needed when PlzNavigate is
on?

The idea was with PlzNavigate and the new SiteInstance changes, we aren't going
to need SWProcessManager to keep track of RenderProcessHosts on its own.

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
content/public/browser/service_worker_context.h:145: // the specified render
process |host|. Must be called from the UI thread. The
The "for... for... in..." is ambiguous: is the document in host, or preconnect
in host?

This function itself also doesn't really have to do NavigationPreconnect. It
just starts the worker and the caller happens to be the predictor, and it's used
even if preconnect won't happen. I wonder if just StartServiceWorker is a better
name?

I'd suggest:

"Starts the service worker whose scope matches |document_url|. The service
worker is started in |host|. This can be used to warm up a service worker for a
navigation."

[commit-bot: I haz the power, 2017-06-01 08:16:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 08:16:27]

Dry run: This issue passed the CQ dry run.

[horo, 2017-06-01 10:53:11]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 10:53:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2017-06-01 11:13:51]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 11:14:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[horo, 2017-06-01 11:15:07]

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
File chrome/browser/net/predictor_tab_helper.cc (right):

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:119: void
PredictorTabHelper::DidStartServiceWorkerForNavigaionPreconnect(
On 2017/06/01 06:54:17, falken wrote:
> Navigation

Done.

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:130:
START_SERVICE_WORKER_AND_DEFER_PRECONNECT) {
On 2017/06/01 06:54:17, falken wrote:
> I feel a switch would be more readable here, since it we're already mentioning
2
> of 4 of the cases, and more robust to changes to the enum later.

Done.

https://codereview.chromium.org/2916533002/diff/60001/chrome/browser/net/pred...
chrome/browser/net/predictor_tab_helper.cc:135: void
PredictorTabHelper::PreconnectUrlWithPredictor(const GURL& url) {
On 2017/06/01 06:54:17, falken wrote:
> I'm wondering why did the name change to WithPredictor? It seems PreconnectUrl
> was always done by (with?) the predictor.

Done.

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:891:
registration->pattern(), render_process_id);
On 2017/06/01 06:54:17, falken wrote:
> Have you tested this with PlzNavigate? Is it still needed when PlzNavigate is
> on?
> 
> The idea was with PlzNavigate and the new SiteInstance changes, we aren't
going
> to need SWProcessManager to keep track of RenderProcessHosts on its own.

Added tests for PlzNavigate.

I think we still need this AddProcessReferenceToPattern() even if PlzNavigate is
on.
If we don't call AddProcessReferenceToPattern, StartWorker() may create a new
process.

But PredictorTabHelper::DidStartNavigation() is called before executing the
checks in InterceptNavigationDelegate::ShouldIgnoreNavigation().
So the navigation request may be canceled after here.

To avoid creating a new process for the request which will be canceled, we have
to call AddProcessReferenceToPattern() here.

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
content/public/browser/service_worker_context.h:145: // the specified render
process |host|. Must be called from the UI thread. The
On 2017/06/01 06:54:17, falken wrote:
> The "for... for... in..." is ambiguous: is the document in host, or preconnect
> in host?
> 
> This function itself also doesn't really have to do NavigationPreconnect. It
> just starts the worker and the caller happens to be the predictor, and it's
used
> even if preconnect won't happen. I wonder if just StartServiceWorker is a
better
> name?
> 
> I'd suggest:
> 
> "Starts the service worker whose scope matches |document_url|. The service
> worker is started in |host|. This can be used to warm up a service worker for
a
> navigation."

I changed it to "StartServiceWorkerForNavigation".
I think we should keep "Navigation" for the method name, because UMAs of
EventType::NAVIGATION will be recorded if this method is called.

[commit-bot: I haz the power, 2017-06-01 12:42:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-01 12:42:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[falken, 2017-06-01 23:24:14]

Mostly comments, I don't think we necessarily need to update the code. I think
there's non-trivial design decisions to be made here. Should we hash this out in
a design doc?

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_wrapper.cc (right):

https://codereview.chromium.org/2916533002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_wrapper.cc:891:
registration->pattern(), render_process_id);
On 2017/06/01 11:15:07, horo wrote:
> On 2017/06/01 06:54:17, falken wrote:
> > Have you tested this with PlzNavigate? Is it still needed when PlzNavigate
is
> > on?
> > 
> > The idea was with PlzNavigate and the new SiteInstance changes, we aren't
> going
> > to need SWProcessManager to keep track of RenderProcessHosts on its own.
> 
> Added tests for PlzNavigate.
> 
> I think we still need this AddProcessReferenceToPattern() even if PlzNavigate
is
> on.
> If we don't call AddProcessReferenceToPattern, StartWorker() may create a new
> process.
> 
> But PredictorTabHelper::DidStartNavigation() is called before executing the
> checks in InterceptNavigationDelegate::ShouldIgnoreNavigation().
> So the navigation request may be canceled after here.
> 
> To avoid creating a new process for the request which will be canceled, we
have
> to call AddProcessReferenceToPattern() here.

To be clear, the plan was to remove
ServiceWorkerProcessManager::AddProcessReferenceToPattern totally. A lot of
design went into allowing service workers to use SiteInstance directly with
PlzNavigate:
https://docs.google.com/document/d/1Zujvf5jcUtm1hyh-xhXYlyfPjdO2oEplz22fOhSj6...

I'm a bit reluctant to add a AddProcessReferenceToPattern here if we can't
remove it later.

How does PlzNavigate avoid creating a new process, while this might create a new
process? It's because we're starting the SW before the PlzNavigate logic begins?

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
File content/public/browser/service_worker_context.h (right):

https://codereview.chromium.org/2916533002/diff/60001/content/public/browser/...
content/public/browser/service_worker_context.h:145: // the specified render
process |host|. Must be called from the UI thread. The
On 2017/06/01 11:15:07, horo wrote:
> On 2017/06/01 06:54:17, falken wrote:
> > The "for... for... in..." is ambiguous: is the document in host, or
preconnect
> > in host?
> > 
> > This function itself also doesn't really have to do NavigationPreconnect. It
> > just starts the worker and the caller happens to be the predictor, and it's
> used
> > even if preconnect won't happen. I wonder if just StartServiceWorker is a
> better
> > name?
> > 
> > I'd suggest:
> > 
> > "Starts the service worker whose scope matches |document_url|. The service
> > worker is started in |host|. This can be used to warm up a service worker
for
> a
> > navigation."
> 
> I changed it to "StartServiceWorkerForNavigation".
> I think we should keep "Navigation" for the method name, because UMAs of
> EventType::NAVIGATION will be recorded if this method is called.

Sorry I totally overlooked the EventType. I think PRECONNECT was a better name
than NAVIGATION for the event type.

I'm concerned we're creating in this separate path for starting a service worker
for navigation different than the normal navigation path (starting a service
worker via URLRequestJob), just to start before the predictor. If we do create
this separate path the name PRECONNECT feels better to make it explicit that
it's a separate path.

It's worth noting that this CL will also affect our metrics about navigations
and timing like:
ServiceWorker.ActivatedWorkerPreparationForMainFrame.

[horo, 2017-06-08 08:31:31]

This CL is trying to add two optimizations:
[1]. Start the Service Worker earlier by ignores the SiteInstance's isolation by
     calling AddProcessReferenceToPattern().
[2]. Defer the preconnection to reduce the IO thread contention while starting
     the service worker.

But after inspecting the behavior of PlzNavigate on a low end device, I noticed
that if PlzNavigate is enabled and we can't ignore the SiteInstance's isolation,
these optimizations doesn't have big impact.
To start the service worker in a correct renderer process, we have to wait for
NavigationRequest::OnStartChecksComplete which calls
AddExpectedNavigationToSite.
So we can't start the service worker so earlier.

And also if a new renderer is created for the navigation, the IO thread
contention will not be in the critical path, because the process creation is
far more time consuming. And I think preconnection while starting a new process
looks reasonable. 

For navigations which don't create new renderer, the optimization [2] may have
a impact. But it is difficult to know whether the navigation is creating a new
process or not from WebContentsObserver.

[horo, 2017-06-28 10:44:23]

The CQ bit was checked by horo@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-28 10:44:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-28 12:20:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-28 12:20:30]

Dry run: This issue passed the CQ dry run.