New issue
Advanced search Search tips

Issue 853233 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug
Proj-Servicification



Sign in to add a comment

NetworkService: Move IOThread histograms to NetworkService.

Project Member Reported by mmenke@chromium.org, Jun 15 2018

Issue description

We gather a couple SSL-related histograms in IOThread.  We should move this (And any OpenSSL global initialization calls) into NetworkService.  This probably doesn't block Windows canary, but it's trivial to do, and results in there being less stuff to sort through in IOThread.
 
OpenSSL initialization needs to happen in both places. The browser process calls into crypto bits. In theory we should be calling EnsureOpenSSLInit at all the relevant places at a lower level though.

Actually, on ARM, the initialization requires reading some files out of /proc, to work around Android issues, so it actually can't be first initialized outside of the sandbox anyway. Though used to be covered by this code, but I don't know if Mojo does something else:
https://cs.chromium.org/chromium/src/content/app/content_main_runner_impl.cc?rcl=daf6f231d72f09a419c3cb0c18c1de4b24d0cafb&l=375

Once initialized, yeah, it doesn't matter where those histograms happen.
I should add: we're looking at making BoringSSL initialization-less[*], which should avoid the need for most of these calls.

[*] Technically it already is, but Chromium is allergic to static initializers, so it builds in a less caller-friendly mode.

Comment 3 by mmenke@chromium.org, Jun 15 2018

Hrm...We're initializing open SSL on the IO thread, but network stuff is mostly moving to the UI thread (Since that's where the NetworkService mojo pipe lives), so we may need to re-think where and how that happens.  I'm leaving the old initialization call alone for now, just copying the parameters, but I'll file a new bug specifically for that once I move the histograms (Sending out a massively complicated CL that does that once it manages to build on Android).
The initialization function may freely be called on multiple threads or multiple times. It's totally idempotent. (It wraps a pthread_once_t or equivalent. It's just there because we need to sample the CPUID at some point.)

Comment 5 by mmenke@chromium.org, Jun 15 2018

Right, but if everything that uses OpenSSL is moved from the IO thread to the UI thread...there's a chance the current initialization call, done when creating the IOThread on the IOThread, will then be done too late.
I'm sure both UI and IO thread both thoroughly call into crypto right now, but fair enough. That code should hopefully also call EnsureOpenSSLInit, but maybe we'll need to stick an initialization somewhere.
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 18 2018

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

commit 0be4d07be30b8bac6b75c59b19890ed8efe0b50c
Author: Matt Menke <mmenke@chromium.org>
Date: Mon Jun 18 17:17:23 2018

NetworkService:  Move some histograms over from IOThread.

IOThread is going away eventually, but we should still record the
histograms.

Bug:  853233 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I46b2811a934bbab59066161327f7deb8e8991fd7
Reviewed-on: https://chromium-review.googlesource.com/1102717
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568050}
[modify] https://crrev.com/0be4d07be30b8bac6b75c59b19890ed8efe0b50c/chrome/browser/io_thread.cc
[modify] https://crrev.com/0be4d07be30b8bac6b75c59b19890ed8efe0b50c/services/network/BUILD.gn
[modify] https://crrev.com/0be4d07be30b8bac6b75c59b19890ed8efe0b50c/services/network/DEPS
[modify] https://crrev.com/0be4d07be30b8bac6b75c59b19890ed8efe0b50c/services/network/network_service.cc

Comment 8 by mmenke@chromium.org, Jun 18 2018

Status: Fixed (was: Started)

Sign in to add a comment