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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 6 months ago by Mike Mammarella
Modified:
1 year ago
Reviewers:
CC:
chromium-reviews
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) Patch
A chrome/browser/password_manager/keyring_proxy/gnome_keyring_loader.h View 1 2 3 1 chunk +57 lines, -0 lines 0 comments Download
A chrome/browser/password_manager/keyring_proxy/gnome_keyring_loader.cc View 1 2 3 1 chunk +75 lines, -0 lines 0 comments 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 Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy.h View 1 2 3 1 chunk +55 lines, -0 lines 0 comments Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy.cc View 1 2 3 1 chunk +475 lines, -0 lines 0 comments 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 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 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 Download
A chrome/browser/password_manager/keyring_proxy/keyring_proxy_main.cc View 1 2 3 1 chunk +26 lines, -0 lines 0 comments Download
A chrome/browser/password_manager/keyring_proxy/message_reader.h View 1 2 3 1 chunk +45 lines, -0 lines 0 comments Download
A chrome/browser/password_manager/keyring_proxy/message_reader.cc View 1 2 3 1 chunk +40 lines, -0 lines 0 comments Download
A chrome/browser/password_manager/keyring_proxy/message_reader_unittest.cc View 1 2 3 1 chunk +123 lines, -0 lines 0 comments 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 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 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 Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 3 chunks +12 lines, -0 lines 0 comments Download
M chrome/chrome_exe.gypi View 1 2 3 4 5 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 3 chunks +15 lines, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit: CQ not working?

Messages

Total messages: 6 (0 generated)
Mike Mammarella
OK, all set. Thanks for "volunteering" to review. :)
3 years, 6 months ago (2011-11-22 04:50:39 UTC) #1
Mike Mammarella
OK, all set. Thanks for "volunteering" to review. :)
3 years, 6 months ago (2011-11-22 04:51:31 UTC) #2
vandebo (ex-Chrome)
As I mentioned, I think it's worthwhile to try to use the IPC mechanism instead ...
3 years, 5 months ago (2011-12-01 19:57:30 UTC) #3
vandebo (ex-Chrome)
Hey Mike, this CL has been sitting for four months? Isn't it causing some crashing ...
3 years, 1 month ago (2012-04-07 01:06:31 UTC) #4
vandebo (ex-Chrome)
On 2012/04/07 01:06:31, vandebo wrote: > Hey Mike, this CL has been sitting for four ...
1 year ago (2014-04-30 15:14:12 UTC) #5
Mike Mammarella
1 year ago (2014-04-30 17:15:49 UTC) #6
Oh, hmm. This probably won't even apply cleanly anymore. Let me see if I
can figure out how to clear it out.


On Wed, Apr 30, 2014 at 8:14 AM, <vandebo@chromium.org> wrote:

> On 2012/04/07 01:06:31, vandebo wrote:
>
>> Hey Mike, this CL has been sitting for four months?  Isn't it causing some
>> crashing or other nastiness?
>>
>
> ping
>
> https://codereview.chromium.org/8509038/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld ec887be