Skip to content

fix: ctrl + C does nothing when using firefox-android without auto-reload or during activity start #2523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: Ctrl+C now works in firefox-android target while awaiting to sta…
…rt activity
  • Loading branch information
luskaner committed Oct 10, 2022
commit 8fb99d4c6672dbaf9a59f3a13d1f95cf8be9d751
47 changes: 36 additions & 11 deletions src/extension-runners/firefox-android.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,13 @@ export class FirefoxAndroidExtensionRunner {
// push the profile preferences to the remote profile dir.
await this.adbPrepareProfileDir();

const stdin = this.params.stdin || process.stdin;

if (isTTY(stdin)) {
readline.emitKeypressEvents(stdin);
setRawMode(stdin, true);
}

// NOTE: running Firefox for Android on the Android Emulator can be
// pretty slow, we can run the following 3 steps in parallel to speed up
// it a bit.
Expand All @@ -164,6 +171,11 @@ export class FirefoxAndroidExtensionRunner {
this.adbDiscoveryAndForwardRDPUnixSocket(),
]);

if (isTTY(stdin)) {
// Restore mode so Ctrl-C can be used to kill the process.
setRawMode(stdin, false);
}

// Connect to RDP socket on the local tcp server, install all the pushed extension
// and keep track of the built and installed extension by extension sourceDir.
await this.rdpInstallExtensions();
Expand Down Expand Up @@ -499,12 +511,30 @@ export class FirefoxAndroidExtensionRunner {

log.debug(`Using profile ${deviceProfileDir} (ignored by Fenix)`);

await adbUtils.startFirefoxAPK(
selectedAdbDevice,
selectedFirefoxApk,
firefoxApkComponent,
deviceProfileDir,
);
const stdin = this.params.stdin || process.stdin;

const handleCtrlC = (str, key) => {
if (key.ctrl && key.name === 'c') {
adbUtils.setUserAbortStartActivity(true);
}
};

if (isTTY(stdin)) {
stdin.on('keypress', handleCtrlC);
}

try {
await adbUtils.startFirefoxAPK(
selectedAdbDevice,
selectedFirefoxApk,
firefoxApkComponent,
deviceProfileDir,
);
} finally {
if (isTTY(stdin)) {
stdin.removeListener('keypress', handleCtrlC);
}
}
}

async buildAndPushExtension(sourceDir: string) {
Expand Down Expand Up @@ -579,9 +609,6 @@ export class FirefoxAndroidExtensionRunner {
// TODO: use noInput property to decide if we should
// disable direct keypress handling.
if (isTTY(stdin)) {
readline.emitKeypressEvents(stdin);
setRawMode(stdin, true);

stdin.on('keypress', handleCtrlC);
}

Expand All @@ -598,8 +625,6 @@ export class FirefoxAndroidExtensionRunner {
} finally {
if (isTTY(stdin)) {
stdin.removeListener('keypress', handleCtrlC);
// Restore mode so Ctrl-C can be used to kill the process.
setRawMode(stdin, false);
}
}

Expand Down
30 changes: 29 additions & 1 deletion src/util/adb.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ export default class ADBUtils {
// while it is still executing.
userAbortDiscovery: boolean;

// Toggled when the user wants to abort the Start Activity loop
// while it is still executing.
userAbortStartActivity: boolean;

constructor(params: ADBUtilsParams) {
this.params = params;

Expand All @@ -81,6 +85,7 @@ export default class ADBUtils {
this.artifactsDirMap = new Map();

this.userAbortDiscovery = false;
this.userAbortStartActivity = false;
}

runShellCommand(
Expand Down Expand Up @@ -343,20 +348,43 @@ export default class ADBUtils {
// the following to: `${apk}/${apk}.${apkComponent}`
const component = `${apk}/${apkComponent}`;

await wrapADBCall(async () => {
let exception;
wrapADBCall(async () => {
await adbClient.getDevice(deviceId).startActivity({
wait: true,
action: 'android.activity.MAIN',
component,
extras,
});
}).then(() => {
exception = null;
}).catch((err) => {
exception = err;
});

// Wait for the activity to be started.
while (exception === undefined) {
if (this.userAbortStartActivity) {
throw new UsageError(
'Exiting Firefox Start Activity on user request'
);
}
await new Promise((resolve) => setTimeout(resolve, 1000));
}

if (exception) {
throw exception;
}
}

setUserAbortDiscovery(value: boolean) {
this.userAbortDiscovery = value;
}

setUserAbortStartActivity(value: boolean) {
this.userAbortStartActivity = value;
}

async discoverRDPUnixSocket(
deviceId: string, apk: string,
{maxDiscoveryTime, retryInterval}: DiscoveryParams = {}
Expand Down
42 changes: 42 additions & 0 deletions tests/unit/test-extension-runners/test.firefox-android.js
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ function prepareSelectedDeviceAndAPKParams(
clearArtifactsDir: sinon.spy(() => Promise.resolve()),
detectOrRemoveOldArtifacts: sinon.spy(() => Promise.resolve(true)),
setUserAbortDiscovery: sinon.spy(() => {}),
setUserAbortStartActivity: sinon.spy(() => {}),
ensureRequiredAPKRuntimePermissions: sinon.spy(() => Promise.resolve()),
...adbOverrides,
};
Expand Down Expand Up @@ -786,6 +787,47 @@ describe('util/extension-runners/firefox-android', () => {
);
});

it('allows user to exit while waiting to start activity',
async () => {
const {
params, fakeADBUtils,
} = prepareSelectedDeviceAndAPKParams();

fakeADBUtils.startFirefoxAPK = sinon.spy(async () => {
fakeStdin.emit('keypress', 'c', {name: 'c', ctrl: true});

sinon.assert.calledOnce(fakeADBUtils.setUserAbortStartActivity);
sinon.assert.calledWith(
fakeADBUtils.setUserAbortStartActivity
);

// Reject the expected error, if all the assertion passes.
throw new UsageError('fake user exit');
});

const fakeStdin = createFakeStdin();

params.stdin = fakeStdin;

let actualError;

try {
const runnerInstance = new FirefoxAndroidExtensionRunner(params);
await runnerInstance.run();
} catch (error) {
actualError = error;
} finally {
fakeStdin.emit('keypress', 'c', {name: 'c', ctrl: true});
}

assert.instanceOf(actualError, UsageError);
assert.match(
actualError && actualError.message,
/fake user exit/
);
});


it('rejects on Android Firefox Debugger discovery timeouts',
async () => {
const {
Expand Down
27 changes: 27 additions & 0 deletions tests/unit/test-util/test.adb.js
Original file line number Diff line number Diff line change
Expand Up @@ -1097,6 +1097,33 @@ describe('utils/adb', () => {
}
);
});

it('rejects an UsageError on setUserAbortStartActivity call', async () => {
const adb = getFakeADBKit({
adbDevice: {
startActivity: sinon.spy(() => Promise.resolve()),
},
adbkitUtil: {
readAll: sinon.spy(() => Promise.resolve(Buffer.from('\n'))),
},
});
const adbUtils = new ADBUtils({adb});

adbUtils.setUserAbortStartActivity(true);

const promise = adbUtils.startFirefoxAPK(
'device1',
'org.mozilla.firefox_mybuild',
undefined, // firefoxApkComponent/*
'/fake/custom/profile/path'
);

await assert.isRejected(promise, UsageError);
await assert.isRejected(
promise,
'Exiting Firefox Start Activity on user request'
);
});
});

describe('discoverRDPUnixSocket', () => {
Expand Down