Closed
Bug 1366647
Opened 4 years ago
Closed 4 years ago
Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in dom/indexedDB and dom/filehandle
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bevis, Assigned: bevis)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
16.27 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
According to the Thread API change in quantum-dom project [1], " Thread-safety assertions: Code that used PR_GetCurrentThread for thread safety assertions will be converted to use NS_ASSERT_OWNINGTHREAD " GetCurrentPhysicalThread and GetCurrentVirtualThread could be used if PR_Thread* comparison is not replaceable with NS_ASSERT_OWNINGTHREAD. This bug is filed to address these change in both dom/indexedDB and dom/filehandle which is heavily used by dom/indexedDB shall be refactored at once. [1] https://groups.google.com/forum/#!topic/mozilla.dev.platform/L8PI6E-l2vM
Assignee | ||
Comment 1•4 years ago
|
||
I'd like to simplify mozilla::dom::ThreadObject with NS_DECL_OWNINGTHREAD/NS_ASSERT_OWNINGTHREAD because the call site of AssertIsOnOwningThread() in the ctor of these ThreadObjects could just be replaced by the call site of these ctors with {CallerObject}::AssertIsOnOwningThread().
Attachment #8869902 -
Flags: review?(jvarga)
Assignee | ||
Comment 2•4 years ago
|
||
Replace the data member of mOwningThread in IDB classes with NS_DECL_OWNINGTHREAD. Note: 1. NS_DECL_OWNINGTHREAD is added implicitly in NS_INLINE_DECL_REFCOUNTING, NS_DECL_CYCLE_COLLECTING_ISUPPORTS so we don't do redundant declaration in some class definition. 2. ConnectionPool::DatabaseInfo::mDEBUGConnectionThread has different lifecycle to its owning class and is not the main thread, so GetCurrentPhysicalThread() is used for the implementation of ConnectionPool::DatabaseInfo::AssertIsOnConnectionThread().
Attachment #8869904 -
Flags: review?(jvarga)
Assignee | ||
Updated•4 years ago
|
Summary: Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD → Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in IndexedDB
Assignee | ||
Updated•4 years ago
|
Summary: Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in IndexedDB → Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in dom/indexedDB and dom/filehandle
Assignee | ||
Comment 3•4 years ago
|
||
Fixed incorrect assertion.
Attachment #8869902 -
Attachment is obsolete: true
Attachment #8869902 -
Flags: review?(jvarga)
Attachment #8869940 -
Flags: review?(jvarga)
Assignee | ||
Comment 4•4 years ago
|
||
treeherder result looks fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2734cc0bfc182f2a46ef036134ad8611c8231720
Comment 5•4 years ago
|
||
Ok, will look at this right after bug 1360185, finishing a review of the last patch there.
Comment 6•4 years ago
|
||
Hm, I think we need to redo some stuff in dom/filehandle, I will send a part 0 patch today.
Comment 7•4 years ago
|
||
I don't like that all the objects would now call PR_GetCurrentThread() to initialize debug only owning thread member variable. So, I thought that this patch would work, but I gave up since virtual AssertIsOnOwningThread() needs to be called in object ctor/dtor directly or indirectly. Sometimes it compiles fine but I get runtime crashes due to pure virtual calls ... Another option would be to extend nsAutoOwningThread to support passing owning thread, instead of calling PR_GetCurrentThread() Bevis, do you think this is doable and you are willing to try that ?
Comment 8•4 years ago
|
||
Bevis, let me try something else in the meantime.
Comment 9•4 years ago
|
||
Ok, I'm finishing a patch that removes the filehandle object abstraction by integrating stuff into dom/indexedDB There's no evidence that FileHandle API would be used in other storage APIs. The patch is just for the child side, but that should be enough for this bug.
Assignee | ||
Comment 10•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #7) Sorry for the late response. > > I don't like that all the objects would now call PR_GetCurrentThread() to > initialize debug only owning thread member variable. I didn't see the point why we should not call PR_GetCurrentThread() every time to initialize the debug-only member variable if they are *debug-only*. Is there any concern about that? > Another option would be to extend nsAutoOwningThread to support passing > owning thread, instead of calling PR_GetCurrentThread() > Bevis, do you think this is doable and you are willing to try that ? Unless there is an scenario that the passed PRThread* to nsAutoOwningThread would be different from PR_GetCurrentThread(), otherwise there is no good reason to me to have PRThreadThread* as a parameter to construct nsAutoOwningThread... :( I'd prefer to make the debugging code compact and easy to be virtualized by Nathan/Bill in the future if PR_GetCurrentThread() call internally in each initialization of nsAutoOwningThread is not a problem.
Comment 11•4 years ago
|
||
(In reply to Bevis Tseng [:bevis][:btseng] from comment #10) > (In reply to Jan Varga [:janv] from comment #7) > Sorry for the late response. > > > > I don't like that all the objects would now call PR_GetCurrentThread() to > > initialize debug only owning thread member variable. > I didn't see the point why we should not call PR_GetCurrentThread() every > time to initialize the debug-only member variable if they are *debug-only*. > Is there any concern about that? The whole chain of objects should live on the same thread, so we only call PR_GetCurrentThread() for IDBFactory. dom/filehandle worked around this by using OwningThread() > > Another option would be to extend nsAutoOwningThread to support passing > > owning thread, instead of calling PR_GetCurrentThread() > > Bevis, do you think this is doable and you are willing to try that ? > Unless there is an scenario that the passed PRThread* to nsAutoOwningThread > would be different from PR_GetCurrentThread(), otherwise there is no good > reason to me to have PRThreadThread* as a parameter to construct > nsAutoOwningThread... :( > > I'd prefer to make the debugging code compact and easy to be virtualized by > Nathan/Bill in the future if PR_GetCurrentThread() call internally in each > initialization of nsAutoOwningThread is not a problem. Ok, I agree, that it should be compact. That's why I changed my mind and I just decided to merge dom/filehandle with dom/indexedDB I have it almost done.
Comment 12•4 years ago
|
||
Ok, (In reply to Jan Varga [:janv] from comment #11) > Ok, I agree, that it should be compact. That's why I changed my mind and I > just > decided to merge dom/filehandle with dom/indexedDB > I have it almost done. It's on try now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f88f93ff922df09be129e35e1217a669646d6cef
Comment 13•4 years ago
|
||
Comment on attachment 8869904 [details] [diff] [review] (v1) Part 2: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in dom/indexedDB. Review of attachment 8869904 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.h @@ -95,5 @@ > IDBFactory* aFactory, > indexedDB::BackgroundDatabaseChild* aActor, > DatabaseSpec* aSpec); > > -#ifdef DEBUG So, if we remove this DEBUG only "optimization", AssertIsOnOwningThread() will call mFactory->AssertIsOnOwningThread() which is a no-op in debug builds, but are we sure that compilers will do the right thing in optimized builds and eliminate the whole call chain for us ?
Assignee | ||
Comment 14•4 years ago
|
||
Comment on attachment 8869904 [details] [diff] [review] (v1) Part 2: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in dom/indexedDB. Review of attachment 8869904 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.h @@ -95,5 @@ > IDBFactory* aFactory, > indexedDB::BackgroundDatabaseChild* aActor, > DatabaseSpec* aSpec); > > -#ifdef DEBUG Now, I see the point of having an empty function inline in class to ensure that it will be removed in the optimized build. :\ I'll keep these #ifdef unchanged in next update.
Comment 15•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #13) > Comment on attachment 8869904 [details] [diff] [review] > (v1) Part 2: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in > dom/indexedDB. > > Review of attachment 8869904 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/IDBDatabase.h > @@ -95,5 @@ > > IDBFactory* aFactory, > > indexedDB::BackgroundDatabaseChild* aActor, > > DatabaseSpec* aSpec); > > > > -#ifdef DEBUG > > So, if we remove this DEBUG only "optimization", AssertIsOnOwningThread() > will call mFactory->AssertIsOnOwningThread() which is a no-op in debug It's a no-op in optimized builds, sorry, but I guess you get my point now.
Comment 16•4 years ago
|
||
(In reply to Bevis Tseng [:bevis][:btseng] from comment #14) > Comment on attachment 8869904 [details] [diff] [review] > (v1) Part 2: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in > dom/indexedDB. > > Review of attachment 8869904 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/IDBDatabase.h > @@ -95,5 @@ > > IDBFactory* aFactory, > > indexedDB::BackgroundDatabaseChild* aActor, > > DatabaseSpec* aSpec); > > > > -#ifdef DEBUG > > Now, I see the point of having an empty function inline in class to ensure > that it will be removed in the optimized build. :\ > I'll keep these #ifdef unchanged in next update. The next update could be based on bug 1370519.
Assignee | ||
Comment 17•4 years ago
|
||
(In reply to Jan Varga [:janv] from comment #16) > The next update could be based on bug 1370519. Will do! :)
Assignee | ||
Comment 18•4 years ago
|
||
Revise the patches based on the fix in bug 1370519.
Attachment #8869904 -
Attachment is obsolete: true
Attachment #8869940 -
Attachment is obsolete: true
Attachment #8871228 -
Attachment is obsolete: true
Attachment #8869904 -
Flags: review?(jvarga)
Attachment #8869940 -
Flags: review?(jvarga)
Attachment #8875236 -
Flags: review?(jvarga)
Comment 19•4 years ago
|
||
Comment on attachment 8875236 [details] [diff] [review] (v2) Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality. Review of attachment 8875236 [details] [diff] [review]: ----------------------------------------------------------------- Reviewing this now...
Comment 20•4 years ago
|
||
Comment on attachment 8875236 [details] [diff] [review] (v2) Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality. Review of attachment 8875236 [details] [diff] [review]: ----------------------------------------------------------------- Ok, looks good.
Attachment #8875236 -
Flags: review?(jvarga) → review+
Comment 21•4 years ago
|
||
Pushed by btseng@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bad37b9f58eb Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality. r=janv
Assignee | ||
Comment 22•4 years ago
|
||
treeherder result for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=33c12a835263c1feb419d1554700ac94135cd970
Comment 23•4 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bad37b9f58eb
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•