1879259 - IDBFactory does not notice worker global shutdown which can result in empty IDB databases created without letting the upgradeneeded event run

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P2)

Description

[Andrew Sutherland [:asuth] (he/him)]

As observed in https://bugzilla.mozilla.org/show_bug.cgi?id=1877619#c2, correct handling of situations where the global is shutting down while an openDatabase call is in flight require us to notice when the global is being torn down. Unfortunately, we only have logic in place for this on windows, not workers. This can result in IDB in the parent process thinking the database has been successfully populated despite nothing happening in the version change transaction because the upgradeneeded event never actually was triggered in content code. Excerpting the details about the specific problem from that comment:

That we try and move forward with the version change transaction on workers is somewhat surprising because we intend to handle this through the call to !EnsureDOMObject(). In particular, we would hope the !factory.GetParentObject() check would detect this. But IDBFactory is not a DOMEventTargetHelper subclass; it only clears mGlobal in IDBFactory::DisconnectFromGlobal but this is only called from nsGlobalWindowInner and never on workers. The good news is that :saschanaz introduced GlobalTeardownObserver for cases like this, and we can use that to replace the window-specific logic IDBFactory::DisconnectFromGlobal.

Comment 1

[Jan Varga [:janv]]

Maybe it would require less refactoring if you added:

  if (mIndexedDB) {
    mIndexedDB->DisconnectFromGlobal(this);
    mIndexedDB = nullptr;
  }

to WorkerGlobalScope::NoteShuttingDown

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

(Bug 1653276 fixed this for windows, but not workers.)

(In reply to Jan Varga [:janv] from comment #1)

Maybe it would require less refactoring if you added:

I already have a working fix using GlobalTeardownObserver which isn't too large and does simplify some things. Also it's arguably nicer/cleaner! Looking at how easy it is to add a test. (test_abort_on_reload.html is what bug 1653276 added.)

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1879259 - Use GlobalTeardownObserver for consistency on workers. r=#dom-storage!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/def4a6f94713
Use GlobalTeardownObserver for consistency on workers. r=dom-storage-reviewers,janv

Comment 5

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/44629 for changes under testing/web-platform/tests

Comment 6

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/def4a6f94713

Comment 7

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot