Closed Bug 714565 Opened 8 years ago Closed 8 years ago

Concurrency issue in BrowserProvider.java.in

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- fixed
firefox12 --- fixed

People

(Reporter: rnewman, Assigned: rnewman)

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Proposed patch. v1 (obsolete) — Splinter Review
Builds, but hasn't been tested. (Does Fennec have unit tests for all this stuff? I can't find any by grepping for "Test"...)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #585237 - Flags: review?(mark.finkle)
Slightly more elegant.
Attachment #585237 - Attachment is obsolete: true
Attachment #585237 - Flags: review?(mark.finkle)
Attachment #585250 - Flags: review?(mark.finkle)
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!)
Attachment #585251 - Flags: review?(mark.finkle)
Comment on attachment 585250 [details] [diff] [review]
Part 1: fix concurrency issue and some tidying. v2

Good catch
Attachment #585250 - Flags: review?(mark.finkle) → review+
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
Attachment #585251 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/ac280e7eb3e6
https://hg.mozilla.org/mozilla-central/rev/44d992ccc97a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
OK for me to land on Aurora, mfinkle?
(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.
Attachment #585250 - Flags: approval-mozilla-aurora?
Attachment #585251 - Flags: approval-mozilla-aurora?
Comment on attachment 585251 [details] [diff] [review]
Part 2: clean up excessive logging. v1

[Triage Comment]
Mobile only - approving for Aurora.
Attachment #585251 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #585250 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.