Bug 111459 – IndexedDB: Use WeakPtr for Factory-to-BackingStore reference

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

[Joshua Bell]

IndexedDB: Use WeakPtr for Factory-to-BackingStore reference

[Joshua Bell]

Created attachment 191519 [details]
Patch

[Joshua Bell]

This is an exploratory change. We should probably not use it as-is.

...

As currently implemented, the factory maintains a map of origin->backing store (raw ptr) so that each database opened within the same origin uses the same backing store. To maintain this map, the backing store holds a RefPtr to the factory and when it is destructed it calls into the factory to remove the map entry.

This patch changes the map's pointer type to a WeakRef, so that backing store no longer needs an explicit reference to the factory.

The impetus is https://bugs.webkit.org/show_bug.cgi?id=110820 - for in-memory backing stores, we want the IDBFactoryBackendImpl lifetime to dictate the IDBBackingStore lifetime - so, the factory should maintain a RefPtr to the backing store. If the backing store had a RefPtr to the factory that would introduce a reference cycle.

...

As written the HashMap<String, WeakPtr<>> entries will never be cleared out. That will cost some memory for each origin encountered, which is not ideal. We may need to introduce a WeakPtrHashMap<> that cleans up HashMap entries when the ref goes away.

...

Thoughts?

[Adam Barth]

Comment on attachment 191519 [details]
Patch

Sounds like a reasonable idea.  We're trying to be careful about where we use WeakPtr so that we gain experience with it slowly, but this sounds like a good use case.  (I haven't reviewed the details of this patch.)

[Joshua Bell]

Created attachment 191803 [details]
Patch

[Joshua Bell]

Rebased on other IDBBackingStore changes.

This prevents the HashMap<K,WeakPtr<M>> from growing unbounded by introducing an explicit "cleanWeakMap" call at the point of insertion that scrubs the map of nulled WeakPtrs. Not an ideal approach, but plausible.

[Adam Barth]

Comment on attachment 191803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191803&action=review

We could also let you register a callback with WeakPtr to be notified when it gets cleared, but we probably don't need to tech to that level yet.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:45
> +template<typename K, typename M>
> +static void cleanWeakMap(HashMap<K, WeakPtr<M> >& map)

This seems like a pretty general-purpose function for this file.  I wonder if we should move it to WTF somewhere...  Maybe this pattern is obscure enough that it's fine to keep it here.

[Joshua Bell]

Created attachment 192086 [details]
Patch

[Joshua Bell]

alecflett@ - can you look this over before landing since the merge with your backing store changes/tests involved deleting some assertions? I may have missed the intent of something.

[Joshua Bell]

(In reply to comment #6)
> This seems like a pretty general-purpose function for this file.  I wonder if we should move it to WTF somewhere...  Maybe this pattern is obscure enough that it's fine to keep it here.

Yeah, I coded it in a generic fashion anticipating re-use. It could be re-used in the following other places in IDB to the need for custom "lifetime awareness" tracking:

IDBTransaction - replace register/unregisterOpenCursor/HashSet<IDBCursor*>
IDBTransactionBackendImpl - same thing, but the back end objects
IDBFactoryBackendImpl - currently manages lifetime of IDBDatabaseBackendImpls but could be made weak

Given that it bridges HashSet and WeakPtr I didn't spot a good place in WTF to wedge it, but will keep pondering.

[Joshua Bell]

(In reply to comment #9)
> It could be re-used

"it" = WeakPtr + HashMap

> ...to the need for...

...to replace...

[WebKit Review Bot]

Comment on attachment 192086 [details]
Patch

Clearing flags on attachment: 192086

Committed r145166: <http://trac.webkit.org/changeset/145166>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 111819

[Joshua Bell]

Created attachment 192239 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 192239 [details]
Patch for landing

Rejecting attachment 192239 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-01', 'validate-changelog', '--non-interactive', 192239, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

/mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17109112

[Joshua Bell]

Created attachment 192241 [details]
Patch for landing

[Joshua Bell]

(In reply to comment #15)
> /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Apparently my "update changelogs with newly added files" script has some glitches. :P

[WebKit Review Bot]

Comment on attachment 192241 [details]
Patch for landing

Clearing flags on attachment: 192241

Committed r145238: <http://trac.webkit.org/changeset/145238>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.