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

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 1: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in dom/filehandle.

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().

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 2: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in dom/indexedDB.

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().

Comment 3

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v1) Part 1: Use NS_ASSERT_OWNINGTHREAD for PRThread equality in dom/filehandle.

Fixed incorrect assertion.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

treeherder result looks fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2734cc0bfc182f2a46ef036134ad8611c8231720

Comment 5

[Jan Varga [:janv]]

Ok, will look at this right after bug 1360185, finishing a review of the last patch there.

Comment 6

[Jan Varga [:janv]]

Hm, I think we need to redo some stuff in dom/filehandle, I will send a part 0 patch today.

Comment 7

[Jan Varga [:janv]]

Attachment: alternative patch

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

[Jan Varga [:janv]]

Bevis, let me try something else in the meantime.

Comment 9

[Jan Varga [:janv]]

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.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(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

[Jan Varga [:janv]]

(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

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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 ?

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

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

[Jan Varga [:janv]]

(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

[Jan Varga [:janv]]

(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.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #16)
> The next update could be based on bug 1370519.
Will do! :)

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality.

Revise the patches based on the fix in bug 1370519.

Comment 19

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bad37b9f58eb
Adopt NS_ASSERT_OWNINGTHREAD for PRThread equality. r=janv

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33c12a835263c1feb419d1554700ac94135cd970

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bad37b9f58eb