New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Jan 11
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 905903: Concurrent calls to ConcurrentHashMap put() can fail to add the key/value to the map

Reported by nyquist@chromium.org, Nov 16 Project Member

Issue description

See this bug from the public Android issue tracker: https://issuetracker.google.com/issues/37042460

From the bug:
"Basically, multithreaded put(), putIfAbsent(), putAll(), ... operations can fail to add a key/value pair if there are hash collisions on the added keys and the underlying datastructure needs to be resized at the time of the concurrent puts."


Since we're using ConcurrentHashMap in ApplicationStatus, we might be observing the effects of this bug. See:
https://cs.chromium.org/search/?q=ConcurrentHashMap+-file:third_party/android_tools/sdk+-file:third_party/mockito+-file:third_party/robolectric&sq=package:chromium&type=cs

We should see if this affects us, and if so, whether there is some way we can work around it, for example by ensuring we are using a newer version of ConcurrentHashMap.


Oracle issue:
https://bugs.openjdk.java.net/browse/JDK-8028564
Fix (I think):
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/jdk7/java/util/concurrent/ConcurrentHashMap.java?r1=1.35&r2=1.36
 

Comment 1 by nyquist@chromium.org, Nov 16

Status: Available (was: Untriaged)

Comment 2 by pasko@chromium.org, Nov 16

Cc: pasko@chromium.org
One other option is to switch to a normal HashMap. The sActivityInfo is already used under sCurrentApplicationStateLock in a few places, maybe adding it in a few other places would be not too expensive?

My understanding of the ConcurrentHashMap is that it is a server-size beast that scales well under heavy read-write load. We likely do not need that.

Comment 3 by agrieve@chromium.org, Jan 11

Labels: DevX
Agree the thing to do here is to just ban use of ConcurrentHashMap in favor of Collections.synchronizedMap(). 

Would also be good to add an ErrorProne check.

Comment 4 by bugdroid1@chromium.org, Jan 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b083d66bda71ca14e758550c9edb60ef236770b8

commit b083d66bda71ca14e758550c9edb60ef236770b8
Author: Andrew Grieve <agrieve@chromium.org>
Date: Fri Jan 11 21:03:22 2019

Android: Replace ConcurrentHashMap usage with Collections.synchronizedMap()

ConcurrentHashMap has a bug on L.
Also adds a checkstyle check to prevent future uses.

Bug:  905903 
Change-Id: Ic6afed17b5d7d3c6a150935117fb33e0408955e4
Reviewed-on: https://chromium-review.googlesource.com/c/1407493
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622143}
[modify] https://crrev.com/b083d66bda71ca14e758550c9edb60ef236770b8/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/b083d66bda71ca14e758550c9edb60ef236770b8/tools/android/checkstyle/chromium-style-5.0.xml

Comment 5 by agrieve@chromium.org, Jan 11

Status: Fixed (was: Available)

Comment 6 by bugdroid1@chromium.org, Jan 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e0e82c93404791efbe187bfa1f9a8e72335dbd80

commit e0e82c93404791efbe187bfa1f9a8e72335dbd80
Author: agrieve <agrieve@chromium.org>
Date: Sat Jan 12 03:22:40 2019

Revert "Android: Replace ConcurrentHashMap usage with Collections.synchronizedMap()"

This reverts commit b083d66bda71ca14e758550c9edb60ef236770b8.

Reason for revert: Realized forgot to synchronize iterators.

Original change's description:
> Android: Replace ConcurrentHashMap usage with Collections.synchronizedMap()
> 
> ConcurrentHashMap has a bug on L.
> Also adds a checkstyle check to prevent future uses.
> 
> Bug:  905903 
> Change-Id: Ic6afed17b5d7d3c6a150935117fb33e0408955e4
> Reviewed-on: https://chromium-review.googlesource.com/c/1407493
> Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
> Commit-Queue: agrieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#622143}

TBR=agrieve@chromium.org,tiborg@chromium.org

Change-Id: I64fa06d829df489fdf85249bc69331adc7df0031
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  905903 
Reviewed-on: https://chromium-review.googlesource.com/c/1408029
Reviewed-by: agrieve <agrieve@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622284}
[modify] https://crrev.com/e0e82c93404791efbe187bfa1f9a8e72335dbd80/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/e0e82c93404791efbe187bfa1f9a8e72335dbd80/tools/android/checkstyle/chromium-style-5.0.xml

