Chromium Code Reviews
Help | Chromium Project | Sign in
(256)

Issue 8509038: Linux: split GNOME Keyring integration into a separate process.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 5 months ago by Mike Mammarella
Modified:
2 years ago
Reviewers:
vandebo
CC:
chromium-reviews_chromium.org
Visibility:
Public.

Description

Linux: split GNOME Keyring integration into a separate process.

This will avoid possible deadlocks with password sync by removing any need to
access the UI thread (aka the GLib main thread, required by GNOME Keyring).

Even with this change, the sync team needs to remove calls to private blocking
password store methods so that we can remove those methods and avoid blocking
the DB thread for slow GNOME Keyring operations. (We can then move GNOME
Keyring access back in process if we want.)

Basic theory of operation:

NativeBackendGnome creates a KeyringProxyClient, which forks and executes
chrome-keyring-proxy (a small helper binary). Then NativeBackendGnome creates
a RequestContext for each RPC it wants to run, and calls a proxy client method
that returns immediately but saves a pointer to the request context. Inside the
request context is a WaitableEvent that will be signaled when the request is
complete and the reply has been stored in the request context. (This design is
necessary because NativeBackendGnome implements a synchronous API, but we want
to allow parallel requests to be made.)

The proxy client assigns each request an ID and marshalls it into a set of
lines terminated by a blank line to send over over a socket to the keyring
proxy, which unmarshalls the request and calls the appropriate GNOME Keyring
APIs. The reply is sent back identified by the request ID, and a refcounted
file descriptor watcher delegate sees the readable file descriptor on the file
thread and handles the data, possibly signalling a waitable event if the data
completes the reply to a pending request.

The chrome-keyring-proxy binary is very small and does not depend on any chrome
libraries like base, net, etc. Further, the protocol is human readable which
may be useful for debugging. These were both intentional choices that ruled out
any of the other IPC mechanisms that might have also worked.

BUG=98601

Patch Set 1 : everything works #

Total comments: 3

Patch Set 2 : merge #

Patch Set 3 : fix unit test compile #

Patch Set 4 : classes renamed, namespaceified #

Patch Set 5 : fix test ordering dependency #

Total comments: 15

Patch Set 6 : merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2025 lines, -545 lines) Lint Patch
A chrome/browser/password_manager/keyring_proxy/gnome_keyring_loader.h View 1 2 3 1 chunk +57 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/gnome_keyring_loader.cc View 1 2 3 1 chunk +75 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/gnome_keyring_loader_unittest.cc View 1 2 3 4 1 chunk +37 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy.h View 1 2 3 1 chunk +55 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy.cc View 1 2 3 1 chunk +475 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.h View 1 2 3 4 5 1 chunk +142 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy_client.cc View 1 2 3 4 5 1 chunk +430 lines, -0 lines 0 comments 3 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy_client_unittest.cc View 1 2 3 4 5 1 chunk +270 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy_main.cc View 1 2 3 1 chunk +26 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/message_reader.h View 1 2 3 1 chunk +45 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/message_reader.cc View 1 2 3 1 chunk +40 lines, -0 lines 0 comments 1 errors Download
A chrome/browser/password_manager/keyring_proxy/message_reader_unittest.cc View 1 2 3 1 chunk +123 lines, -0 lines 0 comments 1 errors Download
M chrome/browser/password_manager/native_backend_gnome_x.h View 1 2 3 4 5 4 chunks +16 lines, -45 lines 0 comments 0 errors Download
M chrome/browser/password_manager/native_backend_gnome_x.cc View 1 2 3 4 5 8 chunks +64 lines, -460 lines 0 comments 0 errors Download
M chrome/browser/password_manager/native_backend_gnome_x_unittest.cc View 1 2 3 4 5 39 chunks +122 lines, -40 lines 0 comments 1 errors Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 3 chunks +12 lines, -0 lines 0 comments 0 errors Download
M chrome/chrome_exe.gypi View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments 0 errors Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 3 chunks +15 lines, -0 lines 0 comments 0 errors Download
Trybot results: Sign in to try more bots
Commit:

Issue must be closed to Revert

Messages

Total messages: 4
Mike Mammarella
OK, all set. Thanks for "volunteering" to review. :)
2 years, 5 months ago #1
Mike Mammarella
OK, all set. Thanks for "volunteering" to review. :)
2 years, 5 months ago #2
vandebo
As I mentioned, I think it's worthwhile to try to use the IPC mechanism instead ...
2 years, 4 months ago #3
vandebo
2 years ago #4
Hey Mike, this CL has been sitting for four months?  Isn't it causing some
crashing or other nastiness?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6