861308 - IndexedDB keeps nsGlobalWindow alive until a transaction is complete

Status: RESOLVED WORKSFORME

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

Description

[Marco Bonardo [:mak]]

Attachment: test patch

...or at least this is what it looks like.

I made this reduced mochitest-browser test case. What it does is opening a tab to a page, the page opens an indexedDB connection, inits 3 operations and notifies back the test, that closes the tab.

The cc-analyzer in the harness run GC+CC, but it still finds the nsGlobalWindows alive and notifies a nsGlobalWindow leak.

Though, just waiting some ms, until the transactions are properly aborted, fixes the "leak". So basically this is not a real leak, could even be wanted per idb design, but we need a strategy to cope with it in mochitests using the cc-analyzer if so.

Comment 1

[Marco Bonardo [:mak]]

We could even try to change the mochitest-browser detection, if the current system finds some nsGlobalWindow leak it could sleep 1 second and re-run the check again.  The delay would only happen in case of suspected leaks, and may improve some cases where we already have random oranges (iirc) due to globalwindow leaks that appear only sometimes. Real leaks should still be properly detected.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [:mak] from comment #1)
> We could even try to change the mochitest-browser detection, if the current
> system finds some nsGlobalWindow leak it could sleep 1 second and re-run the
> check again.  The delay would only happen in case of suspected leaks, and
> may improve some cases where we already have random oranges (iirc) due to
> globalwindow leaks that appear only sometimes. Real leaks should still be
> properly detected.

Hmm, this seems reasonable.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I tend to think that the test should wait for outstanding transactions to complete before moving on.

Comment 4

[Marco Bonardo [:mak]]

Unless there's a way to be notified when all outstanding operations are complete, that would mean to track each operation to be able to notify completion of all of them. It's a lot of code complication, that is unneeded for the page functionality, only needed by the test.
And this is just the first browser mochitest using indexedDB, there could be others in future, requesting all of them to track every single write sounds like a nightmare.

The perfect fix would clearly change idb to drop window references when the window is going away, but I figure there are reasons if it keeps all references around.
Thus I'd probably first go for a solution similar to comment 1, even if it's a not perfect solution.

Though, if there's something in the idb spec that allows me to be notified when all requests have been handled or the connection is being closed, that may work.  I couldn't find it.  I may try with the onabort handler on the connection, but it still looks like a flaky solution (what if sometimes they are completed and sometimes aborted?).

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, the easiest thing to do would be to add onabort/oncomplete handlers to your transactions (it could even be the same function). All you'd have to do is increment a counter every time you make a new one and then decrement whenever one finished. Then your cleanup promise would be resolved when the counter hits 0.

We could probably change the IDB code to drop the window right away but it's not risk-free. The current code is all built with the assumption that the main thread objects are still around while DB operations are in progress, so changing that assumption might break things in subtle ways that will be hard to test (since it's a race we're talking about here).

Comment 6

[Marco Bonardo [:mak]]

fwiw, I went the harness solution in bug 863447, because I think having the requirement for every new test to handle this (or issues similar to this) apart would not be really nice.
Unless we find a common way to track all the pending work I don't think it's wise to request each test to handle it, moreover this issue shown the current harness detection was just too strict.

So, if you think the de-facto situation where off main-thread work keeps alive main-thread references is acceptable, or maybe even wanted, and there's no low hanging fruit to have a global "I'm done" notification, I think you may even decide to wontfix this bug.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We could also add some kind of DOMWindowUtils method to wait for all DB activity to finish, and then the harness could automatically call that?

Comment 8

[Marco Bonardo [:mak]]

(In reply to ben turner [:bent] from comment #7)
> We could also add some kind of DOMWindowUtils method to wait for all DB
> activity to finish, and then the harness could automatically call that?

if it's accessible from js, the harness can use it, sure.

Comment 9

[Jens Stutte [:jstutte]]

I think we can close this, assuming we found other workarounds in the meantime?

Comment 10

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

This was definitely mooted by bug 1539407 moving IDBDatabase to be a DOMEventTargetHelper if it wasn't already addressed by the introduction of the IDBWrapperCache in bug 994190. As a DETH IDBDatabase never has an owning reference to the global.