1161342 - StrictMode violation in org.mozilla.gecko.widget.ActivityChooserModel.hasOtherSyncClients

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Data Providers, defect)

Description

[Nick Alexander :nalexander [he/him]]

E StrictMode(8407)            A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
E StrictMode(8407)            java.lang.Throwable: Explicit termination method 'close' not called
E StrictMode(8407)            	at dalvik.system.CloseGuard.open(CloseGuard.java:184)
E StrictMode(8407)            	at android.database.sqlite.SQLiteDatabase.openInner(SQLiteDatabase.java:892)
E StrictMode(8407)            	at android.database.sqlite.SQLiteDatabase.open(SQLiteDatabase.java:859)
E StrictMode(8407)            	at android.database.sqlite.SQLiteDatabase.openDatabase(SQLiteDatabase.java:696)
E StrictMode(8407)            	at android.app.ContextImpl.openOrCreateDatabase(ContextImpl.java:1219)
E StrictMode(8407)            	at android.content.ContextWrapper.openOrCreateDatabase(ContextWrapper.java:242)
E StrictMode(8407)            	at android.database.sqlite.SQLiteOpenHelper.getDatabaseLocked(SQLiteOpenHelper.java:224)
E StrictMode(8407)            	at android.database.sqlite.SQLiteOpenHelper.getReadableDatabase(SQLiteOpenHelper.java:188)
E StrictMode(8407)            	at org.mozilla.gecko.sync.repositories.android.CachedSQLiteOpenHelper.getCachedReadableDatabase(CachedSQLiteOpenHelper.java:27)
E StrictMode(8407)            	at org.mozilla.gecko.sync.repositories.android.ClientsDatabase.fetchAllClients(ClientsDatabase.java:235)
E StrictMode(8407)            	at org.mozilla.gecko.sync.repositories.android.ClientsDatabaseAccessor.clientsCount(ClientsDatabaseAccessor.java:147)
E StrictMode(8407)            	at org.mozilla.gecko.widget.ActivityChooserModel.hasOtherSyncClients(ActivityChooserModel.java:1315)
E StrictMode(8407)            	at org.mozilla.gecko.widget.ActivityChooserModel.loadActivitiesIfNeeded(ActivityChooserModel.java:781)
E StrictMode(8407)            	at org.mozilla.gecko.widget.ActivityChooserModel.ensureConsistentState(ActivityChooserModel.java:719)
E StrictMode(8407)            	at org.mozilla.gecko.widget.ActivityChooserModel.getDistinctActivityCountInHistory(ActivityChooserModel.java:689)
E StrictMode(8407)            	at org.mozilla.gecko.widget.GeckoActionProvider.onCreateActionView(GeckoActionProvider.java:122)
E StrictMode(8407)            	at org.mozilla.gecko.menu.GeckoMenuItem.getActionView(GeckoMenuItem.java:144)
E StrictMode(8407)            	at org.mozilla.gecko.menu.GeckoMenu.addActionItem(GeckoMenu.java:200)
E StrictMode(8407)            	at org.mozilla.gecko.menu.GeckoMenu.onShowAsActionChanged(GeckoMenu.java:575)
E StrictMode(8407)            	at org.mozilla.gecko.menu.GeckoMenuItem.setActionProvider(GeckoMenuItem.java:273)
E StrictMode(8407)            	at org.mozilla.gecko.BrowserApp.onCreateOptionsMenu(BrowserApp.java:2882)
E StrictMode(8407)            	at org.mozilla.gecko.GeckoApp.onCreatePanelMenu(GeckoApp.java:412)
E StrictMode(8407)            	at org.mozilla.gecko.GeckoApp.getMenuPanel(GeckoApp.java:339)
E StrictMode(8407)            	at org.mozilla.gecko.GeckoApp.onMenuOpened(GeckoApp.java:437)
E StrictMode(8407)            	at com.android.internal.policy.impl.PhoneWindow.openPanel(PhoneWindow.java:684)
E StrictMode(8407)            	at com.android.internal.policy.impl.PhoneWindow.openPanel(PhoneWindow.java:642)
E StrictMode(8407)            	at android.app.Activity.openOptionsMenu(Activity.java:2913)
E StrictMode(8407)            	at org.mozilla.gecko.BrowserApp.openOptionsMenu(BrowserApp.java:2916)
E StrictMode(8407)            	at org.mozilla.gecko.GeckoApp.onKeyDown(GeckoApp.java:509)
E StrictMode(8407)            	at org.mozilla.gecko.BrowserApp.onKeyDown(BrowserApp.java:655)
E StrictMode(8407)            	at android.view.KeyEvent.dispatch(KeyEvent.java:2891)
E StrictMode(8407)            	at android.app.Activity.dispatchKeyEvent(Activity.java:2456)
E StrictMode(8407)            	at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchKeyEvent(PhoneWindow.java:2211)
E StrictMode(8407)            	at android.view.ViewRootImpl$ViewPostImeInputStage.processKeyEvent(ViewRootImpl.java:4562)
E StrictMode(8407)            	at android.view.ViewRootImpl$ViewPostImeInputStage.onProcess(ViewRootImpl.java:4538)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4143)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:4193)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:4162)
E StrictMode(8407)            	at android.view.ViewRootImpl$AsyncInputStage.forward(ViewRootImpl.java:4247)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:4170)
E StrictMode(8407)            	at android.view.ViewRootImpl$AsyncInputStage.apply(ViewRootImpl.java:4304)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4143)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:4193)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:4162)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:4170)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4143)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:4193)
E StrictMode(8407)            	at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java
...

