New issue
Advanced search Search tips

Issue 908993 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 18
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Run network service in its own thread when run in-process

Project Member Reported by cduvall@chromium.org, Nov 27

Issue description

Right now the network service runs on the IO thread when running with the NetworkServiceInProcess flag. If we run network service on it's own thread, it will unblock the IO thread and may have performance gains.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 28

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

commit c2227ae1467d525c15802d8dd6ddbe309c432515
Author: Clark DuVall <cduvall@chromium.org>
Date: Wed Nov 28 22:26:36 2018

Fix some browser tests when running network service in process

This fixes all of content_browsertests and some of the main failures in
browser_tests.

Bug:  908993 
Change-Id: Ic77481ef0a5e05110878a9610172863fb9ee6994
Reviewed-on: https://chromium-review.googlesource.com/c/1352411
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611898}
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/chrome/browser/net/chrome_network_service_restart_browsertest.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/chrome/browser/net/network_context_configuration_browsertest.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/content/browser/browsing_data/clear_site_data_handler_browsertest.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/content/browser/frame_host/render_frame_host_impl.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/content/browser/loader/loader_browsertest.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/content/browser/network_service_restart_browsertest.cc
[modify] https://crrev.com/c2227ae1467d525c15802d8dd6ddbe309c432515/content/browser/webrtc/webrtc_browsertest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, Nov 29

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

commit a55fa40b78c9d14cb9377915ae89849f5e4f4d22
Author: Clark DuVall <cduvall@chromium.org>
Date: Thu Nov 29 19:57:15 2018

Fix ChromeMockCertVerifier not setting up in-process NS certs

ChromeMockCertVerifier was forgetting to call the parent class methods
that set up the in-process NS cert verifier.

Bug:  908993 
Change-Id: I15251a705006c7f5dd1dc99027999e1b8739e416
Reviewed-on: https://chromium-review.googlesource.com/c/1355194
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612327}
[modify] https://crrev.com/a55fa40b78c9d14cb9377915ae89849f5e4f4d22/chrome/browser/ssl/cert_verifier_browser_test.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Nov 30

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

commit 8c88d4ef08d8370208df516a2644a0d1baa221d6
Author: Clark DuVall <cduvall@chromium.org>
Date: Fri Nov 30 01:45:16 2018

Fix browser tests when running network service in process

These mostly fall into two categories:
1. tests trying to crash network service
2. tests using NetworkServiceTest (which isn't available with in-process NS)

Bug:  908993 
Change-Id: I91d518b260f79ef263a58fb2a3a24e28e7c96210
Reviewed-on: https://chromium-review.googlesource.com/c/1355615
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612488}
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/browsing_data/browsing_data_remover_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/net/network_connection_tracker_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/net/network_quality_estimator_prefs_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/net/network_quality_tracker_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/policy/policy_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/policy/policy_network_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/ssl/ssl_browsertest.cc
[modify] https://crrev.com/8c88d4ef08d8370208df516a2644a0d1baa221d6/chrome/browser/ui/login/login_handler_browsertest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Dec 1

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

commit 3528b5502f924b4d0315e8b8ca4e87a12a5e3bc4
Author: Clark DuVall <cduvall@chromium.org>
Date: Sat Dec 01 00:47:36 2018

Convert WebKitBrowserTest to use URLLoaderInterceptor

This test was using a net::URLRequestInterceptor so was not actually
working correctly with network service. The interceptor it was using
also assumes it is on the IO thread which was causing issues when
running NS in-process on its own thread.

Bug:  908993 
Change-Id: I009ddf13b53cecac9615c4d2019405ddede57c69
Reviewed-on: https://chromium-review.googlesource.com/c/1357405
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612879}
[modify] https://crrev.com/3528b5502f924b4d0315e8b8ca4e87a12a5e3bc4/content/browser/webkit_browsertest.cc
[modify] https://crrev.com/3528b5502f924b4d0315e8b8ca4e87a12a5e3bc4/content/test/BUILD.gn
[delete] https://crrev.com/3f5f28927b8ce587d11353069f1f05e646607627/content/test/net/url_request_abort_on_end_job.cc
[delete] https://crrev.com/3f5f28927b8ce587d11353069f1f05e646607627/content/test/net/url_request_abort_on_end_job.h

Project Member

Comment 5 by bugdroid1@chromium.org, Dec 4

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

commit c17c54716d06bfb37040b0d0ce7416ddc32f051e
Author: Clark DuVall <cduvall@chromium.org>
Date: Tue Dec 04 21:25:04 2018

Change DataElement's ChunkedDataPipeGetter to PtrInfo

When this was a Ptr, if a RequestBody was passed between threads there
would be an error when trying to use the chunked data pipe because it is
used on a different sequence. Changing it to PtrInfo prevents this
issue, and also matches the behavior of DataElement::data_pipe_getter_.

