Skip to content
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.

Sign up for GitHub

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

Jump to bottom

PM-2559 Messaging Rework for Passkey Bug #6282

Merged
merged 7 commits into from
Sep 27, 2023
Merged

PM-2559 Messaging Rework for Passkey Bug #6282

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6330ae7
[PM-2559] Messaging Rework
cagonzalezcs Sep 13, 2023
512e51e
remove unused sendResponse
Jingo88 Sep 13, 2023
621a80e
merged latest from EC-598 fix conflicts
Jingo88 Sep 19, 2023
31cf690
updated browser-api with default type and comments
Jingo88 Sep 25, 2023
d4ec395
put async back to await for lock in app component
Jingo88 Sep 25, 2023
e622101
minor refactor app component in browser
Jingo88 Sep 26, 2023
a08e7cf
removed unused code in app component browser
Jingo88 Sep 27, 2023
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
15 changes: 7 additions & 8 deletions apps/browser/src/autofill/background/context-menus.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,16 @@ export default class ContextMenusBackground {

BrowserApi.messageListener(
"contextmenus.background",
async (
(
msg: { command: string; data: LockedVaultPendingNotificationsItem },
sender: chrome.runtime.MessageSender,
sendResponse: any
sender: chrome.runtime.MessageSender
) => {
if (msg.command === "unlockCompleted" && msg.data.target === "contextmenus.background") {
await this.contextMenuClickedHandler.cipherAction(
msg.data.commandToRetry.msg.data,
msg.data.commandToRetry.sender.tab
);
await BrowserApi.tabSendMessageData(sender.tab, "closeNotificationBar");
this.contextMenuClickedHandler
.cipherAction(msg.data.commandToRetry.msg.data, msg.data.commandToRetry.sender.tab)
.then(() => {
BrowserApi.tabSendMessageData(sender.tab, "closeNotificationBar");
});
Comment on lines +28 to +32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question(non-blocking): why are changing from async/await to promise-then syntax? I usually think that the former is easier to read and more consistent with the majority of the code-base

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coroiu

I agree with your sentiment here. What would make sense is to pull this behavior out to a separate method so we could still consider the async/await approach.

That suggestion aside, I advised Jason on these changes and can give a reason for why, in this instance, we're avoiding using the await keyword.

When using the chrome.runtime.onMessage.addListener API, any callback that is passed to the listener needs to be structured as a syncrhonous callback. This is due to how the sendMessage parameter is structured to respond when a message from chrome.runtime.sendMessage is received.

The issue that we were seeing in this ticket was occurring because we were setting up the callbacks for onMessage.addListener as async methods, which by default return a Promise<void>. Because of that, we were experiencing race conditions where some chrome.runtime.sendMessage calls that were expecting a response from the onMessage listener were receiving undefined values.

This MDN article explains a bit more on the issue - https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage#parameters

I think if @Jingo88 were to pull this behavior out to a private method, they could then place the method in the onMessage callback function without having to use a .then chain.

Copy link
Contributor

@cagonzalezcs cagonzalezcs Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.contextMenuClickedHandler
.cipherAction(msg.data.commandToRetry.msg.data, msg.data.commandToRetry.sender.tab)
.then(() => {
BrowserApi.tabSendMessageData(sender.tab, "closeNotificationBar");
});
handleUnlockCompletedCommand();
[...]
async handleUnlockCompletedCommand() {
await this.contextMenuClickedHandler
.cipherAction(msg.data.commandToRetry.msg.data, msg.data.commandToRetry.sender.tab);
BrowserApi.tabSendMessageData(sender.tab, "closeNotificationBar");
}

Something akin to this.

Copy link
Contributor

@coroiu coroiu Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhhhhhhh of course! I never occured to me that making the functions async would cause them to return a Promise behind the scenes, which would be a truthy value. Someone is 100% going to fall for this again and having no idea what the issue is, this is the third time we're having to address this issue (first me, then Robyn, now Jason)!

Could we maybe force the callbacks to return boolean | void and add a comment explaining why Promises are forbidden?

Copy link
Contributor

@cagonzalezcs cagonzalezcs Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that would be a solid idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving open to make it easier to find in the future

}
}
);
Expand Down
4 changes: 2 additions & 2 deletions apps/browser/src/autofill/background/notification.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ export default class NotificationBackground {

BrowserApi.messageListener(
"notification.background",
async (msg: any, sender: chrome.runtime.MessageSender) => {
await this.processMessage(msg, sender);
(msg: any, sender: chrome.runtime.MessageSender) => {
this.processMessage(msg, sender);
}
);

Expand Down
14 changes: 4 additions & 10 deletions apps/browser/src/background/commands.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,11 @@ export default class CommandsBackground {
}

async init() {
BrowserApi.messageListener(
"commands.background",
async (msg: any, sender: chrome.runtime.MessageSender, sendResponse: any) => {
if (msg.command === "unlockCompleted" && msg.data.target === "commands.background") {
await this.processCommand(
msg.data.commandToRetry.msg.command,
msg.data.commandToRetry.sender
);
}
BrowserApi.messageListener("commands.background", (msg: any) => {
if (msg.command === "unlockCompleted" && msg.data.target === "commands.background") {
this.processCommand(msg.data.commandToRetry.msg.command, msg.data.commandToRetry.sender);
}
);
});

if (chrome && chrome.commands) {
chrome.commands.onCommand.addListener(async (command: string) => {
Expand Down
4 changes: 1 addition & 3 deletions apps/browser/src/background/runtime.background.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,7 @@ export default class RuntimeBackground {
return false;
};

BrowserApi.messageListener("runtime.background", (msg, sender, sendResponse) => {
return backgroundMessageListener(msg, sender, sendResponse);
});
BrowserApi.messageListener("runtime.background", backgroundMessageListener);
coroiu marked this conversation as resolved.
Show resolved Hide resolved
if (this.main.popupOnlyContext) {
(window as any).bitwardenBackgroundMessageListener = backgroundMessageListener;
}
Expand Down
22 changes: 9 additions & 13 deletions apps/browser/src/platform/browser/browser-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,20 +217,16 @@ export class BrowserApi {

static messageListener(
name: string,
callback: (message: any, sender: chrome.runtime.MessageSender, response: any) => unknown
callback: (
message: any,
sender: chrome.runtime.MessageSender,
sendResponse: any
) => boolean | void
) {
chrome.runtime.onMessage.addListener(
(msg: any, sender: chrome.runtime.MessageSender, sendResponse: any) => {
const messageResponse = callback(msg, sender, sendResponse);

if (!messageResponse) {
return false;
}

Promise.resolve(messageResponse);
return true;
}
);
// updated to pass synchronous callbacks to addListener.
// Will not pass async methods because they will default return a Promoise<void>
// this causes race conditions in Firefox when a runtime.sendMessage listener receives undefined
chrome.runtime.onMessage.addListener(callback);

// Keep track of all the events registered in a Safari popup so we can remove
// them when the popup gets unloaded, otherwise we cause a memory leak
Expand Down
7 changes: 3 additions & 4 deletions apps/browser/src/platform/decorators/session-sync-observable/session-syncer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,9 @@ export class SessionSyncer {

private listenForUpdates() {
// This is an unawaited promise, but it will be executed asynchronously in the background.
BrowserApi.messageListener(
this.updateMessageCommand,
async (message) => await this.updateFromMessage(message)
);
BrowserApi.messageListener(this.updateMessageCommand, (message) => {
this.updateFromMessage(message);
});
}

async updateFromMessage(message: any) {
Expand Down
18 changes: 9 additions & 9 deletions apps/browser/src/popup/app.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ export class AppComponent implements OnInit, OnDestroy {
// Component states must not persist between closing and reopening the popup, otherwise they become dead objects
// Clear them aggressively to make sure this doesn't occur
await this.clearComponentStates();
const userIdFromState = await this.stateService.getUserId();
coroiu marked this conversation as resolved.
Show resolved Hide resolved

this.stateService.activeAccount$.pipe(takeUntil(this.destroy$)).subscribe((userId) => {
this.activeUserId = userId;
Expand All @@ -80,11 +81,7 @@ export class AppComponent implements OnInit, OnDestroy {
window.onkeypress = () => this.recordActivity();
});

(window as any).bitwardenPopupMainMessageListener = async (
msg: any,
sender: any,
sendResponse: any
) => {
const bitwardenPopupMainMessageListener = (msg: any, sender: any) => {
if (msg.command === "doneLoggingOut") {
this.authService.logOut(async () => {
if (msg.expired) {
Expand All @@ -103,14 +100,16 @@ export class AppComponent implements OnInit, OnDestroy {
} else if (msg.command === "authBlocked") {
this.router.navigate(["home"]);
} else if (msg.command === "locked") {
if (msg.userId == null || msg.userId === (await this.stateService.getUserId())) {
// the second argument here was written for account switching
// this is not available on browser yet.
if (msg.userId == null || msg.userId === userIdFromState) {
this.router.navigate(["lock"]);
}
} else if (msg.command === "showDialog") {
await this.ngZone.run(() => this.showDialog(msg));
this.showDialog(msg);
} else if (msg.command === "showNativeMessagingFinterprintDialog") {
// TODO: Should be refactored to live in another service.
await this.ngZone.run(() => this.showNativeMessagingFingerprintDialog(msg));
this.showNativeMessagingFingerprintDialog(msg);
} else if (msg.command === "showToast") {
this.showToast(msg);
} else if (msg.command === "reloadProcess") {
Expand All @@ -133,7 +132,8 @@ export class AppComponent implements OnInit, OnDestroy {
}
};

BrowserApi.messageListener("app.component", (window as any).bitwardenPopupMainMessageListener);
(window as any).bitwardenPopupMainMessageListener = bitwardenPopupMainMessageListener;
BrowserApi.messageListener("app.component", bitwardenPopupMainMessageListener);

// eslint-disable-next-line rxjs/no-async-subscribe
this.router.events.pipe(takeUntil(this.destroy$)).subscribe(async (event) => {
Expand Down
Loading