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)
<rdar://problem/16217802>
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.
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?
(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.
(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.
Marked skipped in <https://trac.webkit.org/changeset/165061>
(In reply to comment #2) > StorageTracker.cpp:455: ASSERT(isMainThread()); Aren't we on the main thread?!
Oh, I see: Crashed Thread: 25 WebCore: LocalStorage
(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.
(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.
(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.
Saw this on inspector/dom/content-flow-content-removal.html today. Did we get more tests that use the harness?
(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.
Created attachment 238679 [details] Patch
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.
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.
Committed r173991: <http://trac.webkit.org/changeset/173991>
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.
(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?
> if the local is instead a String&& databaseIdentifierCopy I don't think that would compile.
(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.
(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.
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>.
(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 :)
(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!