New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 834008 link

Starred by 3 users
Status: Started
Owner:
rsleevi@chromium.org
Cc:
█ robpercival@chromium.org
eranm@chromium.org
asymmetric@chromium.org
rsleevi@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task



Sign in to add a comment

Review CT implementation threading and lifetime

Project Member Reported by rsleevi@chromium.org, Apr 17 2018 
We've had a spate of threading and lifetime bugs in the CT code, and need to do a full review of the implementation that has organically evolved.

A few components worth reviewing:
* STHSetComponentInstaller::ComponentReady is not thread-correct ( https://bugs.chromium.org/p/chromium/issues/detail?id=828665#c18)
* STHSetComponentInstaller is created on the UI thread, but accesses objects on the IO thread - https://chromium.googlesource.com/chromium/src/+/2e1d92fb44d6b6fb12a32db59190b47a9766658c/chrome/browser/component_updater/sth_set_component_installer.cc#199
* LogDnsClient::QueryAuditProof has odd cancellation semantics (Issue 827841 and related review)
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 19 2018 

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

commit 013c7af6505fc9bd7341b92f770b80d3888f7af0
Author: Ryan Sleevi <rsleevi@chromium.org>
Date: Thu Apr 19 02:52:40 2018

Defer STH loading until after startup

The STHSet component receives periodic updates of new STHs from known
logs. The process for the update involves receiving the component,
loading the files from disk, then notifying the IO thread of the
received STHs. On the IO thread, it verifies the STHs against the log's
public key, and if valid, processes that STH.

On a fresh install, this process will normally be well after the browser
has started. However, on a browser restart, this may trigger during
the browser startup path, as components are loaded.

As a result, defer the processing of the files from disk until after the
browser has started up, to reduce IO contention.

In addition, the current threading patterns involved three task runners - the
UI thread (which installs the components), the background thread doing
the file IO, and the IO thread validating the STHs. However, the
threading patterns weren't safe - primarily for situations of shutdown
in which the IO thread is in the process of being destructed. This
rearranges the thread sequencing to ensure that all objects have
defined, safe lifetimes, by changing the ownership model for the
STHObserver and hiding the threading details as an internal
implementation detail.

BUG=834008,  828665 

Change-Id: Iea850b4ee3348b9339faea52e14f11965709a5b7
Reviewed-on: https://chromium-review.googlesource.com/1015518
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551927}
[modify] https://crrev.com/013c7af6505fc9bd7341b92f770b80d3888f7af0/chrome/browser/component_updater/sth_set_component_installer.cc
[modify] https://crrev.com/013c7af6505fc9bd7341b92f770b80d3888f7af0/chrome/browser/component_updater/sth_set_component_installer.h
[modify] https://crrev.com/013c7af6505fc9bd7341b92f770b80d3888f7af0/chrome/browser/component_updater/sth_set_component_installer_unittest.cc
►

Sign in to add a comment