remove the ownership of WeakPtr in SupportsWeakPtr

ASSIGNED
Assigned to

Status

()

defect
ASSIGNED
4 years ago
4 years ago

People

(Reporter: jerry, Assigned: jerry)

Tracking

(Blocks 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: btpp-followup-2016-03-08)

Attachments

(1 attachment, 4 obsolete attachments)

I add some assertion for the thread of object creating and release in mfbt RefCounted class.
And we might have problem in some indexDB's op WeakReference ref-counting.

The log shows that the op is released at the thread which is different from its creating thread.
https://pastebin.mozilla.org/8861274

We create the OpenDatabaseOp at background thread(please check [1] and [2]), but post to IO thread at [3].

The OpenDatabaseOp inherit from mozilla::ipc::MessageListenerMessageListener which has SupportsWeakPtr<mozilla::ipc::MessageListener>.

The SupportsWeakPtr has a member WeakReference which is not a thread-safe counting object [4].

My problem is that could we just do the OpenDatabaseOp at background thread?
Why there are some operations run at "IO" thread?

[1]
https://hg.mozilla.org/mozilla-central/annotate/918df3a0bc1c4d07299e4f66274a7da923534577/dom/indexedDB/ActorsParent.cpp#l12824
[2]
https://hg.mozilla.org/mozilla-central/annotate/918df3a0bc1c4d07299e4f66274a7da923534577/dom/indexedDB/ActorsParent.cpp#l12879
[3]
https://hg.mozilla.org/mozilla-central/annotate/918df3a0bc1c4d07299e4f66274a7da923534577/dom/indexedDB/ActorsParent.cpp#l19361
[4]
https://hg.mozilla.org/mozilla-central/annotate/918df3a0bc1c4d07299e4f66274a7da923534577/mfbt/WeakPtr.h#l96
Flags: needinfo?(khuey)
Flags: needinfo?(bent.mozilla)
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #0)
> My problem is that could we just do the OpenDatabaseOp at background thread?

No, we do input/output file operations during that state and the PBackground thread is not intended for I/O.

I guess the problem you described is not specific to dom/indexedDB. There's for example ParentRunnable in AsmJSCache.cpp which also inherits from IProtocol -> MessageListener -> ...
The obvious question is who still has a weak reference to the protocol at this point, and why?
Flags: needinfo?(khuey)
Flags: needinfo?(hshih)
Flags: needinfo?(bent.mozilla)
Whiteboard: btpp-followup-2016-03-04
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #2)
> The obvious question is who still has a weak reference to the protocol at
> this point, and why?

OpenDatabaseOp->IProtocol->MessageListener->SupportsWeakPtr<MessageListener>
Then SupportsWeakPtr<> has a WeakReference which is inherit from RefCounted.

