Closed Bug 1366647 Opened 2 years ago Closed 2 years ago

Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in dom/indexedDB and dom/filehandle

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

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)

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
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)
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)
Summary: Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD → Replace the implementation of AssertIsOnOwningThread() with NS_ASSERT_OWNINGTHREAD in IndexedDB
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
Fixed incorrect assertion.
Attachment #8869902 - Attachment is obsolete: true
Attachment #8869902 - Flags: review?(jvarga)
Attachment #8869940 - Flags: review?(jvarga)
Ok, will look at this right after bug 1360185, finishing a review of the last patch there.
Hm, I think we need to redo some stuff in dom/filehandle, I will send a part 0 patch today.
Attached patch alternative patch (obsolete) — Splinter Review
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 ?
Bevis, let me try something else in the meantime.
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.
(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.
(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.
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 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 ?
Depends on: 1370519
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.
(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.
(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.
(In reply to Jan Varga [:janv] from comment #16)
> The next update could be based on bug 1370519.
Will do! :)
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 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 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+
Pushed by btseng@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad37b9f58eb
Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality. r=janv
https://hg.mozilla.org/mozilla-central/rev/bad37b9f58eb
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.