Bug 129642

Summary:

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

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

WebKit Misc.

Assignee:

Brady Eidson <beidson>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

andersca, ap, beidson, burg, graouts, joepeck, sam, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar, MakingBotsRed

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

129217

Attachments:

Description	Flags
Patch	beidson: review+

Alexey Proskuryakov

Reported 2014-03-03 16:59:34 PST

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)

Attachments
Patch (5.12 KB, patch) 2014-09-25 15:43 PDT, Brian Burg	beidson: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-03-03 17:00:49 PST

<rdar://problem/16217802>

Timothy Hatcher

Comment 2 2014-03-04 09:51:11 PST

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

Comment 3 2014-03-04 10:09:06 PST

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?

Blaze Burg

Comment 4 2014-03-04 10:15:00 PST

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

Blaze Burg

Comment 5 2014-03-04 10:16:25 PST

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

Blaze Burg

Comment 6 2014-03-04 10:57:40 PST

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

Joseph Pecoraro

Comment 7 2014-03-04 11:18:53 PST

(In reply to comment #2) > StorageTracker.cpp:455: ASSERT(isMainThread()); Aren't we on the main thread?!

Joseph Pecoraro

Comment 8 2014-03-04 11:20:09 PST

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

Brady Eidson

Comment 9 2014-03-10 09:50:56 PDT

(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

Comment 10 2014-03-10 09:51:35 PDT

(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

Comment 11 2014-03-12 11:38:31 PDT

(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

Comment 12 2014-03-29 23:05:35 PDT

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

Brian Burg

Comment 13 2014-03-31 14:25:34 PDT

(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

Comment 14 2014-09-25 15:43:33 PDT

Created attachment 238679 [details] Patch

Brady Eidson

Comment 15 2014-09-25 15:50:50 PDT

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 16 2014-09-25 16:24:23 PDT

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

Comment 17 2014-09-25 19:20:10 PDT

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

Alexey Proskuryakov

Comment 18 2014-09-26 00:02:31 PDT

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

Comment 19 2014-09-26 10:19:03 PDT

(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

Comment 20 2014-09-26 10:45:16 PDT

> if the local is instead a String&& databaseIdentifierCopy I don't think that would compile.

Anders Carlsson

Comment 21 2014-09-26 11:01:14 PDT

(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

Comment 22 2014-09-26 11:29:26 PDT

(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

Comment 23 2014-09-26 11:45:59 PDT

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

Comment 24 2014-09-26 13:03:49 PDT

(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

Comment 25 2014-09-26 13:19:54 PDT

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

Note You need to log in before you can comment on or make changes to this bug.