StorageTracker::deleteOrigin being called off the main thread (ASSERTs in inspector/test-harness-trivially-works.html test)

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

inspector/test-harness-trivially-works.html frequently asserts:

http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r165013%20(3509)/inspector/test-harness-trivially-works-crash-log.txt

Thread 25 Crashed:: WebCore: LocalStorage
0   com.apple.JavaScriptCore      	0x0000000101d74b2a WTFCrash + 42 (Assertions.cpp:333)
1   com.apple.WebCore             	0x00000001050e4db2 WebCore::StorageTracker::deleteOrigin(WebCore::SecurityOrigin*) + 130 (StorageTracker.cpp:455)
2   com.apple.WebCore             	0x00000001050e402d WebCore::StorageTracker::deleteOriginWithIdentifier(WTF::String const&) + 61 (StorageTracker.cpp:449)
3   com.apple.WebCore             	0x00000001050cd815 WebCore::StorageAreaSync::deleteEmptyDatabase() + 421 (StorageAreaSync.cpp:517)
4   com.apple.WebCore             	0x00000001050cfac2 WTF::FunctionWrapper<void (WebCore::StorageAreaSync::*)()>::operator()(WebCore::StorageAreaSync*) + 114 (Functional.h:218)
5   com.apple.WebCore             	0x00000001050cfa45 WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (WebCore::StorageAreaSync::*)()>, void (WebCore::StorageAreaSync*)>::operator()() + 53 (Functional.h:496)

[Radar WebKit Bug Importer]

<rdar://problem/16217802>

[Timothy Hatcher]

StorageTracker.cpp:455: ASSERT(isMainThread());

This is an indication of a treading issue or the assertion is bad. It does not seem Inspector specific, this test does nothing more than test the Inspector test harness. The Inspector is closing when this happens.

[Alexey Proskuryakov]

What should we do in the meanwhile? Would skipping this particular test be sufficient? Or does an assertion  in "trivially works" test mean that all Inspector tests are affected?

[BJ Burg]

