Closed Bug 1277064 Opened 3 years ago Closed 3 years ago

Possible deadlock when initializing distribution

Categories

(Firefox for Android :: Android partner distribution, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed
fennec 48+ ---

People

(Reporter: jchen, Assigned: jchen)

Details

Attachments

(2 files, 3 obsolete files)

For example,

> "GeckoBackgroundThread"
>   at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:211)
>   - waiting to lock <0x098a6315> (a java.util.HashMap) held by thread 16
>   at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:207)
>   at org.mozilla.gecko.GeckoSharedPrefs.forProfile(GeckoSharedPrefs.java:99)
>   at org.mozilla.gecko.GeckoSharedPrefs.forProfile(GeckoSharedPrefs.java:90)
>   at org.mozilla.gecko.preferences.DistroSharedPrefsImport.importPreferences(DistroSharedPrefsImport.java:34)
>   at org.mozilla.gecko.GeckoProfile$1.distributionFound(GeckoProfile.java:1096)
>   - locked <0x0f057a59> (a org.mozilla.gecko.GeckoProfile)
> 
> "IntentService[GeckoTelemetryUploadSerWorker]"
>   at org.mozilla.gecko.GeckoProfile.getDir(GeckoProfile.java:571)
>   - waiting to lock <0x0f057a59> (a org.mozilla.gecko.GeckoProfile) held by thread 9
>   at org.mozilla.gecko.GeckoProfile.get(GeckoProfile.java:292)
>   - locked <0x098a6315> (a java.util.HashMap)

There are two locks involved (on the profile and on the profile cache hash map), but they can be locked in different orders, causing possible deadlocks.
Only lock the profile cache hash map when we're accessing the hash map;
otherwise unlock it, so that the lock doesn't inadvertently cause
deadlocks.
Attachment #8758754 - Flags: review?(snorp)
Comment on attachment 8758754 [details] [diff] [review]
Only lock profile cache when accessing it (v1)

Review of attachment 8758754 [details] [diff] [review]:
-----------------------------------------------------------------

Maybe just use java.util.concurrent.ConcurrentHashMap

Failing that, having some utility functions that do the synchronizing seems wise. Otherwise we're hosed if there is a sProfileCache.get/put without the synchronized block.
Attachment #8758754 - Flags: review?(snorp) → review-
ConcurrentHashMap version
Attachment #8759246 - Flags: review?(snorp)
Attachment #8758754 - Attachment is obsolete: true
ConcurrentHashMap version with fix for a possible race condition
Attachment #8759256 - Flags: review?(snorp)
Attachment #8759246 - Attachment is obsolete: true
Attachment #8759246 - Flags: review?(snorp)
Attachment #8759256 - Flags: review?(snorp) → review+
Pushed by nchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8e9ee5565
Use ConcurrentHashMap for profile cache; r=snorp
https://hg.mozilla.org/mozilla-central/rev/e8d8e9ee5565
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
FYI, we need this in Firefox 48 after it has baked a little.
tracking-fennec: --- → ?
jchen:

Can you do the aurora/beta uplift ask?

Thanks.
Flags: needinfo?(nchen)
tracking-fennec: ? → 48+
Is this ready for fF 48?
Comment on attachment 8759256 [details] [diff] [review]
Use ConcurrentHashMap for profile cache (v3)

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Possible deadlock on startup when using distributions
[Describe test coverage new/current, TreeHerder]: Locally
[Risks and why]: Small risk but patch has looked good on m-c and aurora.
[String/UUID change made/needed]: None
Flags: needinfo?(nchen)
Attachment #8759256 - Flags: approval-mozilla-beta?
Comment on attachment 8759256 [details] [diff] [review]
Use ConcurrentHashMap for profile cache (v3)

Fix a pain point, taking it
Attachment #8759256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This is hitting conflicts on beta, can we either get a rebased patch that applies or drop the uplift request?

$ hg graft -er e8d8e9ee5565                                                                                                                                 
grafting 346699:e8d8e9ee5565 "Bug 1277064 - Use ConcurrentHashMap for profile cache; r=snorp"
merging mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(nchen)
Attached patch Patch for Beta (obsolete) — Splinter Review
Rebased patch for Beta.
Flags: needinfo?(nchen) → needinfo?(wkocher)
Attachment #8768017 - Flags: review+
still seems to have conflicts like patching file mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java
Hunk #1 FAILED at 8
Hunk #2 FAILED at 63
Hunk #3 FAILED at 200
Hunk #4 FAILED at 258
4 out of 4 hunks FAILED -- saving rejects to file mobile/android/base/java/org/mozilla/gecko/GeckoProfile.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1277064-beta.patch
Flags: needinfo?(wkocher) → needinfo?(nchen)
Sorry, here's a new patch.
Attachment #8768017 - Attachment is obsolete: true
Flags: needinfo?(nchen) → needinfo?(cbook)
Attachment #8768875 - Flags: review+
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.