Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare for NetworkService migration #2351

Closed
iefremov opened this issue Dec 4, 2018 · 10 comments
Closed

Prepare for NetworkService migration #2351

iefremov opened this issue Dec 4, 2018 · 10 comments
Assignees
Labels
feature/cookies feature/shields The overall Shields feature in Brave. priority/P1 A very extremely bad problem. We might push a hotfix for it.

Comments

@iefremov
Copy link
Contributor

iefremov commented Dec 4, 2018

Chromium team is in the middle of a big project, called servicification [1]. One of the most significant parts of it is transition to the Network Service [2].

One of the goals of moving to NS is to avoid storing information in ProfileIOData (i.e. cookies), so ProfileIOData will eventually become deprecated. At the moment, the core code of brave shields brave_shields_util.cc depends on ProfileIOData, so it needs to be reworked in the future. `

ChromeNetworkDelegate itself probably will also be deprecated in favor of NetworkServiceNetworkDelegate

Deadlines or milestone dates of [1] and [2] are not yet clear.

[1] https://www.chromium.org/servicification
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=598073

@iefremov
Copy link
Contributor Author

iefremov commented Dec 4, 2018

@tomlowenthal FYI

@tildelowengrimm tildelowengrimm added feature/shields The overall Shields feature in Brave. priority/P4 Planned work. We expect to get to it "soon". feature/cookies labels Dec 4, 2018
@tildelowengrimm tildelowengrimm added this to the 1.x Backlog milestone Dec 4, 2018
@zorae
Copy link

zorae commented Dec 8, 2018

Can confirm that Shield becomes ineffective with chrome://flags/#network-service enabled.

@iefremov
Copy link
Contributor Author

ChromeNetworkDelegate is not functioning with NetworkService, it seems we should start with replacing it witih NetworkServiceNetworkDelegate. ProfileIOData will stay with us for a while.

@tildelowengrimm tildelowengrimm added priority/P3 The next thing for us to work on. It'll ride the trains. and removed priority/P4 Planned work. We expect to get to it "soon". labels Jan 29, 2019
@rebron rebron modified the milestone: 1.x Backlog Feb 7, 2019
mkarolin added a commit to brave/brave-core that referenced this issue Mar 13, 2019
When enabled this feature does not instantiate ChromeNetworkDelegate and
by its virtue BraveNetworkDelegateBase which is where we add hooks into
network stack.

Related: brave/brave-browser#2351

Chromium change to not instantiate ChromeNetworkDelegate:

commit  4b7d02f22b70c27f32fa5508f785ba310d08aad3
Author: John Abd-El-Malek < [email protected]  >
Date: Thu May 03 17:46:35 2018

Don't instantiate ChromeNetworkDelegate when network service is enabled.

Bug:  598073

Chromium change in C74 to enable network service by default:

commit 708db9bc46bec1d0037f44f8fdaff7b7d72724e0
Author: John Abd-El-Malek <[email protected]>
Date:   Mon Feb 25 18:10:35 2019 +0000

    Enable network service on Win/Mac/Linux by default as it launched on stable.

    Removing running integration tests on these bots (waterfall + CQ).

    Keep running the layout tests with network service disabled on on debug Linux Tests bots as we're still supporting ChromeOS/Android/Cast which haven't switched over yet.

    Also remove the disabled-service-worker-servicification virtual test.

    Bug: 933880,926114
mkarolin added a commit to brave/brave-core that referenced this issue Mar 15, 2019
When enabled this feature does not instantiate ChromeNetworkDelegate and
by its virtue BraveNetworkDelegateBase which is where we add hooks into
network stack.

Related: brave/brave-browser#2351

Chromium change to not instantiate ChromeNetworkDelegate:

commit  4b7d02f22b70c27f32fa5508f785ba310d08aad3
Author: John Abd-El-Malek < [email protected]  >
Date: Thu May 03 17:46:35 2018

Don't instantiate ChromeNetworkDelegate when network service is enabled.

Bug:  598073

Chromium change in C74 to enable network service by default:

commit 708db9bc46bec1d0037f44f8fdaff7b7d72724e0
Author: John Abd-El-Malek <[email protected]>
Date:   Mon Feb 25 18:10:35 2019 +0000

    Enable network service on Win/Mac/Linux by default as it launched on stable.

    Removing running integration tests on these bots (waterfall + CQ).

    Keep running the layout tests with network service disabled on on debug Linux Tests bots as we're still supporting ChromeOS/Android/Cast which haven't switched over yet.

    Also remove the disabled-service-worker-servicification virtual test.

    Bug: 933880,926114
mkarolin added a commit to brave/brave-core that referenced this issue Mar 18, 2019
When enabled this feature does not instantiate ChromeNetworkDelegate and
by its virtue BraveNetworkDelegateBase which is where we add hooks into
network stack.

Related: brave/brave-browser#2351

Chromium change to not instantiate ChromeNetworkDelegate:

commit  4b7d02f22b70c27f32fa5508f785ba310d08aad3
Author: John Abd-El-Malek < [email protected]  >
Date: Thu May 03 17:46:35 2018

Don't instantiate ChromeNetworkDelegate when network service is enabled.

Bug:  598073

Chromium change in C74 to enable network service by default:

commit 708db9bc46bec1d0037f44f8fdaff7b7d72724e0
Author: John Abd-El-Malek <[email protected]>
Date:   Mon Feb 25 18:10:35 2019 +0000

    Enable network service on Win/Mac/Linux by default as it launched on stable.

    Removing running integration tests on these bots (waterfall + CQ).

    Keep running the layout tests with network service disabled on on debug Linux Tests bots as we're still supporting ChromeOS/Android/Cast which haven't switched over yet.

    Also remove the disabled-service-worker-servicification virtual test.

    Bug: 933880,926114
mkarolin added a commit to brave/brave-core that referenced this issue Mar 19, 2019
When enabled this feature does not instantiate ChromeNetworkDelegate and
by its virtue BraveNetworkDelegateBase which is where we add hooks into
network stack.

Related: brave/brave-browser#2351

Chromium change to not instantiate ChromeNetworkDelegate:

commit  4b7d02f22b70c27f32fa5508f785ba310d08aad3
Author: John Abd-El-Malek < [email protected]  >
Date: Thu May 03 17:46:35 2018

Don't instantiate ChromeNetworkDelegate when network service is enabled.

Bug:  598073

Chromium change in C74 to enable network service by default:

commit 708db9bc46bec1d0037f44f8fdaff7b7d72724e0
Author: John Abd-El-Malek <[email protected]>
Date:   Mon Feb 25 18:10:35 2019 +0000

    Enable network service on Win/Mac/Linux by default as it launched on stable.

    Removing running integration tests on these bots (waterfall + CQ).

    Keep running the layout tests with network service disabled on on debug Linux Tests bots as we're still supporting ChromeOS/Android/Cast which haven't switched over yet.

    Also remove the disabled-service-worker-servicification virtual test.

    Bug: 933880,926114
mkarolin added a commit to brave/brave-core that referenced this issue Mar 20, 2019
When enabled this feature does not instantiate ChromeNetworkDelegate and
by its virtue BraveNetworkDelegateBase which is where we add hooks into
network stack.

Related: brave/brave-browser#2351

Chromium change to not instantiate ChromeNetworkDelegate:

commit  4b7d02f22b70c27f32fa5508f785ba310d08aad3
Author: John Abd-El-Malek < [email protected]  >
Date: Thu May 03 17:46:35 2018

Don't instantiate ChromeNetworkDelegate when network service is enabled.

Bug:  598073

Chromium change in C74 to enable network service by default:

commit 708db9bc46bec1d0037f44f8fdaff7b7d72724e0
Author: John Abd-El-Malek <[email protected]>
Date:   Mon Feb 25 18:10:35 2019 +0000

    Enable network service on Win/Mac/Linux by default as it launched on stable.

    Removing running integration tests on these bots (waterfall + CQ).

    Keep running the layout tests with network service disabled on on debug Linux Tests bots as we're still supporting ChromeOS/Android/Cast which haven't switched over yet.

    Also remove the disabled-service-worker-servicification virtual test.

    Bug: 933880,926114
@bbondy
Copy link
Member

bbondy commented Mar 20, 2019

Please revert this as part of this work:
brave/brave-core@9257ced

@bbondy bbondy added priority/P1 A very extremely bad problem. We might push a hotfix for it. and removed priority/P3 The next thing for us to work on. It'll ride the trains. labels Mar 20, 2019
@bbondy
Copy link
Member

bbondy commented Mar 20, 2019

upgraded to p1 priority

@iefremov
Copy link
Contributor Author

iefremov commented May 2, 2019

As part of migration we have to fix Tor - it doesn't work with enabled NetworkService. It seems that the reason is TorProfileServiceImpl::SetNewTorCircuitOnIOThread: URLRequestContext is not compatible with NetworkService @darkdh

@iefremov
Copy link
Contributor Author

iefremov commented May 6, 2019

Filed #4312

@simonhong simonhong self-assigned this May 7, 2019
@simonhong
Copy link
Member

simonhong commented May 7, 2019

Failed browser test with NS on MacOS.

    AdBlockServiceTest.AdBlockThirdPartyWorksForThirdPartyHost (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:649)
    AdBlockServiceTest.AdsGetBlockedAfterDataFileVersionUpgrade (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:393)
    AdBlockServiceTest.AdsGetBlockedByCustomBlocker (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:290)
    AdBlockServiceTest.AdsGetBlockedByDefaultBlocker (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:244)
    AdBlockServiceTest.AdsGetBlockedByRegionalBlocker (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:335)
    AdBlockServiceTest.BlockNYP (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:671)
    AdBlockServiceTest.CancelRequestOptionTest (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:792)
    AdBlockServiceTest.NewTabContinuesToBlock (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:474)
    AdBlockServiceTest.SocialButttonAdBLockTagTest (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:693)
    AdBlockServiceTest.SubFrame (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:509)
    AdBlockServiceTest.TagPrefsControlTags (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:744)
    AdBlockServiceTest.TrackerReferencedFromUntrustedDomain (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:573)
    AdBlockServiceTest.TwoDiffAdsGetCountedAsTwo (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:450)
    AdBlockServiceTest.TwoSameAdsGetCountedAsOne (../../brave/components/brave_shields/browser/ad_block_service_browsertest.cc:426)
    BraveNetworkDelegateBaseBrowserTest.ThirdPartySTS (../../brave/browser/net/brave_network_delegate_hsts_fingerprinting_browsertest.cc:93)
    BraveNetworkDelegateBrowserTest.Iframe3PCookieBlocked (../../brave/browser/net/brave_network_delegate_browsertest.cc:70)
    BraveNetworkDelegateBrowserTest.IframeJs3PCookieBlocked (../../brave/browser/net/brave_network_delegate_browsertest.cc:87)
    HTTPSEverywhereServiceTest.RedirectsKnownSite (../../brave/components/brave_shields/browser/https_everywhere_service_browsertest.cc:101)
    HTTPSEverywhereServiceTest.RedirectsKnownSiteInIframe (../../brave/components/brave_shields/browser/https_everywhere_service_browsertest.cc:127)
    TrackingProtectionServiceTest.TrackerReferencedFromUntrustedDomainGetsBlocked (../../brave/components/brave_shields/browser/tracking_protection_service_browsertest.cc:169)

@iefremov
Copy link
Contributor Author

iefremov commented Jun 14, 2019

The current plan is to implement our own ProxyingURLLoaderFactory similar to WebRequestProxyingURLLoaderFactory, since this is the only logical interface for our Shields machinery after enabling NetworkService.

There is a draft PR with this factory, cargoculted/copypasted from two existing Proxying factories in Chromium (brave/brave-core#2701), not sure if it is possible to reduce the amount of boilerplate, but probably stuff connected to TrustedHeaderClient will not be needed.

Plumbing easiest individual shield callbacks seems working (not presented in the PR), though porting the whole loop of callbacks that occasionally redirect requests is somewhat bigger task, which I'm doing right now.

There is one thing that could be done in parallel with the current stuff - dealing with AdBlockInterceptor, which is a part of our ad-blocking mechanism. In the current shields architecture, we set some special header during the ad-blocking session in NetworkDelegate and later craft an empty 200 OK response in the interceptor. NetworkService doesn't work with URLRequest interceptors, instead, some custom URLLoaderFactory should be provided, which probably should be different from the Proxying one described above. it seems it should be replaced with custom URLLoaderRequestInterceptor or NavigationLoaderInterceptor paired with implementation of mojom::URLLoaderClient

@iefremov
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/cookies feature/shields The overall Shields feature in Brave. priority/P1 A very extremely bad problem. We might push a hotfix for it.
Projects
None yet
Development

No branches or pull requests

7 participants