Bug 157525

Summary: Modern IDB: Some blob tests ASSERT sometimes on the bots
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebCore Misc.Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, ap, commit-queue, jsbell, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=158004
Bug Depends on:    
Bug Blocks: 149117, 154968    
Attachments:
Description Flags
main thread
none
BlobUtility thread
none
Speculative fix achristensen: review+

Brady Eidson
Reported 2016-05-10 10:31:33 PDT
Attachments
main thread (128.52 KB, text/plain)
2016-05-15 11:22 PDT, Alexey Proskuryakov
no flags
BlobUtility thread (136.30 KB, text/plain)
2016-05-15 11:22 PDT, Alexey Proskuryakov
no flags
Speculative fix (6.17 KB, patch)
2016-05-23 15:38 PDT, Brady Eidson
achristensen: review+
Brady Eidson
Comment 1 2016-05-10 10:34:41 PDT
Without even thinking about, this seems obvious - Blobs were done before Workers, and that means they were done before the threading assertions were added. Blobs do a little bit of thread hopping, so I'm sure there's just one or two adjustments that need to be made.
Brady Eidson
Comment 2 2016-05-10 15:55:14 PDT
*** Bug 157543 has been marked as a duplicate of this bug. ***
Brady Eidson
Comment 3 2016-05-10 16:11:43 PDT
I will not be able to look into this for awhile, nor do I have a clean enough check out to do anything about it. We should comment out the assert for now, in lieu of skipping all blob tests.
Brady Eidson
Comment 4 2016-05-11 11:41:51 PDT
I cannot reproduce this no matter how many iterations and combinations of tests I try.
Brady Eidson
Comment 5 2016-05-11 11:45:21 PDT
It's somewhat confusing. In these tests that are failing, the TransactionOperations are being created on the main thread, so they must be torn down on a background thread for the ASSERT to fire. However, I can't see the code path where it's possible for them to be torn down on a background thread.
Alexey Proskuryakov
Comment 6 2016-05-15 11:21:56 PDT
This assertion can fire on both main thread and secondary threads. Attaching a couple crash logs.
Alexey Proskuryakov
Comment 7 2016-05-15 11:22:15 PDT
Created attachment 278975 [details] main thread
Alexey Proskuryakov
Comment 8 2016-05-15 11:22:40 PDT
Created attachment 278976 [details] BlobUtility thread
Alexey Proskuryakov
Comment 9 2016-05-19 13:28:19 PDT
This is pretty frequent on the bots, sadly.
Brady Eidson
Comment 10 2016-05-19 14:08:20 PDT
I'll be taking care of this after I resolve https://bugs.webkit.org/show_bug.cgi?id=157917, which seems much more pressing.
Brady Eidson
Comment 11 2016-05-20 09:37:16 PDT
The "main thread" crash Alexey attached is not actually related to blobs - It's a worker-related crash. That one seems more imminently explorable.
Brady Eidson
Comment 12 2016-05-20 09:37:44 PDT
(In reply to comment #11) > The "main thread" crash Alexey attached is not actually related to blobs - > It's a worker-related crash. > > That one seems more imminently explorable. Maybe not, I don't know why I thought that. Sucks I can't reproduce any of these. But there's at least two different things going on here (blobs and workers)
Brady Eidson
Comment 13 2016-05-20 09:41:29 PDT
It's related to nesting these lambdas, having one call the other, etc etc., and then the order in which they are torn down. The teardown order is obviously racey. This would be *simply* solved if we had C++14 lambda capture semantics, because we could move the RefPtr around instead of copying it. *sigh* I'll figure something out.
Brady Eidson
Comment 14 2016-05-20 09:57:34 PDT
In general, all of the blob writing code gets Workers wrong, as it presumes MainThreadedness for the callbacks. Filed https://bugs.webkit.org/show_bug.cgi?id=157947 for that
Brady Eidson
Comment 15 2016-05-23 12:39:36 PDT
We have C++14 now. I'm working on a resolution to this today.
Brady Eidson
Comment 16 2016-05-23 15:38:02 PDT
Created attachment 279598 [details] Speculative fix This is speculative in that I could never nail down a way to reproduce the bug nearly as reliably as the bots do. But reproducibility aside, this should be a positive change.
Alex Christensen
Comment 17 2016-05-23 15:46:48 PDT
Comment on attachment 279598 [details] Speculative fix Move all the things!
Brady Eidson
Comment 18 2016-05-23 16:14:41 PDT
Landed that in http://trac.webkit.org/changeset/201302 It should significantly improve things for the blob test. The other one that Alexey attached is still potentially a problem. The patch landed is likely to improve things, making it less likely to happen. But it's still definitely a potential problem.
Brady Eidson
Comment 19 2016-05-23 16:16:20 PDT
The big problem is in IDBConnectionProxy: template<typename T, typename... Parameters, typename... Arguments> void performCallbackOnCorrectThread(T& object, void (T::*method)(Parameters...), Arguments&&... arguments) { ASSERT(isMainThread()); if (object.originThreadID() == currentThread()) { (object.*method)(arguments...); return; } ScriptExecutionContext* context = object.scriptExecutionContext(); if (!context) return; context->postCrossThreadTask(object, method, arguments...); } It is possible that the worker thread's context has been stopped by the time a TransactionOperation that was created on the worker thread is completed. In that case, it falls into the "if (!context) return;" clause. And when it does that, the operation will be released on the main thread, which is its only option.
Brady Eidson
Comment 20 2016-05-23 16:21:14 PDT
(In reply to comment #19) > > It is possible that the worker thread's context has been stopped by the time > a TransactionOperation that was created on the worker thread is completed. > > In that case, it falls into the "if (!context) return;" clause. > > And when it does that, the operation will be released on the main thread, > which is its only option. So, what do we do for this case? We either convince ourselves that its okay in such cases to destroy that TransactionOperation on the main thread, or we create a way to keep the thread itself alive past the lifetime of the WorkerThread so the transaction operation can be destroyed appropriately. I think the later is the better choice, but *might* be complicated.
Brady Eidson
Comment 21 2016-05-23 16:28:59 PDT
Since this bug was originally and generally about the blob-related ASSERTs, I'll close it. The remaining TransactionOperation issue is now covered separately by https://bugs.webkit.org/show_bug.cgi?id=158004
Brady Eidson
Comment 22 2016-05-23 16:29:23 PDT
And, of course, since this fix was speculative, we'll just reopen this if it didn't work. But it should.
Note You need to log in before you can comment on or make changes to this bug.