Open
Bug 1251559
Opened 8 years ago
Updated 1 year ago
remove the ownership of WeakPtr in SupportsWeakPtr
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
NEW
People
(Reporter: jerry, Unassigned)
References
(Blocks 2 open bugs)
Details
(Whiteboard: btpp-followup-2016-03-08)
Attachments
(1 file, 4 obsolete files)
3.80 KB,
patch
|
jerry
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•8 years ago
|
Flags: needinfo?(khuey)
Flags: needinfo?(bent.mozilla)
Comment 1•8 years ago
|
||
(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)
Updated•8 years ago
|
Whiteboard: btpp-followup-2016-03-04
Reporter | ||
Comment 3•8 years ago
|
||
(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)
Reporter | ||
Comment 4•8 years ago
|
||
(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)
Comment 5•8 years ago
|
||
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)
Updated•8 years ago
|
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)
Reporter | ||
Comment 7•8 years ago
|
||
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
Reporter | ||
Comment 8•8 years ago
|
||
Reporter | ||
Comment 9•8 years ago
|
||
Attachment #8728204 -
Flags: review?(nfroyd)
Attachment #8728204 -
Flags: review?(jwalden+bmo)
Attachment #8728204 -
Flags: review?(ehsan)
Reporter | ||
Updated•8 years ago
|
Attachment #8727766 -
Attachment is obsolete: true
Reporter | ||
Comment 10•8 years ago
|
||
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 12•8 years ago
|
||
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)
Reporter | ||
Comment 13•8 years ago
|
||
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)
Reporter | ||
Comment 14•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Attachment #8728204 -
Attachment is obsolete: true
Attachment #8728204 -
Flags: review?(nfroyd)
Attachment #8728204 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 15•8 years ago
|
||
try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ee29955bfba
Comment 16•8 years ago
|
||
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 17•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8728813 -
Flags: review?(ehsan)
Reporter | ||
Updated•8 years ago
|
Attachment #8728813 -
Attachment is obsolete: true
Attachment #8728813 -
Flags: review?(jwalden+bmo)
Comment 18•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Attachment #8728812 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•8 years ago
|
Keywords: checkin-needed
Summary: Race condition of mozilla::dom::indexDB::OpenDatabaseOp WeakReference ref-counting → remove the ownership of WeakPtr in SupportsWeakPtr
Reporter | ||
Updated•8 years ago
|
Attachment #8728812 -
Attachment is obsolete: true
Reporter | ||
Comment 20•8 years ago
|
||
Please land the attachment 8731745 [details] [diff] [review] to m-c. This patch should before bug 1251593.
Comment 21•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd8db347c49
Keywords: checkin-needed
Comment 22•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/20a5b1e76922
Comment 23•2 years ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months.
:sg, could you have a look please?
For more information, please visit auto_nag documentation.
Assignee: bignose1007+bugzilla → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(simon.giesecke)
Updated•2 years ago
|
Flags: needinfo?(simon.giesecke)
Updated•1 year ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•