Comment 7 by bugdroid1@chromium.org, Jan 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b7225829975f5a97b986f51cd4c2df74bd237271

commit b7225829975f5a97b986f51cd4c2df74bd237271
Author: Andrew Grieve <agrieve@chromium.org>
Date: Tue Jan 15 02:59:45 2019

Reland: Android: Replace ConcurrentHashMap usage with manual lock.

ConcurrentHashMap has a bug on L.
* Also adds a checkstyle check to prevent future uses.
* Also adds @AnyThread / @MainThread to all public methods

Reason for reland:
 * Now synchronizing map iterators

Bug:  905903 
Change-Id: Id7788a4707329a5e441cc010e1fb297668510aef
Reviewed-on: https://chromium-review.googlesource.com/c/1409725
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622699}
[modify] https://crrev.com/b7225829975f5a97b986f51cd4c2df74bd237271/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/b7225829975f5a97b986f51cd4c2df74bd237271/tools/android/checkstyle/chromium-style-5.0.xml

Comment 8 by bugdroid1@chromium.org, Jan 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ab41c2866ed57abc1f778496440e84a7023a787c

commit ab41c2866ed57abc1f778496440e84a7023a787c
Author: Arthur Sonzogni <arthursonzogni@chromium.org>
Date: Tue Jan 15 09:26:42 2019

Revert "Reland: Android: Replace ConcurrentHashMap usage with manual lock."

This reverts commit b7225829975f5a97b986f51cd4c2df74bd237271.

Reason for revert: bug 921945

Original change's description:
> Reland: Android: Replace ConcurrentHashMap usage with manual lock.
> 
> ConcurrentHashMap has a bug on L.
> * Also adds a checkstyle check to prevent future uses.
> * Also adds @AnyThread / @MainThread to all public methods
> 
> Reason for reland:
>  * Now synchronizing map iterators
> 
> Bug:  905903 
> Change-Id: Id7788a4707329a5e441cc010e1fb297668510aef
> Reviewed-on: https://chromium-review.googlesource.com/c/1409725
> Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
> Commit-Queue: agrieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#622699}

TBR=agrieve@chromium.org,tiborg@chromium.org

Change-Id: Ifa376b028ffe8d08517a87145ec8fe3c0a8ea8e3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  905903 
Reviewed-on: https://chromium-review.googlesource.com/c/1411337
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622794}
[modify] https://crrev.com/ab41c2866ed57abc1f778496440e84a7023a787c/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/ab41c2866ed57abc1f778496440e84a7023a787c/tools/android/checkstyle/chromium-style-5.0.xml

Comment 9 by bugdroid1@chromium.org, Jan 18

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d69c7a37d711183e6c215115ccaa29ffc6b59a76

commit d69c7a37d711183e6c215115ccaa29ffc6b59a76
Author: Andrew Grieve <agrieve@chromium.org>
Date: Fri Jan 18 17:55:46 2019

Reland #2: Android: Replace ConcurrentHashMap usage with manual lock.

ConcurrentHashMap has a bug on L.
* Also adds a checkstyle check to prevent future uses.
* Also adds @AnyThread / @MainThread to all public methods

Previously reverted in ab41c2866ed57abc1f778496440e84a7023a787c.

Reason for reland: Fixed toolchain issue (crbug.com/693079)

Change since previous land:
* Optimizations so that we're now removing some methods rather than
adding some (which the previous land did).

Change-Id: I00afd74394964c62b6cc5358a8546048aa215e8d
Bug:  905903 
Reviewed-on: https://chromium-review.googlesource.com/c/1412233
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Tibor Goldschwendt <tiborg@chromium.org>
Commit-Queue: agrieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624190}
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/base/android/java/src/org/chromium/base/ApplicationStatus.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/base/android/java/src/org/chromium/base/ContextUtils.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/base/android/junit/src/org/chromium/base/ApplicationStatusTest.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/base/test/android/junit/src/org/chromium/base/test/BaseRobolectricTestRunner.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/chrome/android/junit/src/org/chromium/chrome/browser/gcore/GoogleApiClientHelperTest.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/chrome/android/junit/src/org/chromium/chrome/browser/offlinepages/OfflineBackgroundTaskTest.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java
[modify] https://crrev.com/d69c7a37d711183e6c215115ccaa29ffc6b59a76/tools/android/checkstyle/chromium-style-5.0.xml

Sign in to add a comment