852787 - StrictMode policy violation from LocalBrowserDB.getUrlForKeyword()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P4)

Description

[Chris Peterson [:cpeterson]]

While running the Robocop test "TEST_PATH=testLoad make mochitest-robotium", I saw the following StrictMode violation when the test was entering a URL into the address bar:

D/StrictMode(29467): StrictMode policy violation; ~duration=16 ms: android.os.StrictMode$StrictModeDiskReadViolation: policy=31 violation=2
D/StrictMode(29467): 	at android.os.StrictMode$AndroidBlockGuardPolicy.onReadFromDisk(StrictMode.java:1107)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteConnection.applyBlockGuardPolicy(SQLiteConnection.java:1034)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteConnection.executeForCursorWindow(SQLiteConnection.java:835)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteSession.executeForCursorWindow(SQLiteSession.java:836)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteQuery.fillWindow(SQLiteQuery.java:62)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteCursor.fillWindow(SQLiteCursor.java:143)
D/StrictMode(29467): 	at android.database.sqlite.SQLiteCursor.getCount(SQLiteCursor.java:133)
D/StrictMode(29467): 	at android.content.ContentResolver.query(ContentResolver.java:390)
D/StrictMode(29467): 	at android.content.ContentResolver.query(ContentResolver.java:315)
D/StrictMode(29467): 	at org.mozilla.gecko.db.LocalBrowserDB.getUrlForKeyword(LocalBrowserDB.java:520)
D/StrictMode(29467): 	at org.mozilla.gecko.db.BrowserDB.getUrlForKeyword(BrowserDB.java:189)
D/StrictMode(29467): 	at org.mozilla.gecko.AwesomeBar.openUserEnteredAndFinish(AwesomeBar.java:394)
D/StrictMode(29467): 	at org.mozilla.gecko.AwesomeBar.access$300(AwesomeBar.java:50)
D/StrictMode(29467): 	at org.mozilla.gecko.AwesomeBar$3.onKeyPreIme(AwesomeBar.java:159)
D/StrictMode(29467): 	at org.mozilla.gecko.CustomEditText.onKeyPreIme(CustomEditText.java:34)
D/StrictMode(29467): 	at android.view.View.dispatchKeyEventPreIme(View.java:7179)
D/StrictMode(29467): 	at android.view.ViewGroup.dispatchKeyEventPreIme(ViewGroup.java:1338)
D/StrictMode(29467): 	at android.view.ViewGroup.dispatchKeyEventPreIme(ViewGroup.java:1338)
D/StrictMode(29467): 	at android.view.ViewGroup.dispatchKeyEventPreIme(ViewGroup.java:1338)
D/StrictMode(29467): 	at android.view.ViewGroup.dispatchKeyEventPreIme(ViewGroup.java:1338)
D/StrictMode(29467): 	at android.view.ViewGroup.dispatchKeyEventPreIme(ViewGroup.java:1338)
D/StrictMode(29467): 	at android.view.ViewRootImpl.deliverKeyEvent(ViewRootImpl.java:3606)
D/StrictMode(29467): 	at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:3161)
D/StrictMode(29467): 	at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:4292)
D/StrictMode(29467): 	at android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:4271)
D/StrictMode(29467): 	at android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:4363)
D/StrictMode(29467): 	at android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:179)
D/StrictMode(29467): 	at android.os.MessageQueue.nativePollOnce(Native Method)
D/StrictMode(29467): 	at android.os.MessageQueue.next(MessageQueue.java:125)
D/StrictMode(29467): 	at android.os.Looper.loop(Looper.java:124)
D/StrictMode(29467): 	at android.app.ActivityThread.main(ActivityThread.java:5226)
D/StrictMode(29467): 	at java.lang.reflect.Method.invokeNative(Native Method)
D/StrictMode(29467): 	at java.lang.reflect.Method.invoke(Method.java:511)
D/StrictMode(29467): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:795)
D/StrictMode(29467): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:562)
D/StrictMode(29467): 	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch1: limit keyword searches to text that looks like a search query

First patch is just a small optimization. We should never even attempt to do a DB query for a keyword match if the text (url entered by the user) does not look like a search query. 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/StringUtils.java#28

Text that looks like a URL is not a keyword. The new domain/hostname auto-completion code will make this situation (text is a URL) happen even more.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2: move keyword searches to background thread

This patch builds on the first. It moves the keyword search to the background thread. Since "url" becomes final, I had to tweak the logic a tiny bit.

Testing shows no more strict violation. keyword searches do work and we fallback to google searches correctly too, if a keyword is not defined.

Comment 3

[Brian Nicholson (:bnicholson)]

Comment on attachment 734316 [details] [diff] [review]
patch 2: move keyword searches to background thread

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

This will always do a replace on the string for %s, but we should only do a replace if we have a keyword URL. r+ with that fixed.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Made the change to skip the replace if keywordUrl is null
https://hg.mozilla.org/integration/mozilla-inbound/rev/deef421f9169
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba48781b74e4

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 734205 [details] [diff] [review]
patch1: limit keyword searches to text that looks like a search query

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Accessing the DB on the UI thread can cause hangs and locks, like the test failures linked in bug 856621
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): some risk if I made a stupid mistake in the patch. Seems to work well and reduces the DB work too.
String or IDL/UUID changes made by this patch: none

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 734316 [details] [diff] [review]
patch 2: move keyword searches to background thread

[Approval Request Comment]
Same as above

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/deef421f9169
https://hg.mozilla.org/mozilla-central/rev/ba48781b74e4

Comment 8

[bhavana bajaj [:bajaj]]

Approving in favor of fixing intermittent failures given in bug 856621 and may help avoid hangs on UI thread DB access.

Early enough in the beta cycle where the risk is manageable . Please watch for any new regressions this may cause due to less baketime on m-c aurora.

Comment 9

[bhavana bajaj [:bajaj]]

Comment on attachment 734205 [details] [diff] [review]
patch1: limit keyword searches to text that looks like a search query

Please land asap to get this into for Beta 2 going to build shortly.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cd024a6af563
https://hg.mozilla.org/releases/mozilla-aurora/rev/2fe1b366eae9

https://hg.mozilla.org/releases/mozilla-beta/rev/e9a9918f8c76
https://hg.mozilla.org/releases/mozilla-beta/rev/f2e3a0d44669