Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix multi-tab session lock on Firefox not being cleared #11800

Merged
merged 2 commits into from
Oct 26, 2023
Merged
Changes from all commits
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
41 changes: 25 additions & 16 deletions src/utils/SessionLock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,27 @@ export async function getSessionLock(onNewInstance: () => Promise<void>): Promis
}
}

// handler for pagehide and unload events, used later
function onPagehideEvent(): void {
// only remove the ping if we still think we're the owner. Otherwise we could be removing someone else's claim!
if (lockServicer !== null) {
prefixedLogger.debug("page hide: clearing our claim");
window.clearInterval(lockServicer);
Copy link
Member

@richvdh richvdh Oct 26, 2023

Choose a reason for hiding this comment

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

Looks like this is new.

On balance, adding it is probably a good idea.

Previously I left the lockServicer in place, which would mean if the page came back to life, we would reclaim the lock. But that is racy: if we clear our claim, but the lockServicer fires again before the tab actually dies, we'll end up with a stuck claim.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was my motivation - with the two handlers the risk to have a racy access might be higher. But I did not think about the page after being restored.

To close the bfache hole, couldn't we listen to pageshow with event.persisted==true and immediately re-start the lock servicer? But this might induce other problems, e.g.

  1. have one Element instance in bfcache
  2. open another instance in another tab (which works after the lock is expired)
  3. restore the first instance of the bfcache

Maybe listening for pageshow with event.persisted==true and somehow forcing the session to be re-initialized would be better (or just reloading the whole app?).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there are no ideal solutions here. To be honest, my understanding is that the presence of an unload handler prevents the app being eligible for the bfcache anyway, so it's probably fine in practice. Longer term, the "right" solution is to fix multi-tab loading so we don't need any of this locking business (element-hq/element-web#26231).

window.localStorage.removeItem(SESSION_LOCK_CONSTANTS.STORAGE_ITEM_PING);
window.localStorage.removeItem(SESSION_LOCK_CONSTANTS.STORAGE_ITEM_OWNER);
lockServicer = null;
}

// It's worth noting that, according to the spec, the page might come back to life again after a pagehide.
//
// In practice that's unlikely because Element is unlikely to qualify for the bfcache, but if it does,
// this is probably the best we can do: we certainly don't want to stop the user loading any new tabs because
// Element happens to be in a bfcache somewhere.
//
// So, we just hope that we aren't in the middle of any crypto operations, and rely on `onStorageEvent` kicking
// in soon enough after we resume to tell us if another tab woke up while we were asleep.
}

async function releaseLock(): Promise<void> {
// tell the app to shut down
await onNewInstance();
Expand Down Expand Up @@ -239,23 +260,11 @@ export async function getSessionLock(onNewInstance: () => Promise<void>): Promis
window.addEventListener("storage", onStorageEvent);

// also add a listener to clear our claims when our tab closes or navigates away
window.addEventListener("pagehide", (event) => {
// only remove the ping if we still think we're the owner. Otherwise we could be removing someone else's claim!
if (lockServicer !== null) {
prefixedLogger.debug("page hide: clearing our claim");
window.localStorage.removeItem(SESSION_LOCK_CONSTANTS.STORAGE_ITEM_PING);
window.localStorage.removeItem(SESSION_LOCK_CONSTANTS.STORAGE_ITEM_OWNER);
}
window.addEventListener("pagehide", onPagehideEvent);

// It's worth noting that, according to the spec, the page might come back to life again after a pagehide.
//
// In practice that's unlikely because Element is unlikely to qualify for the bfcache, but if it does,
// this is probably the best we can do: we certainly don't want to stop the user loading any new tabs because
// Element happens to be in a bfcache somewhere.
//
// So, we just hope that we aren't in the middle of any crypto operations, and rely on `onStorageEvent` kicking
// in soon enough after we resume to tell us if another tab woke up while we were asleep.
});
// The pagehide event is called unreliably on Firefox, so additionally add an unload handler.
// https://bugzilla.mozilla.org/show_bug.cgi?id=1854492
window.addEventListener("unload", onPagehideEvent);

return true;
}