[webkit-reviews] review denied: [Bug 203690] IndexedDB: perform IDBServer work only on background thread : [Attachment 384857] Patch

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 203690: IndexedDB: perform IDBServer work only on background thread
https://bugs.webkit.org/show_bug.cgi?id=203690

Attachment 384857: Patch

https://bugs.webkit.org/attachment.cgi?id=384857&action=review




--- Comment #9 from Brady Eidson <beidson at apple.com> ---
Comment on attachment 384857
  --> https://bugs.webkit.org/attachment.cgi?id=384857
Patch

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

Everywhere an ASSERT(isMainThread()) is removed because that work is now done
on the database thread, please change to ASSERT(isDatabaseThread()) (making
isDatabaseThread() if it doesn't exist)

In general I'm concerned about how this change overhauls threading assumptions
that had been built in to IDB since the rewrite.

I would suggest cutting as much out of this patch as possible to make it easier
to review (e.g. moving IDBServer from WebCore to WebKitLegacy is not critical)
and also see if we can find a way to do this in multiple steps.

We need to have a good strategy to make sure we don't introduce threading
mistakes like the one mentioned further down.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:634
>      for (auto& database : m_allUniqueIDBDatabases)
> -	   database.resume();
> +	   database.abortActiveTransactions();
>  }

This is one example of a threading error that was missed; It iterates over the
set m_allUniqueIDBDatabases, but that set is also mutated by UniqueIDBDatabases
on the database thread simultaneously. These happen with no locking.