Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(148)

Issue 11299159: Fix CrossProcessNotificationMultiProcessTest.Basic on Android. (Closed)

Created:
8 years ago by Philippe
Modified:
8 years ago
CC:
chromium-reviews, ilevy+watch_chromium.org, feature-media-reviews_chromium.org, bulach+watch_chromium.org, yfriedman+watch_chromium.org, klundberg+watch_chromium.org, erikwright+watch_chromium.org, peter+watch_chromium.org, frankf+watch_chromium.org
Visibility:
Public.

Description

Fix CrossProcessNotificationMultiProcessTest.Basic on Android. On Android the ashmem API is used to deal with shared memory. This API doesn't provide any way to open a memory region created by another process for security reasons. The only way to share memory on Android is to share the underlying file descriptor between processes. This can be accomplished by: - Creating a new shared memory region in a process. Note that this region can be anonymous (i.e. the string provided to ashmem_create_region() can be empty). - Forking and keeping open both in the parent and child processes the file descriptor corresponding to the previously created memory region. - Doing an mmap() in both processes (nothing special here). The unit test was deadlocking (stuck in the while loop in the child process) since the call to CreateNamed() wasn't opening the existing shared memory region (created by the parent process) but was actually creating a new shared memory region. This means that the two processes were actually seeing different memory regions. Since this also works on POSIX platforms, the unit test was modified to follow this strategy on all POSIX systems (including Android). On Windows the old strategy (using non-anonymous shared memory) is still used. BUG=136720 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169662

Patch Set 1 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -41 lines) Patch
M build/android/gtest_filter/media_unittests_disabled View 1 chunk +0 lines, -3 lines 0 comments Download
M media/audio/cross_process_notification_unittest.cc View 8 chunks +35 lines, -38 lines 0 comments Download

Messages

Total messages: 9 (0 generated)
Philippe
8 years ago (2012-11-23 16:34:58 UTC) #1
tommi (sloooow) - chröme
lgtm Thanks for the detailed explanation.
8 years ago (2012-11-23 20:44:01 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/11299159/4002
8 years ago (2012-11-23 20:44:10 UTC) #3
commit-bot: I haz the power
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, ...
8 years ago (2012-11-23 23:23:10 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/11299159/4002
8 years ago (2012-11-24 08:35:10 UTC) #5
commit-bot: I haz the power
Commit queue patch verification failed without an error message. Something went wrong, probably a crash, ...
8 years ago (2012-11-24 10:22:55 UTC) #6
Philippe
On 2012/11/24 10:22:55, I haz the power (commit-bot) wrote: > Commit queue patch verification failed ...
8 years ago (2012-11-26 11:40:37 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pliard@chromium.org/11299159/4002
8 years ago (2012-11-27 14:48:32 UTC) #8
commit-bot: I haz the power
8 years ago (2012-11-27 15:47:19 UTC) #9
Message was sent while issue was closed.
Change committed as 169662

Powered by Google App Engine
This is Rietveld 408576698