From 0ae24ebfa48b517b848152b5343267cbed955828 Mon Sep 17 00:00:00 2001 From: Agi Sferro Date: Mon, 15 Mar 2021 17:04:41 +0000 Subject: [PATCH] Bug 1697503 - Remove GeckoAppShell.KillAnyZombies. r=mossop,droeh Before this patch, when the profile is locked on Android, we would call "ps", parse the human-readable output and kill any other Gecko process that we could find. But this is completely unnecessary, as we know exactly that the PID of the process holding the lock is. In this patch we just kill the process holding the lock since this is equivalent to the previous behavior. Differential Revision: https://phabricator.services.mozilla.com/D106186 --- .../src/androidTest/AndroidManifest.xml | 4 + .../geckoview/test/ProfileLockedTest.kt | 115 ++++++++++++++++++ .../test/TestProfileLockService.java | 99 +++++++++++++++ .../java/org/mozilla/gecko/GeckoAppShell.java | 95 --------------- toolkit/profile/ProfileUnlockerAndroid.cpp | 37 ++++++ toolkit/profile/ProfileUnlockerAndroid.h | 26 ++++ toolkit/profile/moz.build | 3 + toolkit/profile/nsProfileLock.cpp | 19 ++- toolkit/profile/nsProfileLock.h | 3 +- toolkit/xre/nsAppRunner.cpp | 9 +- 10 files changed, 304 insertions(+), 106 deletions(-) create mode 100644 mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProfileLockedTest.kt create mode 100644 mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestProfileLockService.java create mode 100644 toolkit/profile/ProfileUnlockerAndroid.cpp create mode 100644 toolkit/profile/ProfileUnlockerAndroid.h diff --git a/mobile/android/geckoview/src/androidTest/AndroidManifest.xml b/mobile/android/geckoview/src/androidTest/AndroidManifest.xml index f5cc3c2981f9..5083bf175394 100644 --- a/mobile/android/geckoview/src/androidTest/AndroidManifest.xml +++ b/mobile/android/geckoview/src/androidTest/AndroidManifest.xml @@ -43,6 +43,10 @@ android:exported="false" android:process=":gecko"> + + + + diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProfileLockedTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProfileLockedTest.kt new file mode 100644 index 000000000000..38fc26ffcf65 --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/ProfileLockedTest.kt @@ -0,0 +1,115 @@ +package org.mozilla.geckoview.test + +import android.content.ComponentName +import android.content.Context +import android.content.Intent +import android.content.ServiceConnection +import android.os.* +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.filters.MediumTest +import androidx.test.platform.app.InstrumentationRegistry +import org.hamcrest.CoreMatchers.equalTo +import org.junit.Assert.assertThat +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import org.mozilla.geckoview.GeckoResult +import java.io.File + + +@RunWith(AndroidJUnit4::class) +@MediumTest +class ProfileLockedTest { + private val targetContext + get() = InstrumentationRegistry.getInstrumentation().targetContext + + @get:Rule + var folder = TemporaryFolder() + + /** + * Starts GeckoRuntime in the process given in input, and waits for the MESSAGE_PAGE_STOP + * event that's fired when the first GeckoSession receives the onPageStop event. + * + * We wait for a page load to make sure that everything started up correctly (as opposed + * to quitting during the startup procedure). + */ + class RuntimeInstance( + private val context: Context, + private val service: Class, + private val profileFolder: File) : ServiceConnection { + + var isConnected = false + var disconnected = GeckoResult() + var started = GeckoResult() + var quitted = GeckoResult() + + override fun onServiceConnected(name: ComponentName, binder: IBinder) { + val handler = object : Handler(Looper.getMainLooper()) { + override fun handleMessage(msg: Message) { + when (msg.what) { + TestProfileLockService.MESSAGE_PAGE_STOP -> started.complete(null) + TestProfileLockService.MESSAGE_QUIT -> { + quitted.complete(null) + // No reason to keep the service around anymore + context.unbindService(this@RuntimeInstance) + } + } + } + } + + val messenger = Messenger(handler) + val service = Messenger(binder) + + val msg = Message.obtain(null, TestProfileLockService.MESSAGE_REGISTER) + msg.replyTo = messenger + service.send(msg) + + isConnected = true + } + + override fun onServiceDisconnected(name: ComponentName?) { + isConnected = false + context.unbindService(this) + disconnected.complete(null) + } + + fun start() { + val intent = Intent(context, service) + intent.putExtra("args", "-profile ${profileFolder.absolutePath}") + + context.bindService(intent, this, Context.BIND_AUTO_CREATE) + } + } + + @Test + fun profileLocked() { + val profileFolder = folder.newFolder("profile-locked-test") + + val runtime0 = RuntimeInstance(targetContext, + TestProfileLockService.p0::class.java, + profileFolder) + val runtime1 = RuntimeInstance(targetContext, + TestProfileLockService.p1::class.java, + profileFolder) + + // Start the first runtime and wait until it's ready + runtime0.start() + runtime0.started.pollDefault() + + assertThat("The service should be connected now", runtime0.isConnected, equalTo(true)) + + // Now start a _second_ runtime with the same profile folder, this will kill the first + // runtime + runtime1.start() + + // Wait for the first runtime to disconnect + runtime0.disconnected.pollDefault() + + // GeckoRuntime will quit after killing the offending process + runtime1.quitted.pollDefault() + + assertThat("The service shouldn't be connected anymore", + runtime0.isConnected, equalTo(false)) + } +} diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestProfileLockService.java b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestProfileLockService.java new file mode 100644 index 000000000000..4bfcc853c731 --- /dev/null +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/TestProfileLockService.java @@ -0,0 +1,99 @@ +package org.mozilla.geckoview.test; + +import android.app.Service; +import android.content.Intent; +import android.os.IBinder; +import android.os.Message; +import android.os.Messenger; +import android.os.RemoteException; + +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; + +import org.mozilla.geckoview.GeckoRuntime; +import org.mozilla.geckoview.GeckoRuntimeSettings; +import org.mozilla.geckoview.GeckoSession; + +public class TestProfileLockService extends Service implements + GeckoSession.ProgressDelegate, + GeckoRuntime.Delegate { + public static final class p0 extends TestProfileLockService {} + public static final class p1 extends TestProfileLockService {} + + // Used by the client to register themselves + public static final int MESSAGE_REGISTER = 1; + // Sent when the first page load completes + public static final int MESSAGE_PAGE_STOP = 2; + // Sent when GeckoRuntime exits + public static final int MESSAGE_QUIT = 3; + + private GeckoRuntime mRuntime; + + private Messenger mClient; + + private class Handler extends android.os.Handler { + @Override + public void handleMessage(@NonNull Message msg) { + switch (msg.what) { + case MESSAGE_REGISTER: + mClient = msg.replyTo; + return; + default: + throw new IllegalStateException("Unknown message: " + msg.what); + } + } + } + + final Messenger mMessenger = new Messenger(new Handler()); + + @Override + public void onShutdown() { + final Message message = Message.obtain(null, MESSAGE_QUIT); + try { + mClient.send(message); + } catch (RemoteException ex) { + throw new RuntimeException(ex); + } + } + + @Override + public void onPageStop(@NonNull GeckoSession session, boolean success) { + final Message message = Message.obtain(null, MESSAGE_PAGE_STOP); + try { + mClient.send(message); + } catch (RemoteException ex) { + throw new RuntimeException(ex); + } + } + + @Override + public void onDestroy() { + // Sometimes the service doesn't die on it's own so we need to kill it here. + System.exit(0); + } + + @Nullable + @Override + public IBinder onBind(Intent intent) { + // Request to be killed as soon as the client unbinds. + stopSelf(); + + if (mRuntime != null) { + // We only expect one client + throw new RuntimeException("Multiple clients !?"); + } + + GeckoRuntimeSettings settings = new GeckoRuntimeSettings.Builder() + .extras(intent.getExtras()) + .build(); + mRuntime = GeckoRuntime.create(getApplicationContext(), settings); + + mRuntime.setDelegate(this); + + final GeckoSession session = new GeckoSession(); + session.setProgressDelegate(this); + session.open(mRuntime); + + return mMessenger.getBinder(); + } +} diff --git a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java index 95cd2397ad1b..93e2186cdcf4 100644 --- a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java +++ b/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java @@ -5,10 +5,7 @@ package org.mozilla.gecko; -import java.io.BufferedReader; import java.io.File; -import java.io.FileReader; -import java.io.InputStreamReader; import java.net.Proxy; import java.nio.ByteBuffer; import java.util.Iterator; @@ -21,7 +18,6 @@ import org.mozilla.gecko.annotation.WrapForJNI; import org.mozilla.gecko.util.HardwareCodecCapabilityUtils; import org.mozilla.gecko.util.HardwareUtils; import org.mozilla.gecko.util.InputDeviceUtils; -import org.mozilla.gecko.util.IOUtils; import org.mozilla.gecko.util.ProxySelector; import org.mozilla.gecko.util.ThreadUtils; import org.mozilla.geckoview.BuildConfig; @@ -1068,94 +1064,6 @@ public class GeckoAppShell { return result; } - @WrapForJNI(calledFrom = "gecko") - public static void killAnyZombies() { - GeckoProcessesVisitor visitor = new GeckoProcessesVisitor() { - @Override - public boolean callback(final int pid) { - if (pid != android.os.Process.myPid()) - android.os.Process.killProcess(pid); - return true; - } - }; - - EnumerateGeckoProcesses(visitor); - } - - interface GeckoProcessesVisitor { - boolean callback(int pid); - } - - private static void EnumerateGeckoProcesses(final GeckoProcessesVisitor visiter) { - int pidColumn = -1; - int userColumn = -1; - - Process ps = null; - InputStreamReader inputStreamReader = null; - BufferedReader in = null; - try { - // run ps and parse its output - ps = Runtime.getRuntime().exec("ps"); - inputStreamReader = new InputStreamReader(ps.getInputStream()); - in = new BufferedReader(inputStreamReader, 2048); - - String headerOutput = in.readLine(); - - // figure out the column offsets. We only care about the pid and user fields - StringTokenizer st = new StringTokenizer(headerOutput); - - int tokenSoFar = 0; - while (st.hasMoreTokens()) { - String next = st.nextToken(); - if (next.equalsIgnoreCase("PID")) - pidColumn = tokenSoFar; - else if (next.equalsIgnoreCase("USER")) - userColumn = tokenSoFar; - tokenSoFar++; - } - - // alright, the rest are process entries. - String psOutput = null; - while ((psOutput = in.readLine()) != null) { - String[] split = psOutput.split("\\s+"); - if (split.length <= pidColumn || split.length <= userColumn) - continue; - int uid = android.os.Process.getUidForName(split[userColumn]); - if (uid == android.os.Process.myUid() && - !split[split.length - 1].equalsIgnoreCase("ps")) { - int pid = Integer.parseInt(split[pidColumn]); - boolean keepGoing = visiter.callback(pid); - if (keepGoing == false) - break; - } - } - } catch (Exception e) { - Log.w(LOGTAG, "Failed to enumerate Gecko processes.", e); - } finally { - IOUtils.safeStreamClose(in); - IOUtils.safeStreamClose(inputStreamReader); - if (ps != null) { - ps.destroy(); - } - } - } - - public static String getAppNameByPID(final int pid) { - BufferedReader cmdlineReader = null; - String path = "/proc/" + pid + "/cmdline"; - try { - File cmdlineFile = new File(path); - if (!cmdlineFile.exists()) - return ""; - cmdlineReader = new BufferedReader(new FileReader(cmdlineFile)); - return cmdlineReader.readLine().trim(); - } catch (Exception ex) { - return ""; - } finally { - IOUtils.safeStreamClose(cmdlineReader); - } - } - @WrapForJNI(calledFrom = "gecko") private static byte[] getIconForExtension(final String aExt, final int iconSize) { try { @@ -1451,9 +1359,6 @@ public class GeckoAppShell { @WrapForJNI(calledFrom = "gecko") private static boolean unlockProfile() { - // Try to kill any zombie Fennec's that might be running - GeckoAppShell.killAnyZombies(); - // Then force unlock this profile final GeckoProfile profile = GeckoThread.getActiveProfile(); if (profile != null) { diff --git a/toolkit/profile/ProfileUnlockerAndroid.cpp b/toolkit/profile/ProfileUnlockerAndroid.cpp new file mode 100644 index 000000000000..58836c7b8e63 --- /dev/null +++ b/toolkit/profile/ProfileUnlockerAndroid.cpp @@ -0,0 +1,37 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "ProfileUnlockerAndroid.h" +#include "nsPrintfCString.h" +#include + +namespace mozilla { + +ProfileUnlockerAndroid::ProfileUnlockerAndroid(const pid_t aPid) : mPid(aPid) {} + +ProfileUnlockerAndroid::~ProfileUnlockerAndroid() {} + +NS_IMPL_ISUPPORTS(ProfileUnlockerAndroid, nsIProfileUnlocker) + +NS_IMETHODIMP +ProfileUnlockerAndroid::Unlock(uint32_t aSeverity) { + if (aSeverity != FORCE_QUIT) { + return NS_ERROR_NOT_IMPLEMENTED; + } + + NS_WARNING(nsPrintfCString("Process %d has the profile " + "lock, will try to kill it.", + mPid) + .get()); + if (mPid == getpid()) { + NS_ERROR("Lock held by current process !?"); + return NS_ERROR_FAILURE; + } + if (kill(mPid, SIGKILL) != 0) { + NS_WARNING("Could not kill process."); + } + return NS_OK; +} + +} // namespace mozilla diff --git a/toolkit/profile/ProfileUnlockerAndroid.h b/toolkit/profile/ProfileUnlockerAndroid.h new file mode 100644 index 000000000000..a8dc33bcb5c3 --- /dev/null +++ b/toolkit/profile/ProfileUnlockerAndroid.h @@ -0,0 +1,26 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef ProfileUnlockerAndroid_h +#define ProfileUnlockerAndroid_h + +#include "nsIProfileUnlocker.h" + +namespace mozilla { + +class ProfileUnlockerAndroid final : public nsIProfileUnlocker { + public: + NS_DECL_ISUPPORTS + NS_DECL_NSIPROFILEUNLOCKER + + explicit ProfileUnlockerAndroid(const pid_t aPid); + + private: + ~ProfileUnlockerAndroid(); + pid_t mPid; +}; + +} // namespace mozilla + +#endif // ProfileUnlockerAndroid_h diff --git a/toolkit/profile/moz.build b/toolkit/profile/moz.build index 2dbf6f9b9b62..db4df9abb2a7 100644 --- a/toolkit/profile/moz.build +++ b/toolkit/profile/moz.build @@ -25,6 +25,9 @@ UNIFIED_SOURCES += ["nsProfileLock.cpp"] if CONFIG["OS_ARCH"] == "WINNT": UNIFIED_SOURCES += ["ProfileUnlockerWin.cpp"] +if CONFIG["OS_TARGET"] == "Android": + UNIFIED_SOURCES += ["ProfileUnlockerAndroid.cpp"] + UNIFIED_SOURCES += ["nsToolkitProfileService.cpp"] LOCAL_INCLUDES += [ diff --git a/toolkit/profile/nsProfileLock.cpp b/toolkit/profile/nsProfileLock.cpp index 01818d32e6f7..8cbb67deb5d8 100644 --- a/toolkit/profile/nsProfileLock.cpp +++ b/toolkit/profile/nsProfileLock.cpp @@ -7,6 +7,8 @@ #include "nsCOMPtr.h" #include "nsQueryObject.h" #include "nsString.h" +#include "nsPrintfCString.h" +#include "nsDebug.h" #if defined(XP_WIN) # include "ProfileUnlockerWin.h" @@ -17,6 +19,10 @@ # include #endif +#if defined(MOZ_WIDGET_ANDROID) +# include "ProfileUnlockerAndroid.h" +#endif + #ifdef XP_UNIX # include # include @@ -182,7 +188,8 @@ void nsProfileLock::FatalSignalHandler(int signo _exit(signo); } -nsresult nsProfileLock::LockWithFcntl(nsIFile* aLockFile) { +nsresult nsProfileLock::LockWithFcntl(nsIFile* aLockFile, + nsIProfileUnlocker** aUnlocker) { nsresult rv = NS_OK; nsAutoCString lockFilePath; @@ -211,6 +218,14 @@ nsresult nsProfileLock::LockWithFcntl(nsIFile* aLockFile) { mLockFileDesc = -1; rv = NS_ERROR_FAILURE; } else if (fcntl(mLockFileDesc, F_SETLK, &lock) == -1) { +# ifdef MOZ_WIDGET_ANDROID + MOZ_ASSERT(aUnlocker); + RefPtr unlocker( + new mozilla::ProfileUnlockerAndroid(testlock.l_pid)); + nsCOMPtr unlockerInterface(do_QueryObject(unlocker)); + unlockerInterface.forget(aUnlocker); +# endif + close(mLockFileDesc); mLockFileDesc = -1; @@ -479,7 +494,7 @@ nsresult nsProfileLock::Lock(nsIFile* aProfileDir, // First, try locking using fcntl. It is more reliable on // a local machine, but may not be supported by an NFS server. - rv = LockWithFcntl(lockFile); + rv = LockWithFcntl(lockFile, aUnlocker); if (NS_SUCCEEDED(rv)) { // Check to see whether there is a symlink lock held by an older // Firefox build, and also place our own symlink lock --- but diff --git a/toolkit/profile/nsProfileLock.h b/toolkit/profile/nsProfileLock.h index cfa8fa524f63..bc7335324e38 100644 --- a/toolkit/profile/nsProfileLock.h +++ b/toolkit/profile/nsProfileLock.h @@ -83,7 +83,8 @@ class nsProfileLock ); static PRCList mPidLockList; - nsresult LockWithFcntl(nsIFile* aLockFile); + nsresult LockWithFcntl(nsIFile* aLockFile, + nsIProfileUnlocker** aUnlocker = nullptr); /** * @param aHaveFcntlLock if true, we've already acquired an fcntl lock so this diff --git a/toolkit/xre/nsAppRunner.cpp b/toolkit/xre/nsAppRunner.cpp index 13fafe0821f0..457fde4d466e 100644 --- a/toolkit/xre/nsAppRunner.cpp +++ b/toolkit/xre/nsAppRunner.cpp @@ -2430,7 +2430,7 @@ static ReturnAbortOnError ProfileLockedDialog(nsIFile* aProfileDir, if (aUnlocker) { int32_t button; #ifdef MOZ_WIDGET_ANDROID - java::GeckoAppShell::KillAnyZombies(); + // On Android we always kill the process if the lock is still being held button = 0; #else const uint32_t flags = (nsIPromptService::BUTTON_TITLE_IS_STRING * @@ -2457,15 +2457,8 @@ static ReturnAbortOnError ProfileLockedDialog(nsIFile* aProfileDir, return LaunchChild(false, true); } } else { -#ifdef MOZ_WIDGET_ANDROID - if (java::GeckoAppShell::UnlockProfile()) { - return NS_LockProfilePath(aProfileDir, aProfileLocalDir, nullptr, - aResult); - } -#else rv = ps->Alert(nullptr, killTitle.get(), killMessage.get()); NS_ENSURE_SUCCESS_LOG(rv, rv); -#endif } return NS_ERROR_ABORT;