Re: [IndexedDB] setVersion blocked on uncollected garbage IDBDatabases

On Wed, Feb 9, 2011 at 11:20 AM, Jeremy Orlow <jorlow@chromium.org> wrote:

> On Wed, Feb 9, 2011 at 11:05 AM, Drew Wilson <atwilson@google.com> wrote:
>
>> This discussion reminds me of a similar issue with MessagePorts. The
>> original MessagePort spec exposed GC behavior through the use of onclose
>> events/closed attributes on MessagePorts. It turns out that on Chromium,
>> there are situations where it's very difficult for us to GC MessagePorts (a
>> port's reachability depends on the reachability of the entangled port on an
>> entirely separate process), and so we just don't.
>>
>> My concern is that there may be situations like this with IDB - if at some
>> point it's possible for events to be fired on an IDB instance (if we support
>> triggers), you'll have a situation where the reachability of an IDB instance
>> may depend on the reachability of that same DB in other processes. The net
>> effect is that on multi-process/multi-heap platforms, we may not be able to
>> GC databases, while on other platforms (which have a unified heap) we will
>> be able to GC them. This will end up being a source of cross-browser
>> incompatibility, because code will work just fine on platforms that are able
>> to deterministically GC databases, but then will break on other platforms
>> that cannot.
>>
>> (As an aside, Jeremy mentions that there may already be situations where
>> we cannot GC databases today - I don't know the spec well enough to comment,
>> though, so perhaps he can elaborate).
>>
>
> Yeah.  Talking to Drew made me realize that we (WebKit) already have a
> cycle so that we probably can't collect IDBDatabase objects with event
> listeners attached to it.  When there's a listener, we have to hold a
> reference to the JavaScript wrapper since it's what holds onto the
> JavaScript function we call.  But the wrapper holds a reference to our
> WebCore object.  We can break the cycle only when we know that we're not
> going to call any more events on it.  We know that when .close() is called.
>
> Working around this as is will be tricky, but isn't really a spec problem.
>  But it does mean that the developer will need to always call .close() or
> ask the user to close the tab in order to ever be able to run a setVersion
> transaction.  At least for the time being in any WebKit browser.
>

This mainly becomes an issue if some browsers are able to GC when others are
not - if that happens, then you end up with incompatible behavior because
setVersion() calls that work on one browser won't work on others. So if
setVersion() is going to expose GC behavior, then I think you have to codify
in the spec the Least Common Denominator GC behavior (for example,
spec-compliant implementations *must not* GC databases with event handlers
registered). This is a rabbit hole I don't think we want to go down.


>
>
>> In any case, I don't think that IDB should be the first place in the
>> entire web platform where we expose GC behavior to clients.
>>
>> -atw
>>
>> On Tue, Feb 8, 2011 at 4:43 PM, Jonas Sicking <jonas@sicking.cc> wrote:
>>
>>> On Tue, Feb 8, 2011 at 3:31 PM, Glenn Maynard <glenn@zewt.org> wrote:
>>> > On Tue, Feb 8, 2011 at 4:01 PM, Jeremy Orlow <jorlow@chromium.org>
>>> wrote:
>>> >>
>>> >> I talked it over with Darin (Fisher), and he largely agreed with you
>>> guys.
>>> >>  I'll file a bug saying that after unload, all IDBDatabases attached
>>> to that
>>> >> document should be closed.
>>> >
>>> > What happens if a database is open in a page in the back-forward cache?
>>> > That's incompatible with onunload having side-effects.
>>> >
>>> > I know the BF-cache is off-spec, but it's extremely useful and will
>>> > hopefully find its way into the standard some day, so it'd be nice to
>>> keep
>>> > it in mind.
>>> >
>>> > I suppose the browser would discard whole pages from the BF-cache on
>>> demand
>>> > if required by a setVersion call.
>>>
>>> That's exactly what we do in Firefox. Implementations have to be able
>>> to throw things out of the BF cache on command anyway (since you
>>> generally want to limit the number of pages living in BF cache, and so
>>> loading a new page often causes other pages to be thrown out), so it's
>>> just a matter of calling into the same code here.
>>>
>>> / Jonas
>>>
>>>
>>
>