firebase-js-sdk icon indicating copy to clipboard operation
firebase-js-sdk copied to clipboard

IndexedDB connections must be closed so bfcache works

Open gustavopch opened this issue 3 years ago • 7 comments
trafficstars

[REQUIRED] Describe your environment

  • Operating System version: macOS 12.3
  • Browser version: Chrome 100
  • Firebase SDK version: 9.6.7
  • Firebase Product: auth

[REQUIRED] Describe the problem

As described in https://web.dev/bfcache, the browsers' back/forward cache doesn't work when IndexedDB has an open connection. Currently, connections are opened here and then they're never closed:

https://github.com/firebase/firebase-js-sdk/blob/cdada6c68f9740d13dd6674bcb658e28e68253b6/packages/auth/src/platform_browser/persistence/indexed_db.ts#L90-L94

They should be closed (db.close()) immediately after used or when deleteApp() is called.

As a workaround, I monkey patched window.indexedDB.open to keep references to each connection so I can close them on pagehide so bfcache works:

const open = window.indexedDB.open.bind(window.indexedDB)
const dbs = []

window.indexedDB.open = (...args) => {
  const request = open(...args)

  request.addEventListener('success', event => {
    const db = event.target.result
    dbs.push(db)
  })

  return request
}

window.addEventListener(
  'pagehide',
  () => {
    for (const db of dbs) {
      db.close()
    }
  },
  { capture: true },
)

gustavopch avatar Apr 20 '22 02:04 gustavopch

In addition, when deleteApp() is called, IndexedDBLocalPersistence should stop polling IndexedDB.

Polling starts here: https://github.com/firebase/firebase-js-sdk/blob/cdada6c68f9740d13dd6674bcb658e28e68253b6/packages/auth/src/platform_browser/persistence/indexed_db.ts#L402-L409

But then it keeps happening forever.

gustavopch avatar Apr 21 '22 02:04 gustavopch

Note: May want to audit other non-Auth packages that use indexedDB to make sure they close the indexedDB connection in their delete method.

hsubox76 avatar Apr 25 '22 18:04 hsubox76

I'm experimenting here. Closing IndexedDB connections on pagehide event sometimes doesn't work. It seems the browser (Chrome v100 on macOS at least) doesn't give enough time before fully leaving the page and they aren't closed fast enough. So closing them on deleteApp won't be enough to make bfcache work. Instead, it would be better to close the connections immediately after used. Pseudo-code:

const runWithDatabase = async (fn) => {
  const db = await openDatabase()
  const result = await fn(db)
  closeDatabase(db)
  return result
}

gustavopch avatar May 02 '22 17:05 gustavopch

@gustavopch Is your experiment with a non-auth package, as suggested in https://github.com/firebase/firebase-js-sdk/issues/6167#issuecomment-1108908079?

yoyomyo avatar May 05 '22 18:05 yoyomyo

@yoyomyo I've tested Auth and Firestore. The IndexedDB issue explained in this issue only seems to happen with Auth.

There's also something in Firestore that breaks bfcache under certain circumstances, but I didn't experiment enough to bring the reproduction steps.

gustavopch avatar May 05 '22 18:05 gustavopch

Solving this issue may involve refactoring deleteApp() to be synchronous. If it can synchronously clean up all IndexedDB connections, web socket subscriptions and HTTP requests, having window.addEventListener('pagehide', () => deleteApp(getApp())) in user's code may be enough to make bfcache work.

gustavopch avatar Jun 20 '22 16:06 gustavopch

On Safari for iOS v15.5, bfcache will sort of work regardless of closing IndexedDB connections or running deleteApp(), but there's a big problem: if you call deleteApp() on pagehide, when you come back to that page and it's restored from bfcache, a newly initialized Firestore instance will just not work anymore (e.g. if you add a document listener, onNext callback won't be called, neither with data from cache nor from network). Probably due to an incomplete cleanup. Now, if I let the page be hidden by Safari without calling deleteApp() and then call it on pageshow right before reinitializing Firebase, then it works. This is a serious bug and I think it should elevate the priority of this issue.

gustavopch avatar Jun 20 '22 22:06 gustavopch