New issue
Advanced search Search tips

Issue 752394 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task

Blocked on:
issue 822131

Blocking:
issue 715640



Sign in to add a comment

Add its own feature flag for SW servicification

Project Member Reported by kinuko@chromium.org, Aug 4 2017

Issue description

Currently we're just abusing NetworkService feature flag (+ PlzNavigate) flag, but we should probably have its own flag so that it can be enabled without depending on NetworkService.
 
Am thinking of taking this.

Does s13n SW work without NetworkService? If it doesn't work, it seems we could just rely on NetworkService flag, so that we don't need to specify multiple flags to get s13n SW to run, and the Mojo Linux FYI bot will use it automatically?

Assuming we want to be able to use s13n SW without NetworkService, I propose:
1) --service-worker-service turns on s13n SW
2) --enable-network-service triggers --service-worker-service to be on (because I think the default SW implementation won't work when NetworkService is on)

How does that sound?
Cc: jam@chromium.org
Yeah there's a bit of dependency around that. First, s13n SW does NOT work without NetworkService YET, so yes for the time being we could just rely on NetworkService flag, or do something like what you wrote, or could do the opposite (e.g. --service-worker-service triggers --enable-network-service as well).  From the testing coverage perspective your plan sounds good / better.

Comment 3 by falken@chromium.org, Sep 29 2017

Labels: -Type-Bug -Pri-2 Pri-3 Type-Task

Comment 4 by laforge@google.com, Nov 7 2017

Components: Internals>Network>Service

Comment 5 by laforge@google.com, Nov 7 2017

Components: -Internals>Network>Service Internals>Services>Network
Apologies, applied the wrong component in bulk.
Labels: M-66 Proj-Servicification
Owner: shimazu@chromium.org
Status: Started (was: Available)
I'll start to work on this. 
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/98f946db176850467a07011a7e068897c6b3f184

commit 98f946db176850467a07011a7e068897c6b3f184
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Jan 24 07:29:24 2018

Add ServiceWorkerServicification flag

This CL tentatively adds ServiceWorkerServicification feature flag. Currently it
doesn't work since S13nServiceWorker features are now implemented with
NetworkService. If we ship S13nServiceWorker before NetworkService, we need to
change some parts so that service worker can work with SWURLRequestRequest*
objects.

Bug:  752394 
Change-Id: I605214b738e802f74df72d6f90c265206a6ec488
Reviewed-on: https://chromium-review.googlesource.com/880125
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531472}
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/chrome/browser/about_flags.cc
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/content/common/service_worker/service_worker_utils.cc
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/content/public/common/content_features.cc
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/content/public/common/content_features.h
[modify] https://crrev.com/98f946db176850467a07011a7e068897c6b3f184/tools/metrics/histograms/enums.xml

horo@ has already tried to add the flag at crrev.com/c/867829.
I started to investigate how to use S13nServiceWorker without NetworkService.
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835

commit 2b3953e86bbfc3ce210f514cb1cccb88ea3d2835
Author: Matt Falkenhagen <falken@chromium.org>
Date: Wed Feb 14 07:32:18 2018

Revert "Add ServiceWorkerServicification flag"

This reverts commit 98f946db176850467a07011a7e068897c6b3f184

Reason for revert: Suspected cause of bug 810865.

R=shimazu

Bug:  752394 ,810685
Change-Id: I204cc0070873fcc48716c39fd59ee03a0669bf5f
TBR: shimazu
Reviewed-on: https://chromium-review.googlesource.com/918142
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536668}
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/chrome/browser/about_flags.cc
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/content/common/service_worker/service_worker_utils.cc
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/content/public/common/content_features.cc
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/content/public/common/content_features.h
[modify] https://crrev.com/2b3953e86bbfc3ce210f514cb1cccb88ea3d2835/tools/metrics/histograms/enums.xml

Hmm, I'm still not sure how c#9 helped issue 810685..
I suspected we have something leaked from the flags, but c#9 doesn't seem doing something wrong. That means someone enables the SWS13n flag intentionally.
I'm now guessing some bots automatically enabled the flag. I'm now asking to the chops team if these bots really exist.
Project Member

Comment 11 by bugdroid1@chromium.org, Feb 21 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/22078cc0770cf9d49a592cfb2497b8887790dc80

commit 22078cc0770cf9d49a592cfb2497b8887790dc80
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Wed Feb 21 03:01:44 2018

Reland "Add ServiceWorkerServicification flag"

This reverts commit 2b3953e86bbfc3ce210f514cb1cccb88ea3d2835.

Reason for revert: bug 810865 has been fixed before the revert patch.

Original change's description:
> Revert "Add ServiceWorkerServicification flag"
> 
> This reverts commit 98f946db176850467a07011a7e068897c6b3f184
> 
> Reason for revert: Suspected cause of bug 810865.
> 
> R=​shimazu
> 
> Bug:  752394 ,810685
> Change-Id: I204cc0070873fcc48716c39fd59ee03a0669bf5f
> TBR: shimazu
> Reviewed-on: https://chromium-review.googlesource.com/918142
> Reviewed-by: Matt Falkenhagen <falken@chromium.org>
> Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
> Commit-Queue: Matt Falkenhagen <falken@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#536668}

TBR=falken@chromium.org,shimazu@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  752394 , 810685
Change-Id: I073d76f90fcc78e1ffa7b9d4524fb0b89af73e2f
Reviewed-on: https://chromium-review.googlesource.com/925726
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538009}
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/chrome/browser/about_flags.cc
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/content/common/service_worker/service_worker_utils.cc
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/content/public/common/content_features.cc
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/content/public/common/content_features.h
[modify] https://crrev.com/22078cc0770cf9d49a592cfb2497b8887790dc80/tools/metrics/histograms/enums.xml

Note: We should make this flag trigger NavigationMojoResponse and once it's reasonably working we should add a virtual layout test suite for it.
Blockedon: 822131
Project Member

Comment 14 by bugdroid1@chromium.org, Mar 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42

commit 44c2c32308cfc8d4f3c25ab02a586ddb7facfa42
Author: Makoto Shimazu <shimazu@chromium.org>
Date: Fri Mar 30 01:10:20 2018

Create S13nServiceWorker related objects without network service

When ServiceWorkerServicification is enabled, this CL is to make
ServiceWorkerURLLoaderJob and ServiceWorkerSubresourceLoader work without
NetworkService. This CL tries to create an URLLoaderRequestHandler for a service
worker (ServiceWorkerControlleeRequestHandler) even if no NetworkService flag.

ServiceWorkerServicification still doesn't work since
ServiceWorkerURLLoaderJob::ProceedWithResponse() hasn't been implemented yet.

Bug:  752394 
Change-Id: I808346f248bba254e20e708d372dde46ecc103fe
Reviewed-on: https://chromium-review.googlesource.com/888361
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Makoto Shimazu <shimazu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547053}
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/frame_host/navigation_request_info.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/frame_host/navigation_request_info.h
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/loader/navigation_url_loader.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/service_worker/service_worker_navigation_loader.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/service_worker/service_worker_request_handler.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/browser/storage_partition_impl.cc
[modify] https://crrev.com/44c2c32308cfc8d4f3c25ab02a586ddb7facfa42/content/public/common/browser_side_navigation_policy.cc

Status: Fixed (was: Started)
Now we have ServiceWorkerServicification (aka NetS13nServiceWorker) flag and it excersise the code to intercept by using the new path (SWNavigationLoader, SWSubresourceLoader etc).

Labels: -M-66 M-67

Sign in to add a comment