The default bug view has changed. See this FAQ.

Concurrency issue in BrowserProvider.java.in

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

unspecified
Firefox 12
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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"...)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #585237 - Flags: review?(mark.finkle)
(Assignee)

Comment 2

5 years ago
Created attachment 585250 [details] [diff] [review]
Part 1: fix concurrency issue and some tidying. v2

Slightly more elegant.
Attachment #585237 - Attachment is obsolete: true
Attachment #585237 - Flags: review?(mark.finkle)
Attachment #585250 - Flags: review?(mark.finkle)
(Assignee)

Comment 3

5 years ago
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!)
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+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/ac280e7eb3e6
https://hg.mozilla.org/mozilla-central/rev/44d992ccc97a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Updated

5 years ago
Attachment #585250 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #585251 - Flags: approval-mozilla-aurora?

Comment 9

5 years ago
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+

Updated

5 years ago
Attachment #585250 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 10

5 years ago
Thanks Alex.

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec606ae92bd6
https://hg.mozilla.org/releases/mozilla-aurora/rev/f19189eb513d
status-firefox11: --- → fixed
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
status-firefox12: --- → fixed
You need to log in before you can comment on or make changes to this bug.