| Summary: | [IndexedDB] IndexedDB's threading assertion should respect Web thread | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | alecflett, beidson, dbates, ews-watchlist, jsbell, sihui_liu, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Yusuke Suzuki
2019-11-19 03:07:00 PST
Created attachment 383854 [details]
Patch
Created attachment 383855 [details]
Patch
Comment on attachment 383855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383855&action=review This patch looks good. > Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41 > + ASSERT(canAccessThreadLocalDataForThread(m_originThread.get())); Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument. > Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:54 > + ASSERT(canAccessThreadLocalDataForThread(m_originThread.get())); This is ok as-is. The .get() is unnecessary because Ref<> overloads operator T&(). Comment on attachment 383855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383855&action=review Thanks! >> Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41 >> + ASSERT(canAccessThreadLocalDataForThread(m_originThread.get())); > > Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument. I think this ForThread is a bit important since this function is returning true when the argument’s thread data can be accessed by the current thread. Without “ForThread”, the function name does not tell wether it is (1) checking access right from the current thread to the argument thread or (2) checking the access right from the argument thread to the current thread. >> Source/WebCore/Modules/indexeddb/client/TransactionOperation.h:54 >> + ASSERT(canAccessThreadLocalDataForThread(m_originThread.get())); > > This is ok as-is. The .get() is unnecessary because Ref<> overloads operator T&(). Thanks, will fix. Comment on attachment 383855 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383855&action=review >>> Source/WebCore/Modules/indexeddb/IDBActiveDOMObject.h:41 >>> + ASSERT(canAccessThreadLocalDataForThread(m_originThread.get())); >> >> Naming is ok-as is. The slightly less verbose name would be "canAccessThreadLocalData". That's no need for "ForThread" as that can be implied by the passed argument. > > I think this ForThread is a bit important since this function is returning true when the argument’s thread data can be accessed by the current thread. Without “ForThread”, the function name does not tell wether it is (1) checking access right from the current thread to the argument thread or (2) checking the access right from the argument thread to the current thread. Talked with Daniel, we should rename it to `canCurrentThreadAccessThreadLocalData` Committed r252642: <https://trac.webkit.org/changeset/252642> Committed r252651: <https://trac.webkit.org/changeset/252651> |