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
Brady Eidson
2016-05-10 10:31:33 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. *** Bug 157543 has been marked as a duplicate of this bug. *** 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. I cannot reproduce this no matter how many iterations and combinations of tests I try. 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. This assertion can fire on both main thread and secondary threads. Attaching a couple crash logs. Created attachment 278975 [details]
main thread
Created attachment 278976 [details]
BlobUtility thread
This is pretty frequent on the bots, sadly. I'll be taking care of this after I resolve https://bugs.webkit.org/show_bug.cgi?id=157917, which seems much more pressing. The "main thread" crash Alexey attached is not actually related to blobs - It's a worker-related crash. That one seems more imminently explorable. (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) 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. 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 We have C++14 now. I'm working on a resolution to this today. 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.
Comment on attachment 279598 [details]
Speculative fix
Move all the things!
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. 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. (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. 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 And, of course, since this fix was speculative, we'll just reopen this if it didn't work. But it should. |