Modern IDB: Some blob tests ASSERT sometimes on the bots

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

Modern IDB: Some blob tests ASSERT sometimes on the bots

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r200618%20(5111)/results.html

[Brady Eidson]

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]

*** Bug 157543 has been marked as a duplicate of this bug. ***

[Brady Eidson]

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]

I cannot reproduce this no matter how many iterations and combinations of tests I try.

[Brady Eidson]

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]

This assertion can fire on both main thread and secondary threads. Attaching a couple crash logs.

[Alexey Proskuryakov]

Created attachment 278975 [details]
main thread

[Alexey Proskuryakov]

Created attachment 278976 [details]
BlobUtility thread

[Alexey Proskuryakov]

This is pretty frequent on the bots, sadly.

[Brady Eidson]

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]

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]

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

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]

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]

We have C++14 now. I'm working on a resolution to this today.

[Brady Eidson]

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 on attachment 279598 [details]
Speculative fix

Move all the things!

[Brady Eidson]

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]

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]

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

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]

And, of course, since this fix was speculative, we'll just reopen this if it didn't work.

But it should.