Bug 49313 – IndexedDB: signal IDBFactoryBackendInterface destruction to embedder

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: signal IDBFactoryBackendInterface destruction to embedder

[Hans Wennborg]

Created attachment 73480 [details]
Patch

[Andrei Popescu]

LGTM

[Jeremy Orlow]

Why is this signal needed?  The factory only goes away when webkit is shutting down.  I'm surprised this is useful.

[Hans Wennborg]

(In reply to comment #3)
> Why is this signal needed?  The factory only goes away when webkit is shutting down.

That's what we want to detect.

This seemed like a relatively clean way of piping it through to the BrowserWebKitClientImpl on the Chromium side, which is what we're trying to reach. Do you see a better way?

[Jeremy Orlow]

Why is chromiums factories lifetime related to that of a factory in the renderer?  I mean the browsers factory likely outlives them all.  Note also that we need to make sure that we handle lifetimes correctly when the renderer crashes (which we do not at the moment, iirc)--which I feel would make this unnecessary as well.  Can you please explain the bigger picture here?

[Andrei Popescu]

(In reply to comment #5)
> Why is chromiums factories lifetime related to that of a factory in the renderer?  I mean the browsers factory likely outlives them all.  Note also that we need to make sure that we handle lifetimes correctly when the renderer crashes (which we do not at the moment, iirc)--which I feel would make this unnecessary as well.  Can you please explain the bigger picture here?


Umm, this is backend code so it runs in the browser not the renderer process. Renderer crashes don't really affect matter here, right? We just want to stop the utility process cleanly when the indexed database shuts down.

Andrei

[Jeremy Orlow]

Oops, you're right...was mixed up.  The object store objects don't hold a ref to the factory though, which means that it could go away before were done with the utility process, right?

[Hans Wennborg]

(In reply to comment #7)
> Oops, you're right...was mixed up.
Adding a comment trying to make it less confusing.

> The object store objects don't hold a ref to the factory though, which means that it could go away before were done with the utility process, right?

The destruction of the factory happens after the IndexedDBDispatcherHost (in Chromium) is destructed, so I don't think any more IndexedDB activity could continue after that point - it should be fine to tell the utility process to stop at this point.

[Hans Wennborg]

Created attachment 73604 [details]
Patch

[Jeremy Orlow]

(In reply to comment #8)
> (In reply to comment #7)
> > Oops, you're right...was mixed up.
> Adding a comment trying to make it less confusing.
> 
> > The object store objects don't hold a ref to the factory though, which means that it could go away before were done with the utility process, right?
> 
> The destruction of the factory happens after the IndexedDBDispatcherHost (in Chromium) is destructed, so I don't think any more IndexedDB activity could continue after that point - it should be fine to tell the utility process to stop at this point.

Then why does Chromium require this signal?  Can't we just shut it down when the dispatcher host shuts down?

Also, this is true today, but in the future when we have background threads and such this assumption probably won't hold unless the dispatcher doesn't go away until all running transactions and such have ended.

[Hans Wennborg]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Oops, you're right...was mixed up.
> > Adding a comment trying to make it less confusing.
> > 
> > > The object store objects don't hold a ref to the factory though, which means that it could go away before were done with the utility process, right?
> > 
> > The destruction of the factory happens after the IndexedDBDispatcherHost (in Chromium) is destructed, so I don't think any more IndexedDB activity could continue after that point - it should be fine to tell the utility process to stop at this point.
> 
> Then why does Chromium require this signal?

It's the cleanest solution to what we're trying to do in http://codereview.chromium.org/4678002/

> Can't we just shut it down when the dispatcher host shuts down?

We tried that at first, but didn't see a good way of getting from the dispatcher host to the WebKitClient.

This solution feels natural in the way that the WebKitClient (in this case BrowserWebKitClient impl on the Chromium side) gets a call from WebKit saying all the IDB stuff is done and shutting down so it can stop the utility process. Receiving calls from WebKit is what the WebKitClient does; trying to call it from somewhere else seems less clean.

> Also, this is true today, but in the future when we have background threads and such this assumption probably won't hold unless the dispatcher doesn't go away until all running transactions and such have ended.

The IDBTransactionCoordinator is destructed before the factory, so at this point I suppose all transactions should have finished.

[Jeremy Orlow]

Comment on attachment 73604 [details]
Patch

r=me

Still not convinced this is the best way, but I can't think of a better one.

[Hans Wennborg]

Comment on attachment 73604 [details]
Patch

Clearing flags on attachment: 73604

Committed r71829: <http://trac.webkit.org/changeset/71829>

[Hans Wennborg]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/71829 might have broken Leopard Intel Release (Tests)
The following tests are not passing:
media/video-played-collapse.html
media/video-played-ranges-1.html
media/video-played-reset.html