(In reply to comment #3)
> What should we do in the meanwhile? Would skipping this particular test be sufficient? Or does an assertion  in "trivially works" test mean that all Inspector tests are affected?

It would affect tests in LayoutTests/inspector/ but not LayoutTests/inspector-protocol. The latter uses a dummy page and doesn't actually open up an inspector instance through WK/WK2.

I was working to move tests in LayoutTests/inspector-protocol/ to LayoutTests/inspector, so enabling those tests after moving them to the different test harness is blocked by this flakiness.

[BJ Burg]

(In reply to comment #4)
> (In reply to comment #3)
> > What should we do in the meanwhile? Would skipping this particular test be sufficient? Or does an assertion  in "trivially works" test mean that all Inspector tests are affected?

So, it could be skipped for now to make bots green, but it should be addressed ASAP to unblock writing replay tests.

[BJ Burg]

Marked skipped in <https://trac.webkit.org/changeset/165061>

[Joseph Pecoraro]

(In reply to comment #2)
> StorageTracker.cpp:455: ASSERT(isMainThread());

Aren't we on the main thread?!

[Joseph Pecoraro]

Oh, I see: Crashed Thread:  25  WebCore: LocalStorage

[Brady Eidson]

(In reply to comment #2)
>this test does nothing more than test the Inspector test harness. 

What is "the inspector test harness"?  DRT?  WebKitTestRunner?  Or some other executable?

I ask, because a WebKit app that enables LocalStorage is also required to explicitly shut down local storage before it exits.  DRT and WKTR do this.

[Brady Eidson]

(In reply to comment #9)
> (In reply to comment #2)
> >this test does nothing more than test the Inspector test harness. 
> 
> What is "the inspector test harness"?  DRT?  WebKitTestRunner?  Or some other executable?
> 
> I ask, because a WebKit app that enables LocalStorage is also required to explicitly shut down local storage before it exits.  DRT and WKTR do this.

I guess I misunderstood the significance of "Inspector test harness" as this just appears to be DRT crashing.

[Timothy Hatcher]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #2)
> > >this test does nothing more than test the Inspector test harness. 
> > 
> > What is "the inspector test harness"?  DRT?  WebKitTestRunner?  Or some other executable?
> > 
> > I ask, because a WebKit app that enables LocalStorage is also required to explicitly shut down local storage before it exits.  DRT and WKTR do this.
> 
> I guess I misunderstood the significance of "Inspector test harness" as this just appears to be DRT crashing.

Yes, "Inspector test harness" just runs inside DRT and WKTR.

[Alexey Proskuryakov]

Saw this on inspector/dom/content-flow-content-removal.html today. Did we get more tests that use the harness?

[Brian Burg]

(In reply to comment #12)
> Saw this on inspector/dom/content-flow-content-removal.html today. Did we get more tests that use the harness?

Everything in the inspector/ directory was ported at the same time to the new test "harness". I think any test that uses a real inspector instance–everything under inspector/–could hit this assertion.

[Brian Burg]

Created attachment 238679 [details]
Patch

[Brady Eidson]

We really need an easy way to take sudden termination assertions while all these main thread operations are still outstanding.

This introduces a new one, but there's so many already that it's okay for now.

[Alexey Proskuryakov]

Comment on attachment 238679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238679&action=review

> Source/WebCore/storage/StorageAreaSync.cpp:526
> +            String databaseIdentifierCopy = m_databaseIdentifier.isolatedCopy();
> +            callOnMainThread([databaseIdentifierCopy] {

This is not thread safe. A new copy of the string is made for the capture, and databaseIdentifierCopy races with it for derefing.

[Brian Burg]

Committed r173991: <http://trac.webkit.org/changeset/173991>

[Alexey Proskuryakov]

In the landed patch, what guarantees that "this" object is still alive when code executes on the main thread? I'm concerned that this might have introduced a potential use-after-free security bug.

If you just needed to pass a string, you could make an isolated StringImpl*, and manually delete it when done.

[Brian Burg]

(In reply to comment #16)
> (From update of attachment 238679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238679&action=review
> 
> > Source/WebCore/storage/StorageAreaSync.cpp:526
> > +            String databaseIdentifierCopy = m_databaseIdentifier.isolatedCopy();
> > +            callOnMainThread([databaseIdentifierCopy] {
> 
> This is not thread safe. A new copy of the string is made for the capture, and databaseIdentifierCopy races with it for derefing.

I might be dense, but.. if the local is instead a String&& databaseIdentifierCopy, then this should cause capture to transfer the StringImpl ref from databesIdentifierCopy to the captured version, right?

[Alexey Proskuryakov]

> if the local is instead a String&& databaseIdentifierCopy

I don't think that would compile.

[Anders Carlsson]

(In reply to comment #20)
> > if the local is instead a String&& databaseIdentifierCopy
> 
> I don't think that would compile.

C++11 doesn't support capture-by-move in lambdas. C++14 does.

[Brady Eidson]

(In reply to comment #17)
> Committed r173991: <http://trac.webkit.org/changeset/173991>

I might have r+'ed, but based on Alexey's feedback afterwards I don't think this patch should have been landed as-is.

[Alexey Proskuryakov]

It's not evident from the bug, but Brian did account for my feedback when landing. That was still not quite correct, so he followed up with a better fix in <http://trac.webkit.org/changeset/174014>.

[Brady Eidson]

(In reply to comment #23)
> It's not evident from the bug, but Brian did account for my feedback when landing. That was still not quite correct, so he followed up with a better fix in <http://trac.webkit.org/changeset/174014>.

Ack, my apologies - Saw the continued chatter without actually looking at the landed patch :)

[Brian Burg]

(In reply to comment #24)
> (In reply to comment #23)
> > It's not evident from the bug, but Brian did account for my feedback when landing. That was still not quite correct, so he followed up with a better fix in <http://trac.webkit.org/changeset/174014>.
> 
> Ack, my apologies - Saw the continued chatter without actually looking at the landed patch :)

I assumed that commit-queue would add a link to the fixup commit here, but I must have used dcommit or something. Sorry for the confusion!