Bug:  908993 
Change-Id: Id943817e28b9b0992f76f2619d045ebccaec4243
Reviewed-on: https://chromium-review.googlesource.com/c/1356592
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613701}
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/content/browser/speech/speech_recognition_engine_unittest.cc
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/content/browser/speech/speech_recognizer_impl_unittest.cc
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/services/network/public/cpp/data_element.cc
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/services/network/public/cpp/data_element.h
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/services/network/public/cpp/network_ipc_param_traits.cc
[modify] https://crrev.com/c17c54716d06bfb37040b0d0ce7416ddc32f051e/services/network/url_loader.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Dec 4

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

commit cb429b90be7c147ad24a522373103f95745bd360
Author: Clark DuVall <cduvall@chromium.org>
Date: Tue Dec 04 23:06:32 2018

Fix SSLPKPBrowserTest with NS in-process

These were calling GetRequestContext() when NS was run in-process, which
DCHECKs. Split the setup code into two different functions, so the one
that needs GetRequestContext() isn't run with NS enabled.

Bug:  908993 
Change-Id: I33713fb9da9d2a44d05ecce24db6c59445096918
Reviewed-on: https://chromium-review.googlesource.com/c/1361704
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613745}
[modify] https://crrev.com/cb429b90be7c147ad24a522373103f95745bd360/chrome/browser/ssl/ssl_browsertest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 7

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

commit a89d2f882b65865b5a53241d197691e7f6f4ea3d
Author: Clark DuVall <cduvall@chromium.org>
Date: Thu Dec 06 21:06:21 2018

Run network service tests in-process on FYI bots

This runs all Android tests in-process on the Mojo Android bot, and runs
browser_tests in-process on Windows/Linux.

Bug:  908993 
Change-Id: I86a683885c0c3081533623d5771ffb4f24561a70
Reviewed-on: https://chromium-review.googlesource.com/c/1361921
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614488}
[modify] https://crrev.com/a89d2f882b65865b5a53241d197691e7f6f4ea3d/testing/buildbot/chromium.fyi.json
[modify] https://crrev.com/a89d2f882b65865b5a53241d197691e7f6f4ea3d/testing/buildbot/test_suites.pyl
[modify] https://crrev.com/a89d2f882b65865b5a53241d197691e7f6f4ea3d/testing/buildbot/waterfalls.pyl

Forgot to put the bug on the change, but https://chromium-review.googlesource.com/c/chromium/src/+/1308593 has the implementation of network service in a separate thread.
Labels: M-72 Merge-Request-72
Requesting merge to M72 for http://crrev.com/c/1308593. This change only affects the network service path, so should be safe to merge.
Project Member

Comment 10 by sheriffbot@chromium.org, Dec 17

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-72 Merge-Approved-72
Approving merge for  http://crrev.com/c/1308593 to M72 branch 3626 based on comment #9. Please merge ASAP. Thank you.
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 18

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/19cae4dc7ba24a7049e796022dba422282ac5b47

commit 19cae4dc7ba24a7049e796022dba422282ac5b47
Author: Clark DuVall <cduvall@chromium.org>
Date: Tue Dec 18 01:01:07 2018

Run network service in separate thread when run in-process

This is behind a flag that is enabled by default, so we can turn it off
if necessary. All in-process tests are passing with this change.

Change-Id: I03b9146bfc74b922c4b34d23fec80f85d1126ff5
Reviewed-on: https://chromium-review.googlesource.com/c/1308593
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615318}
(cherry picked from commit b9924e9c41099fe6027708ec65597ad148becdeb)


TBR=cduvall@chromium.org

Bug:  908993 
Change-Id: I60f7ae8e0eec3c1a8bb88430c82b6b2a9fffa004
Reviewed-on: https://chromium-review.googlesource.com/c/1380558
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#415}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/chrome/browser/net/network_quality_estimator_prefs_browsertest.cc
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/chrome/browser/net/network_quality_tracker_browsertest.cc
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/content/browser/network_service_instance.cc
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/content/browser/service_manager/service_manager_context.cc
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/content/browser/service_manager/service_manager_context.h
[modify] https://crrev.com/19cae4dc7ba24a7049e796022dba422282ac5b47/content/public/browser/network_service_instance.h

Status: Fixed (was: Started)
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/19cae4dc7ba24a7049e796022dba422282ac5b47

Commit: 19cae4dc7ba24a7049e796022dba422282ac5b47
Author: cduvall@chromium.org
Commiter: cduvall@chromium.org
Date: 2018-12-18 01:01:07 +0000 UTC

Run network service in separate thread when run in-process

This is behind a flag that is enabled by default, so we can turn it off
if necessary. All in-process tests are passing with this change.

Change-Id: I03b9146bfc74b922c4b34d23fec80f85d1126ff5
Reviewed-on: https://chromium-review.googlesource.com/c/1308593
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615318}
(cherry picked from commit b9924e9c41099fe6027708ec65597ad148becdeb)


TBR=cduvall@chromium.org

Bug:  908993 
Change-Id: I60f7ae8e0eec3c1a8bb88430c82b6b2a9fffa004
Reviewed-on: https://chromium-review.googlesource.com/c/1380558
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#415}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}

Sign in to add a comment