If we post the OpenDatabaseOp task from background thread to io thread, we will release the |RefPtr<WeakReference>| at background thread(it's different from its creating thread - background thread) after the task done and then have the counting problem of |RefPtr<WeakReference>|. There is no leak for the object which the WeakReference points to. It's the ref-counting problem for the WeakReference object itself.
Flags: needinfo?(hshih) → needinfo?(khuey)
(In reply to Jan Varga [:janv] from comment #1)
> (In reply to Jerry Shih[:jerry] (UTC+8) from comment #0)
> > My problem is that could we just do the OpenDatabaseOp at background thread?
> 

> No, we do input/output file operations during that state and the PBackground
> thread is not intended for I/O.
I'm not sure I got your idea. Does it mean the PBackground thread is not ready when we try to run OpenOP?
There are AssertIsOnOwningThread() calls, so I think PBackground thread is ready. Could you please tell more details for the reason?

It seems that we only do init, open and close op at io thread.
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#19955
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#21304
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp#22385
Flags: needinfo?(jvarga)
Input/Output file operations usually need to access a disk drive. Such operations are much slower than memory operations. Sometimes it can take several seconds to get a response from the disk, especially on cheaper mobile devices, etc. The lag leads to user interface unresponsiveness (if it's done on the main thread) which is really bad for the user. However the PBackground thread is used as a "main thread" or if you want "coordination thread" for all storage based APIs, so we don't want to block it by I/O.

Trying to avoid the dispatch to the IO thread is just not the way to go in this case.
Flags: needinfo?(jvarga)
Whiteboard: btpp-followup-2016-03-04 → btpp-followup-2016-03-08
IMO, this is a bug in mozilla::SupportsWeakPtr.  There's no reason the referent itself needs to hold the weak reference alive.  And the XPCOM version of it does not suffer from this flaw.  Because the weakref is deep within IPDL code, I'm confident that, other than the reference carried by SupportsWeakPtr, it is only used on the PBackground thread.  If SupportsWeakPtr didn't hold alive the WeakReference this would just work.
Component: DOM: IndexedDB → MFBT
Flags: needinfo?(khuey)
Yes, the XPCOM version of referent doesn't hold the weakref. I will try to update the mozilla::SupportsWeakPtr later.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Blocks: 1251593
Attachment #8728204 - Flags: review?(nfroyd)
Attachment #8728204 - Flags: review?(jwalden+bmo)
Attachment #8728204 - Flags: review?(ehsan)
Attachment #8727766 - Attachment is obsolete: true
Comment on attachment 8728204 [details] [diff] [review]
remove the ownership of WeakPtr in SupportsWeakPtr. v2

Review of attachment 8728204 [details] [diff] [review]:
-----------------------------------------------------------------

We have a class inheritance tree like:
OpenDatabaseOp->IProtocol->MessageListener->SupportsWeakPtr<MessageListener>.

We create and release the OpenDatabaseOp in different thread.
The OpenDatabaseOp is thread-safe ref-counting, but SupportsWeakPtr<> has a WeakReference member which is inherit from RefCounted(non-thread-safe ref-counting).

In Bug 1251593, I add a thread checking assert in RefCounted. And I hit the assert with OpenDatabaseOp.

I'm trying to do the similar thing as nsWeakReference and nsSupportsWeakReference does[1].
|nsSupportsWeakReference| doesn't hold the reference of nsWeakReference. It just use raw pointer. So there is no nsWeakReference counting problem for nsSupportsWeakReference.

We still can't use WeakPtr in multi-thread, but it will no assert with the OpenDatabaseOp usage.

[1]
https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsWeakReference.cpp

[2]
https://hg.mozilla.org/mozilla-central/diff/287878a32dd2/mfbt/WeakPtr.h#l76
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10)
> We still can't use WeakPtr in multi-thread, but it will no assert with the
> OpenDatabaseOp usage.

And, importantly, the weak reference capability here is only ever used on the PBackground thread, so we don't need to use it on multiple threads.
Comment on attachment 8728204 [details] [diff] [review]
remove the ownership of WeakPtr in SupportsWeakPtr. v2

The idea is good, I think, but the debug printfs make the patch really hard to read.  Also this needs an MFBT peer review, I think.
Attachment #8728204 - Flags: review?(ehsan)
Remove the debug message in attachment 8728204 [details] [diff] [review].
Please check the comment 10 for the detail.
Attachment #8728812 - Flags: review?(nfroyd)
Attachment #8728812 - Flags: review?(jwalden+bmo)
Attachment #8728812 - Flags: review?(ehsan)
Posted patch debug message of WeakPtr. v1 (obsolete) — Splinter Review
These messages are used for debugging with my previous patch. I'm not sure that it's worth to put them in mfbt.
Attachment #8728813 - Flags: review?(nfroyd)
Attachment #8728813 - Flags: review?(jwalden+bmo)
Attachment #8728813 - Flags: review?(ehsan)
Attachment #8728204 - Attachment is obsolete: true
Attachment #8728204 - Flags: review?(nfroyd)
Attachment #8728204 - Flags: review?(jwalden+bmo)
Comment on attachment 8728813 [details] [diff] [review]
debug message of WeakPtr. v1

Review of attachment 8728813 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for separating these out, these changes make the main patch much cleaner.

But I think when Ehsan said that it was hard to read the patch with debug messages, he meant that the debug messages should be deleted entirely, not separated out into a separate patch.
Attachment #8728813 - Flags: review?(nfroyd)
Comment on attachment 8728812 [details] [diff] [review]
remove the ownership of WeakPtr in SupportsWeakPtr. v3

Yes, I meant the debug messages should be deleted.  I also meant that this should be reviewed by an MFBT peer, not me.  :-)
Attachment #8728812 - Flags: review?(ehsan)
Attachment #8728813 - Flags: review?(ehsan)
Attachment #8728813 - Attachment is obsolete: true
Attachment #8728813 - Flags: review?(jwalden+bmo)
Comment on attachment 8728812 [details] [diff] [review]
remove the ownership of WeakPtr in SupportsWeakPtr. v3

Review of attachment 8728812 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for debugging this; apologies for the wait!
Attachment #8728812 - Flags: review?(nfroyd) → review+
Attachment #8728812 - Flags: review?(jwalden+bmo)
Keywords: checkin-needed
Summary: Race condition of mozilla::dom::indexDB::OpenDatabaseOp WeakReference ref-counting → remove the ownership of WeakPtr in SupportsWeakPtr
Attachment #8728812 - Attachment is obsolete: true
Please land the attachment 8731745 [details] [diff] [review] to m-c.
This patch should before bug 1251593.
Blocks: 1258263
You need to log in before you can comment on or make changes to this bug.