fix: prevent duplicate saves by using both local variable and session storage

[marcustyphoon]

See #101 (comment) for background.

This semi-reverts the removal of the handledRequests map object, used in the background script scope to prevent multiple onBeforeRequest handlers from triggering on the same ask (same requestId) as well as to store the timestamp values for reference by the error handlers.

The use of chrome.storage.session that's added to handle the MV3 non-persistent background script is changed to be an addition on top of the previous storage method, handling only the case where the background script is unloaded during a network request, rather than a replacement.

This should prevent asks being saved multiple times. I think. Probably.

Reading this commit-by-commit may be more informative than looking at the diff as a whole.

Testing steps

• If possible, check out the MV3 branch without this PR and confirm that duplicate asks are saved when sending an ask on tumblr.com/blogname.
• Check out this PR.
• Confirm that duplicate asks are not saved when sending an ask on tumblr.com/blogname.
• Confirm that asks are saved with an error when opening an ask to a blog you control on tumblr.com/blogname, disabling asks on that blog, and sending the ask.
• As in the test commit, break the handledRequests.get(requestId) lines in the error handling code to simulate the background script having been closed and reopened. Confirm that asks are still saved with an error when opening an ask to a blog you control on tumblr.com/blogname, disabling asks on that blog, and sending the ask.

[AprilSylph]

Do you think there would be a problem with making async helper functions for getting/setting from the Map and storage.session? i.e.

const handledRequests = new Map();
const getHandledRequest = async (requestId) =>
  handledRequests.get(requestId) ??
  chrome.storage.session.get(requestId).then(({ [requestId]: timeStamp }) => timeStamp);
const setHandledRequest = (requestId, timeStamp) => {
  handledRequests.set(requestId, timeStamp);
  chrome.storage.session.set({ [requestId]: timeStamp });
};

If there's no issues with reading from the Map inside an async function then this would reduce some repetition.

[marcustyphoon]

That looks correct/equivalent to me! And indeed, yeah, seems much cleaner.

[marcustyphoon]

Oh—I see what you might be saying. Like, replacing the handledRequests.has(requestId) condition with await getHandledRequest(requestId) would bring back the race condition, because the first invocation would perform the await on the storage get before doing the set, during which the second invocation could perform its storage get.

So yes those functions could be created and would save some lines; no one can't use them in every location and thus abstract out the storage entirely (again, assuming I'm understanding this issue correctly).

So this should be ok:

  const handledRequests = new Map();
+ const getHandledRequest = async (requestId) =>
+   handledRequests.get(requestId) ??
+   chrome.storage.session.get(requestId).then(({ [requestId]: timeStamp }) => timeStamp);
+ const setHandledRequest = (requestId, timeStamp) => {
+   handledRequests.set(requestId, timeStamp);
+   chrome.storage.session.set({ [requestId]: timeStamp });
+ };

  chrome.webRequest.onBeforeRequest.addListener(({ method, requestBody, requestId, timeStamp, url }) => {
    if (handledRequests.has(requestId)) {
      return;
    } else {
+     setHandledRequest(requestId, timeStamp);
    }
    // etc

  chrome.webRequest.onErrorOccurred.addListener(async ({ requestId }) => {
+   const timeStamp = await getHandledRequest(requestId);
    if (timeStamp) {
      // etc

Looking at it another way, the only reason this change actually resolves the race condition is that it does (only) that synchronous check in onBeforeRequest, which means this doesn't handle the hypothetical case where onBeforeRequest gets triggered twice but the background script is unloaded in between them. The branch without this PR does, or attempts to. (Fortunately they're separated by, per the timestamp values, one millisecond, so there's no way that's happening.)

[AprilSylph]

Wow, one millisecond... yeah, if I'd known that, I wouldn't have even suggested we try the async Map get.

Crazy that we have to do this and that the browser doesn't just... not fire two callbacks for the same event. 🫠