Last Comment Bug 714565 - Concurrency issue in BrowserProvider.java.in
: Concurrency issue in BrowserProvider.java.in
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 12
Assigned To: Richard Newman [:rnewman]
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-01 17:53 PST by Richard Newman [:rnewman]
Modified: 2012-01-31 04:51 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Proposed patch. v1 (5.38 KB, patch)
2012-01-01 18:00 PST, Richard Newman [:rnewman]
no flags Details | Diff | Splinter Review
Part 1: fix concurrency issue and some tidying. v2 (5.56 KB, patch)
2012-01-01 20:17 PST, Richard Newman [:rnewman]
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Part 2: clean up excessive logging. v1 (5.93 KB, patch)
2012-01-01 20:23 PST, Richard Newman [:rnewman]
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2012-01-01 17:53:02 PST
I noticed this as soon as I opened the file to reduce log output.

    private HashMap<String, DatabaseHelper> mDatabasePerProfile;
 
...
 
        DatabaseHelper dbHelper = mDatabasePerProfile.get(profile);
 
        if (dbHelper == null) {
            synchronized (this) {
                dbHelper = new DatabaseHelper(getContext(), getDatabasePath(profile));
                mDatabasePerProfile.put(profile, dbHelper);
            }
        }
 
(similarly in onCreate.)

That .get can receive garbage if it occurs concurrently with that .put. Using a ConcurrentHashMap here is probably overkill, so the solution is simply to use synchronized() correctly.

Whoever wrote this code probably thought "oh, I won't bother taking the lock for the read, I only need to synchronize writes". They were wrong.


"It is a common mistake to assume that synchronization needs to be used only when writing to shared variables; this is simply not true.

"For each mutable state variable that may be accessed by more than one thread, all accesses to that variable must be performed with the same lock held. In this case, we say that the variable is guarded by that lock."

  -- Bloch, Joshua; Goetz, Brian; Peierls, Tim; Bowbeer, Joseph; Holmes, David; Lea, Doug (2006-05-09). Java Concurrency in Practice (Kindle Locations 999-1000). Pearson Education (US). Kindle Edition. 


Patch to follow momentarily. Said patch also removes two redundant log statements.
Comment 1 Richard Newman [:rnewman] 2012-01-01 18:00:18 PST
Created attachment 585237 [details] [diff] [review]
Proposed patch. v1

Builds, but hasn't been tested. (Does Fennec have unit tests for all this stuff? I can't find any by grepping for "Test"...)
Comment 2 Richard Newman [:rnewman] 2012-01-01 20:17:59 PST
Created attachment 585250 [details] [diff] [review]
Part 1: fix concurrency issue and some tidying. v2

Slightly more elegant.
Comment 3 Richard Newman [:rnewman] 2012-01-01 20:23:26 PST
Created attachment 585251 [details] [diff] [review]
Part 2: clean up excessive logging. v1

Given the amount of information embedded in the CP URI (which we print), we can eliminate a whole bunch of redundant logging. For example:

-                    Log.d(LOGTAG, "Query is BOOKMARKS_FOLDER_ID: " + uri);

We *just* printed the URI, which if it was a folder query would contain /bookmarks/folder/#.

I also inverted some of the log choices: for example, we now log only if a custom sort order is provided, rather than concatenating and printing "Using default sort order on query: " every single time.

This change removes hundreds of lines of log spew on every sync, as well as avoiding a *ton* of string concatenation (which will occur even if logging is disabled, of course, because the concatenation occurs at the call site).

This change depends on Part 1, so I'm putting it as a second part in this bug rather than filing a new one. (Think of it as a reversed Part 0!)
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-01 20:40:26 PST
Comment on attachment 585250 [details] [diff] [review]
Part 1: fix concurrency issue and some tidying. v2

Good catch
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-01 20:42:10 PST
Comment on attachment 585251 [details] [diff] [review]
Part 2: clean up excessive logging. v1

>diff --git a/mobile/android/base/db/BrowserProvider.java.in b/mobile/android/base/db/BrowserProvider.java.in

>     public Cursor query(Uri uri, String[] projection, String selection,
>             String[] selectionArgs, String sortOrder) {
>-        Log.d(LOGTAG, "Querying with URI: " + uri);
> 
>         SQLiteDatabase db = getReadableDatabase(uri);

nit: remove the blank line too
Comment 7 Richard Newman [:rnewman] 2012-01-02 08:40:46 PST
OK for me to land on Aurora, mfinkle?
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-01-02 09:25:10 PST
(In reply to Richard Newman [:rnewman] from comment #7)
> OK for me to land on Aurora, mfinkle?

request approval on the patch.

We typically let a patch ride on m-c for at least one nightly before landing on aurora.
Comment 9 Alex Keybl [:akeybl] 2012-01-03 13:43:00 PST
Comment on attachment 585251 [details] [diff] [review]
Part 2: clean up excessive logging. v1

[Triage Comment]
Mobile only - approving for Aurora.
Comment 11 Cristian Nicolae (:xti) 2012-01-31 04:51:06 PST
Verified fixed on:

Mozilla/5.0 (Android;Linux armv7l;rv:11.0a2)Gecko/20120130
Firefox/11.0a2 Fennec/11.0a2
Device: Samsung Galaxy S
OS: Android 2.2

Mozilla/5.0 (Android;Linux armv7l;rv:12.0a1)Gecko/20120130
Firefox/12.0a1 Fennec/12.0a1
Device: Samsung Galaxy S
OS: Android 2.2

Note You need to log in before you can comment on or make changes to this bug.