From 43d9ac81f7722bb539ee67023f10b9f43abbf202 Mon Sep 17 00:00:00 2001 From: Dianne Hackborn Date: Wed, 25 Aug 2010 15:06:25 -0700 Subject: Fix a fun bug with multiple service bindings from an activity. There was a flaw in the service management, when the same activity is doing a bindService() for the same service IBinder. In this case the activity would correctly keep a list of all generated connections, however some other data structures would assume there is only one connection per IBinder, and thus only remember the last. When that last connection was unbound, the service would be destroyed since it thought there were no more connections. Then when the activity was finished, it would try to destroy the service again and end up with an ANR because the service was already gone and would not respond. Change-Id: I59bde38bc24e78147b90b0a7cd525c2a1d20489f --- .../android/server/am/ActivityManagerService.java | 329 ++++++++++++--------- .../java/com/android/server/am/ServiceRecord.java | 12 +- 2 files changed, 198 insertions(+), 143 deletions(-) (limited to 'services/java') diff --git a/services/java/com/android/server/am/ActivityManagerService.java b/services/java/com/android/server/am/ActivityManagerService.java index b37cd89..c316074 100644 --- a/services/java/com/android/server/am/ActivityManagerService.java +++ b/services/java/com/android/server/am/ActivityManagerService.java @@ -609,8 +609,8 @@ public final class ActivityManagerService extends ActivityManagerNative implemen * All currently bound service connections. Keys are the IBinder of * the client's IServiceConnection. */ - final HashMap mServiceConnections - = new HashMap(); + final HashMap> mServiceConnections + = new HashMap>(); /** * List of services that we have been asked to start, @@ -7376,12 +7376,14 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (mServiceConnections.size() > 0) { if (needSep) pw.println(" "); pw.println(" Connection bindings to services:"); - Iterator it + Iterator> it = mServiceConnections.values().iterator(); while (it.hasNext()) { - ConnectionRecord r = it.next(); - pw.print(" * "); pw.println(r); - r.dump(pw, " "); + ArrayList r = it.next(); + for (int i=0; i 0) { - Iterator jt + Iterator> jt = r.connections.values().iterator(); while (jt.hasNext()) { - ConnectionRecord c = jt.next(); - if (c.binding.client != app) { - try { - //c.conn.connected(r.className, null); - } catch (Exception e) { - // todo: this should be asynchronous! - Slog.w(TAG, "Exception thrown disconnected servce " - + r.shortName - + " from app " + app.processName, e); + ArrayList cl = jt.next(); + for (int i=0; i 0; if (hasClients) { @@ -7753,6 +7760,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen ServiceRecord sr = mStoppingServices.get(i); if (sr.app == app) { mStoppingServices.remove(i); + if (DEBUG_SERVICE) Slog.v(TAG, "killServices remove stopping " + sr); } } @@ -8016,11 +8024,15 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (r.app != null && r.app.persistent) { info.flags |= ActivityManager.RunningServiceInfo.FLAG_PERSISTENT_PROCESS; } - for (ConnectionRecord conn : r.connections.values()) { - if (conn.clientLabel != 0) { - info.clientPackage = conn.binding.client.info.packageName; - info.clientLabel = conn.clientLabel; - break; + + for (ArrayList connl : r.connections.values()) { + for (int i=0; i conn : r.connections.values()) { + for (int i=0; i 0) { try { ServiceRecord.StartItem si = r.pendingStarts.remove(0); - if (DEBUG_SERVICE) Slog.v(TAG, "Sending arguments to service: " - + r.name + " " + r.intent + " args=" + si.intent); + if (DEBUG_SERVICE) Slog.v(TAG, "Sending arguments to: " + + r + " " + r.intent + " args=" + si.intent); if (si.intent == null) { // If somehow we got a dummy start at the front, then // just drop it here. @@ -8248,6 +8262,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen grantUriPermissionUncheckedFromIntentLocked(si.targetPermissionUid, r.packageName, si.intent, si); } + if (DEBUG_SERVICE) Slog.v(TAG, ">>> EXECUTING start of " + r); bumpServiceExecutingLocked(r); if (!oomAdjusted) { oomAdjusted = true; @@ -8264,6 +8279,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen } catch (RemoteException e) { // Remote process gone... we'll let the normal cleanup take // care of this. + if (DEBUG_SERVICE) Slog.v(TAG, "Crashed while scheduling start: " + r); break; } catch (Exception e) { Slog.w(TAG, "Unexpected exception", e); @@ -8280,9 +8296,9 @@ public final class ActivityManagerService extends ActivityManagerNative implemen } if ((!i.requested || rebind) && i.apps.size() > 0) { try { + if (DEBUG_SERVICE) Slog.v(TAG, ">>> EXECUTING bind of " + r + + " in " + i + ": shouldUnbind=" + i.hasBound); bumpServiceExecutingLocked(r); - if (DEBUG_SERVICE) Slog.v(TAG, "Connecting binding " + i - + ": shouldUnbind=" + i.hasBound); r.app.thread.scheduleBindService(r, i.intent.getIntent(), rebind); if (!rebind) { i.requested = true; @@ -8290,6 +8306,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen i.hasBound = true; i.doRebind = false; } catch (RemoteException e) { + if (DEBUG_SERVICE) Slog.v(TAG, "Crashed while binding " + r); return false; } } @@ -8316,13 +8333,12 @@ public final class ActivityManagerService extends ActivityManagerNative implemen r.restartTime = r.lastActivity = SystemClock.uptimeMillis(); app.services.add(r); + if (DEBUG_SERVICE) Slog.v(TAG, ">>> EXECUTING create of " + r + " " + r.intent); bumpServiceExecutingLocked(r); updateLruProcessLocked(app, true, true); boolean created = false; try { - if (DEBUG_SERVICE) Slog.v(TAG, "Scheduling start service: " - + r.name + " " + r.intent); mStringBuilder.setLength(0); r.intent.getIntent().toShortString(mStringBuilder, false, true); EventLog.writeEvent(EventLogTags.AM_CREATE_SERVICE, @@ -8482,8 +8498,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen return true; } - if (DEBUG_SERVICE) Slog.v(TAG, "Bringing up service " + r.name - + " " + r.intent); + if (DEBUG_SERVICE) Slog.v(TAG, "Bringing up " + r + " " + r.intent); // We are now bringing the service up, so no longer in the // restarting state. @@ -8534,27 +8549,30 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (!force) { // XXX should probably keep a count of the number of auto-create // connections directly in the service. - Iterator it = r.connections.values().iterator(); + Iterator> it = r.connections.values().iterator(); while (it.hasNext()) { - ConnectionRecord cr = it.next(); - if ((cr.flags&Context.BIND_AUTO_CREATE) != 0) { - return; + ArrayList cr = it.next(); + for (int i=0; i it = r.connections.values().iterator(); + Iterator> it = r.connections.values().iterator(); while (it.hasNext()) { - ConnectionRecord c = it.next(); - try { - // todo: shouldn't be a synchronous call! - c.conn.connected(r.name, null); - } catch (Exception e) { - Slog.w(TAG, "Failure disconnecting service " + r.name + - " to connection " + c.conn.asBinder() + - " (in " + c.binding.client.processName + ")", e); + ArrayList c = it.next(); + for (int i=0; i>> EXECUTING bring down unbind of " + r + + " for " + ibr); bumpServiceExecutingLocked(r); updateOomAdjLocked(r.app); ibr.hasBound = false; @@ -8582,15 +8602,13 @@ public final class ActivityManagerService extends ActivityManagerNative implemen } } - if (DEBUG_SERVICE) Slog.v(TAG, "Bringing down service " + r.name - + " " + r.intent); + if (DEBUG_SERVICE) Slog.v(TAG, "Bringing down " + r + " " + r.intent); EventLog.writeEvent(EventLogTags.AM_DESTROY_SERVICE, System.identityHashCode(r), r.shortName, (r.app != null) ? r.app.pid : -1); mServices.remove(r.name); mServicesByIntent.remove(r.intent); - if (localLOGV) Slog.v(TAG, "BRING DOWN SERVICE: " + r.shortName); r.totalRestartCount = 0; unscheduleServiceRestartLocked(r); @@ -8599,8 +8617,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen for (int i=0; i>> EXECUTING stop of " + r, here); + } bumpServiceExecutingLocked(r); mStoppingServices.add(r); updateOomAdjLocked(r.app); @@ -8636,11 +8656,11 @@ public final class ActivityManagerService extends ActivityManagerNative implemen updateServiceForegroundLocked(r.app, false); } else { if (DEBUG_SERVICE) Slog.v( - TAG, "Removed service that has no process: " + r.shortName); + TAG, "Removed service that has no process: " + r); } } else { if (DEBUG_SERVICE) Slog.v( - TAG, "Removed service that is not running: " + r.shortName); + TAG, "Removed service that is not running: " + r); } } @@ -8675,8 +8695,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen int targetPermissionUid = checkGrantUriPermissionFromIntentLocked( callingUid, r.packageName, service); if (unscheduleServiceRestartLocked(r)) { - if (DEBUG_SERVICE) Slog.v(TAG, "START SERVICE WHILE RESTART PENDING: " - + r.shortName); + if (DEBUG_SERVICE) Slog.v(TAG, "START SERVICE WHILE RESTART PENDING: " + r); } r.startRequested = true; r.callStart = false; @@ -8970,7 +8989,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (unscheduleServiceRestartLocked(s)) { if (DEBUG_SERVICE) Slog.v(TAG, "BIND SERVICE WHILE RESTART PENDING: " - + s.shortName); + + s); } AppBindRecord b = s.retrieveAppBindingLocked(service, callerApp); @@ -8978,7 +8997,12 @@ public final class ActivityManagerService extends ActivityManagerNative implemen connection, flags, clientLabel, clientIntent); IBinder binder = connection.asBinder(); - s.connections.put(binder, c); + ArrayList clist = s.connections.get(binder); + if (clist == null) { + clist = new ArrayList(); + s.connections.put(binder, clist); + } + clist.add(c); b.connections.add(c); if (activity != null) { if (activity.connections == null) { @@ -8987,7 +9011,12 @@ public final class ActivityManagerService extends ActivityManagerNative implemen activity.connections.add(c); } b.client.connections.add(c); - mServiceConnections.put(binder, c); + clist = mServiceConnections.get(binder); + if (clist == null) { + clist = new ArrayList(); + mServiceConnections.put(binder, clist); + } + clist.add(c); if ((flags&Context.BIND_AUTO_CREATE) != 0) { s.lastActivity = SystemClock.uptimeMillis(); @@ -9038,7 +9067,13 @@ public final class ActivityManagerService extends ActivityManagerNative implemen IBinder binder = c.conn.asBinder(); AppBindRecord b = c.binding; ServiceRecord s = b.service; - s.connections.remove(binder); + ArrayList clist = s.connections.get(binder); + if (clist != null) { + clist.remove(c); + if (clist.size() == 0) { + s.connections.remove(binder); + } + } b.connections.remove(c); if (c.activity != null && c.activity != skipAct) { if (c.activity.connections != null) { @@ -9048,7 +9083,13 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (b.client != skipApp) { b.client.connections.remove(c); } - mServiceConnections.remove(binder); + clist = mServiceConnections.get(binder); + if (clist != null) { + clist.remove(c); + if (clist.size() == 0) { + mServiceConnections.remove(binder); + } + } if (b.connections.size() == 0) { b.intent.apps.remove(b.client); @@ -9059,6 +9100,8 @@ public final class ActivityManagerService extends ActivityManagerNative implemen if (s.app != null && s.app.thread != null && b.intent.apps.size() == 0 && b.intent.hasBound) { try { + if (DEBUG_SERVICE) Slog.v(TAG, ">>> EXECUTING unbind of " + s + + " from " + b); bumpServiceExecutingLocked(s); updateOomAdjLocked(s.app); b.intent.hasBound = false; @@ -9081,8 +9124,8 @@ public final class ActivityManagerService extends ActivityManagerNative implemen synchronized (this) { IBinder binder = connection.asBinder(); if (DEBUG_SERVICE) Slog.v(TAG, "unbindService: conn=" + binder); - ConnectionRecord r = mServiceConnections.get(binder); - if (r == null) { + ArrayList clist = mServiceConnections.get(binder); + if (clist == null) { Slog.w(TAG, "Unbind failed: could not find connection for " + connection.asBinder()); return false; @@ -9090,11 +9133,14 @@ public final class ActivityManagerService extends ActivityManagerNative implemen final long origId = Binder.clearCallingIdentity(); - removeConnectionLocked(r, null, null); + while (clist.size() > 0) { + ConnectionRecord r = clist.get(0); + removeConnectionLocked(r, null, null); - if (r.binding.service.app != null) { - // This could have made the service less important. - updateOomAdjLocked(r.binding.service.app); + if (r.binding.service.app != null) { + // This could have made the service less important. + updateOomAdjLocked(r.binding.service.app); + } } Binder.restoreCallingIdentity(origId); @@ -9117,7 +9163,7 @@ public final class ActivityManagerService extends ActivityManagerNative implemen final long origId = Binder.clearCallingIdentity(); - if (DEBUG_SERVICE) Slog.v(TAG, "PUBLISHING SERVICE " + r.name + if (DEBUG_SERVICE) Slog.v(TAG, "PUBLISHING " + r + " " + intent + ": " + service); if (r != null) { Intent.FilterComparison filter @@ -9128,26 +9174,29 @@ public final class ActivityManagerService extends ActivityManagerNative implemen b.requested = true; b.received = true; if (r.connections.size() > 0) { - Iterator it + Iterator> it = r.connections.values().iterator(); while (it.hasNext()) { - ConnectionRecord c = it.next(); - if (!filter.equals(c.binding.intent.intent)) { - if (DEBUG_SERVICE) Slog.v( - TAG, "Not publishing to: " + c); - if (DEBUG_SERVICE) Slog.v( - TAG, "Bound intent: " + c.binding.intent.intent); - if (DEBUG_SERVICE) Slog.v( - TAG, "Published intent: " + intent); - continue; - } - if (DEBUG_SERVICE) Slog.v(TAG, "Publishing to: " + c); - try { - c.conn.connected(r.name, service); - } catch (Exception e) { - Slog.w(TAG, "Failure sending service " + r.name + - " to connection " + c.conn.asBinder() + - " (in " + c.binding.client.processName + ")", e); + ArrayList clist = it.next(); + for (int i=0; i 0 && (adj > FOREGROUND_APP_ADJ || schedGroup == Process.THREAD_GROUP_BG_NONINTERACTIVE)) { - Iterator kt + Iterator> kt = s.connections.values().iterator(); while (kt.hasNext() && adj > FOREGROUND_APP_ADJ) { - // XXX should compute this based on the max of - // all connected clients. - ConnectionRecord cr = kt.next(); - if (cr.binding.client == app) { - // Binding to ourself is not interesting. - continue; - } - if ((cr.flags&Context.BIND_AUTO_CREATE) != 0) { - ProcessRecord client = cr.binding.client; - int myHiddenAdj = hiddenAdj; - if (myHiddenAdj > client.hiddenAdj) { - if (client.hiddenAdj >= VISIBLE_APP_ADJ) { - myHiddenAdj = client.hiddenAdj; - } else { - myHiddenAdj = VISIBLE_APP_ADJ; - } + ArrayList clist = kt.next(); + for (int i=0; i FOREGROUND_APP_ADJ; i++) { + // XXX should compute this based on the max of + // all connected clients. + ConnectionRecord cr = clist.get(i); + if (cr.binding.client == app) { + // Binding to ourself is not interesting. + continue; } - int clientAdj = computeOomAdjLocked( - client, myHiddenAdj, TOP_APP, true); - if (adj > clientAdj) { - adj = clientAdj >= VISIBLE_APP_ADJ - ? clientAdj : VISIBLE_APP_ADJ; - if (!client.hidden) { - app.hidden = false; + if ((cr.flags&Context.BIND_AUTO_CREATE) != 0) { + ProcessRecord client = cr.binding.client; + int myHiddenAdj = hiddenAdj; + if (myHiddenAdj > client.hiddenAdj) { + if (client.hiddenAdj >= VISIBLE_APP_ADJ) { + myHiddenAdj = client.hiddenAdj; + } else { + myHiddenAdj = VISIBLE_APP_ADJ; + } + } + int clientAdj = computeOomAdjLocked( + client, myHiddenAdj, TOP_APP, true); + if (adj > clientAdj) { + adj = clientAdj >= VISIBLE_APP_ADJ + ? clientAdj : VISIBLE_APP_ADJ; + if (!client.hidden) { + app.hidden = false; + } + app.adjType = "service"; + app.adjTypeCode = ActivityManager.RunningAppProcessInfo + .REASON_SERVICE_IN_USE; + app.adjSource = cr.binding.client; + app.adjTarget = s.name; } + if ((cr.flags&Context.BIND_NOT_FOREGROUND) == 0) { + if (client.curSchedGroup == Process.THREAD_GROUP_DEFAULT) { + schedGroup = Process.THREAD_GROUP_DEFAULT; + } + } + } + ActivityRecord a = cr.activity; + //if (a != null) { + // Slog.i(TAG, "Connection to " + a ": state=" + a.state); + //} + if (a != null && adj > FOREGROUND_APP_ADJ && + (a.state == ActivityState.RESUMED + || a.state == ActivityState.PAUSING)) { + adj = FOREGROUND_APP_ADJ; + schedGroup = Process.THREAD_GROUP_DEFAULT; + app.hidden = false; app.adjType = "service"; app.adjTypeCode = ActivityManager.RunningAppProcessInfo .REASON_SERVICE_IN_USE; - app.adjSource = cr.binding.client; + app.adjSource = a; app.adjTarget = s.name; } - if ((cr.flags&Context.BIND_NOT_FOREGROUND) == 0) { - if (client.curSchedGroup == Process.THREAD_GROUP_DEFAULT) { - schedGroup = Process.THREAD_GROUP_DEFAULT; - } - } - } - ActivityRecord a = cr.activity; - //if (a != null) { - // Slog.i(TAG, "Connection to " + a ": state=" + a.state); - //} - if (a != null && adj > FOREGROUND_APP_ADJ && - (a.state == ActivityState.RESUMED - || a.state == ActivityState.PAUSING)) { - adj = FOREGROUND_APP_ADJ; - schedGroup = Process.THREAD_GROUP_DEFAULT; - app.hidden = false; - app.adjType = "service"; - app.adjTypeCode = ActivityManager.RunningAppProcessInfo - .REASON_SERVICE_IN_USE; - app.adjSource = a; - app.adjTarget = s.name; } } } diff --git a/services/java/com/android/server/am/ServiceRecord.java b/services/java/com/android/server/am/ServiceRecord.java index 255fbe3..d5b050b 100644 --- a/services/java/com/android/server/am/ServiceRecord.java +++ b/services/java/com/android/server/am/ServiceRecord.java @@ -72,8 +72,8 @@ class ServiceRecord extends Binder { final HashMap bindings = new HashMap(); // All active bindings to the service. - final HashMap connections - = new HashMap(); + final HashMap> connections + = new HashMap>(); // IBinder -> ConnectionRecord of all bound clients ProcessRecord app; // where this service is running or null. @@ -296,10 +296,12 @@ class ServiceRecord extends Binder { } if (connections.size() > 0) { pw.print(prefix); pw.println("All Connections:"); - Iterator it = connections.values().iterator(); + Iterator> it = connections.values().iterator(); while (it.hasNext()) { - ConnectionRecord c = it.next(); - pw.print(prefix); pw.print(" "); pw.println(c); + ArrayList c = it.next(); + for (int i=0; i