fix: a popup save no longer undoes a network switch the background saved during it (closes #641)
The popup's saveState() and the background's updateState() each read the stored record, change it and write it back, and each was ordered only within its own context, so a write by one could land between the other's read and write and be lost. Both now hold the browser's lock (navigator.locks, shared by every page and the background except a page in a Firefox private window) from the read to the write, through withLock() in src/shared/browserApi.js; the recovery screen's reset takes it too. The lock also orders a page's own saves, so both per-context queues are gone. The unit tests get a small lock from a Jest setup file. Model: opus-5-5
This commit is contained in:
@@ -158,7 +158,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const {
|
||||
Interface,
|
||||
|
||||
@@ -189,7 +189,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const {
|
||||
BaseWallet,
|
||||
|
||||
+4
-1
@@ -367,11 +367,14 @@ async function leaveTransactionDetail(page) {
|
||||
await openAddressDetail(page);
|
||||
}
|
||||
|
||||
// Back out to Home from wherever the previous test finished.
|
||||
// Back out to Home from wherever the previous test finished, and wait for the
|
||||
// save that navigation makes, so a test reading the stored stack next reads
|
||||
// the one Home left.
|
||||
async function goHome(page) {
|
||||
await leaveTransactionDetail(page);
|
||||
await page.click("#btn-address-back");
|
||||
await visible(page, "#view-main");
|
||||
await waitForPersisted(page, "currentView", "main", "back on Home");
|
||||
}
|
||||
|
||||
// One field of the popup's state as it was actually persisted, read out of
|
||||
|
||||
@@ -151,7 +151,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const { state } = require("../src/shared/state");
|
||||
const { NETWORKS } = require("../src/shared/networks");
|
||||
|
||||
@@ -97,7 +97,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const { state } = require("../src/shared/state");
|
||||
const send = require("../src/popup/views/send");
|
||||
|
||||
@@ -151,7 +151,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const { refreshBalances } = require("../src/shared/balances");
|
||||
const { state } = require("../src/shared/state");
|
||||
|
||||
@@ -146,7 +146,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const { Transaction, Wallet, formatEther } = require("ethers");
|
||||
const { refreshBalances } = require("../src/shared/balances");
|
||||
|
||||
@@ -0,0 +1,95 @@
|
||||
// A popup save reads the stored record, merges this page's changes into it
|
||||
// and writes it back. The background's write of a network the user approved
|
||||
// used to be able to land between that read and that write, and the popup's
|
||||
// write then put the old network back
|
||||
// (https://git.eeqj.de/sneak/AutistMask/issues/641). Every write of the record
|
||||
// now holds one lock from its read to its write.
|
||||
//
|
||||
// The popup and the background run here as the real modules over one shared
|
||||
// storage stub, and share the lock the way the extension's pages and its
|
||||
// background share the browser's (tests/support/navigatorLocks.js).
|
||||
|
||||
const { makeStorageStub } = require("./support/storageStub");
|
||||
|
||||
function loadContexts(storage) {
|
||||
jest.resetModules();
|
||||
globalThis.chrome = { storage: { local: storage.local } };
|
||||
return {
|
||||
popup: require("../src/shared/state"),
|
||||
background: require("../src/background/state"),
|
||||
applyChainSwitchFields: require("../src/shared/chainSwitchFields")
|
||||
.applyChainSwitchFields,
|
||||
};
|
||||
}
|
||||
|
||||
// Lets every pending promise callback run, and then some.
|
||||
function nextMacrotask() {
|
||||
return new Promise((resolve) => setImmediate(resolve));
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
delete globalThis.chrome;
|
||||
jest.restoreAllMocks();
|
||||
});
|
||||
|
||||
test("a network the background switches to during a popup save stays stored", async () => {
|
||||
const storage = makeStorageStub({ autistmask: { networkId: "sepolia" } });
|
||||
const { popup, background, applyChainSwitchFields } = loadContexts(storage);
|
||||
await popup.loadState();
|
||||
|
||||
// The popup navigates, so its save has a change of its own to write.
|
||||
popup.state.currentView = "settings";
|
||||
|
||||
// The popup's save has read the record and is about to write it back when
|
||||
// the user's approval of a site's switch to mainnet reaches the
|
||||
// background. Without the lock the background's whole update runs before
|
||||
// the next macrotask; with it, the update waits for the popup's write.
|
||||
let backgroundSwitch;
|
||||
const realSet = storage.local.set.getMockImplementation();
|
||||
storage.local.set.mockImplementationOnce(async (items) => {
|
||||
backgroundSwitch = background.updateState((fresh) =>
|
||||
applyChainSwitchFields(fresh, "mainnet"),
|
||||
);
|
||||
await nextMacrotask();
|
||||
return realSet(items);
|
||||
});
|
||||
|
||||
await popup.saveState();
|
||||
await backgroundSwitch;
|
||||
|
||||
const stored = storage.read("autistmask");
|
||||
expect(stored.networkId).toBe("mainnet");
|
||||
expect(stored.currentView).toBe("settings");
|
||||
});
|
||||
|
||||
test("after a save that failed, the next save still keeps a network the background switches to during it", async () => {
|
||||
jest.spyOn(console, "error").mockImplementation(() => {});
|
||||
const storage = makeStorageStub({ autistmask: { networkId: "sepolia" } });
|
||||
const { popup, background, applyChainSwitchFields } = loadContexts(storage);
|
||||
await popup.loadState();
|
||||
popup.state.currentView = "settings";
|
||||
|
||||
// The first save's write is refused, so it gives the lock back having
|
||||
// failed.
|
||||
storage.local.set.mockRejectedValueOnce(new Error("quota exceeded"));
|
||||
await expect(popup.saveState()).rejects.toThrow("quota exceeded");
|
||||
|
||||
// The next save is between its read and its write when the background
|
||||
// switches to mainnet, as in the test above.
|
||||
let backgroundSwitch;
|
||||
const realSet = storage.local.set.getMockImplementation();
|
||||
storage.local.set.mockImplementationOnce(async (items) => {
|
||||
backgroundSwitch = background.updateState((fresh) =>
|
||||
applyChainSwitchFields(fresh, "mainnet"),
|
||||
);
|
||||
await nextMacrotask();
|
||||
return realSet(items);
|
||||
});
|
||||
|
||||
await popup.saveState();
|
||||
await backgroundSwitch;
|
||||
|
||||
const stored = storage.read("autistmask");
|
||||
expect(stored.networkId).toBe("mainnet");
|
||||
expect(stored.currentView).toBe("settings");
|
||||
});
|
||||
@@ -0,0 +1,26 @@
|
||||
// `navigator.locks` for the unit tests, which run on node, where there is none.
|
||||
//
|
||||
// One lock per name: granted in the order it was asked for, and let go when
|
||||
// the callback settles, whether it resolved or rejected. That is what the
|
||||
// browser's own does for the one call the extension makes,
|
||||
// `navigator.locks.request(name, callback)`. Every module a test file loads
|
||||
// shares it, as every page of the extension and its background share the
|
||||
// browser's.
|
||||
//
|
||||
// Run before each test file by `setupFiles` in package.json.
|
||||
|
||||
// For each name, a promise that settles once the last request for it has
|
||||
// let the lock go.
|
||||
const released = new Map();
|
||||
|
||||
function request(name, callback) {
|
||||
const previous = released.get(name) || Promise.resolve();
|
||||
const result = previous.then(() => callback({ name, mode: "exclusive" }));
|
||||
released.set(
|
||||
name,
|
||||
result.catch(() => {}),
|
||||
);
|
||||
return result;
|
||||
}
|
||||
|
||||
globalThis.navigator = { locks: { request } };
|
||||
@@ -120,7 +120,7 @@ global.document = {
|
||||
body: { prepend() {}, appendChild() {} },
|
||||
addEventListener() {},
|
||||
};
|
||||
global.navigator = { clipboard: { writeText() {} } };
|
||||
global.navigator.clipboard = { writeText() {} };
|
||||
|
||||
const { AbiCoder, parseUnits } = require("ethers");
|
||||
const { fetchTokenBalances } = require("../src/shared/balances");
|
||||
|
||||
Reference in New Issue
Block a user