Comment 1

[Nick Alexander :nalexander [he/him]]

A good first bug!  The issue is that db is not closed at [1].  Throw a `finally { db.close() }` or similar in their.

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java#1314

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This was caused by bug 1122302.

Comment 3

[p.calligaris.code]

Attachment: Proposed Patch

Now the database is closed using db.close(). 'finally' is executed if there is a return inside a 'try' block, so I think this should work, but now ClientsDatabaseAccessor db is no more a final variable, since I assign twice a value to it.

Is this right or not?

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8616455 [details] [diff] [review]
Proposed Patch

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

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +1314,5 @@
>          }
> +		try {
> +        	db = new ClientsDatabaseAccessor(mContext);
> +        	return db.clientsCount() > 0;
> +        } finally {

This is more complicated than it needs to be. It should be as simple as:

   final ClientsDatabaseAccessor db = new ClientsDatabaseAccessor(mContext);
   try {
       return db.clientsCount() > 0;
   } finally {
       db.close();
   }

The constructor can never return null, and if it throws there's nothing to clean up, so we don't need that call to be inside the try block.

Comment 5

[p.calligaris.code]

Attachment: Updated Proposed Patch

Yes, it makes more sense. I updated the patch.

Comment 6

[Nicholas Rosbrook [:enr0n]]

What's up with this bug? No activity since the last patch proposal -- is is it okay to work on?

Comment 7

[Nick Alexander :nalexander [he/him]]

(In reply to Nicholas Rosbrook [:enr0n] from comment #6)
> What's up with this bug? No activity since the last patch proposal -- is is
> it okay to work on?

Looks like a mix-up with flags not getting set, and then me and rnewman not seeing it.

However, I'd love it if you'd investigate and update the ticket.  Be aware that this area has changed a lot -- see Bug 1159020 -- and this may have been addressed by that work.  I'm NIing Ahmed to help mentor 'cuz he's the local expert here.

Comment 8

[Nicholas Rosbrook [:enr0n]]

From what I gathered looking through Bug 1159020 , this issue was sort of solved by replacing ClientsDatabaseAccessor with TabsAccessor within ActivityChooserModel. I'd like to see what Ahmed thinks on the issue but I think that's the case.

Comment 9

[Ahmed Khalil]

Hi Nicholas,

Yes, You are right I think that the issue is already solved after Bug 1159020, as you can see in [1], the cursor is closed in the finally block.


[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ActivityChooserModel.java#1328

Comment 10

[Nick Alexander :nalexander [he/him]]

Ahmed -- thanks for verifying.  Being able to call on a local expert to investigate things is really valuable to me.

Now, enr0n, are you interested in a different ticket?  I see you've already squashed one ticket; Bug 1119915 might be a nice follow-up.  Add a needinfo=nalexander flag on that ticket if you're interested.