170967 2017-04-18 14:52:43 -0700 ASAN Crash running LayoutTests/inspector/worker tests 2017-04-19 14:02:01 -0700 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck achristensen ap bburg bfulgham commit-queue ddkilzer joepeck webkit-bug-importer oldest_to_newest 1298443 0 joepeck 2017-04-18 14:52:43 -0700 Summary: ASAN Crash running LayoutTests/inspector/worker tests Crash: Crashed Thread: 0 Dispatch queue: com.apple.main-thread #0 0x11a673672 in WebCore::WorkerMessagingProxy::postMessageToPageInspector(WTF::String const&)::$_3::operator()() const (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/WebCore.framework/Versions/A/WebCore+0x2f0f672) #1 0x112099403 in WTF::RunLoop::performWork() (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1abe403) #2 0x112099c6e in WTF::RunLoop::performWork(void*) (/Volumes/Data/slave/sierra-asan-release-wk2-tests/build/buildToTest/JavaScriptCore.framework/Versions/A/JavaScriptCore+0x1abec6e) #3 0x7fff89ed83b0 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0xa43b0) #4 0x7fff89eb963b in __CFRunLoopDoSources0 (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x8563b) #5 0x7fff89eb8b25 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation+0x84b25) Notes: - Looks like this can happen if the WorkerMessagingProxy (which deletes itself) deletes itself while there is a still a postMessageToPageInspector block used for that proxy. - I was unable to reproduce the test running LayoutTests/inspector/worker tests with GuardMalloc 100 iterations - I was able to reproduce by forcing this theory to happen (call postMessageToPageInspector when triggering the proxy to go away) Normal solution for these kinds of situations is to use ref counted objects. 1298444 1 joepeck 2017-04-18 14:52:56 -0700 <rdar://problem/31256437> 1298446 2 307419 joepeck 2017-04-18 14:55:15 -0700 Created attachment 307419 [PATCH] Proposed Fix 1298784 3 307419 achristensen 2017-04-19 12:53:37 -0700 Comment on attachment 307419 [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=307419&action=review > Source/WebCore/workers/WorkerMessagingProxy.cpp:242 > - delete this; > + deref(); This is strange lifetime management. Is there really nothing that can own this? This change does make it so we can protect it in the lambda, though, so r=me on that. 1298795 4 307419 commit-queue 2017-04-19 13:22:52 -0700 Comment on attachment 307419 [PATCH] Proposed Fix Clearing flags on attachment: 307419 Committed r215528: <http://trac.webkit.org/changeset/215528> 1298796 5 commit-queue 2017-04-19 13:22:54 -0700 All reviewed patches have been landed. Closing bug. 1298811 6 joepeck 2017-04-19 14:02:01 -0700 (In reply to Alex Christensen from comment #3) > Comment on attachment 307419 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307419&action=review > > > Source/WebCore/workers/WorkerMessagingProxy.cpp:242 > > - delete this; > > + deref(); > > This is strange lifetime management. Is there really nothing that can own > this? This change does make it so we can protect it in the lambda, though, > so r=me on that. This is the clue I had to go on: WorkerGlobalScopeProxy& m_contextProxy; // The proxy outlives the worker to perform thread shutdown. --- From the Worker Creator side (right now only Document on the Main Thread). The Worker object creates the Messaging proxy: inline Worker::Worker(ScriptExecutionContext& context, JSC::RuntimeFlags runtimeFlags) ... , m_contextProxy(WorkerGlobalScopeProxy::create(*this)) And ~Worker tells the proxy to shut down the thread and destroy itself: Worker::~Worker() { ... m_contextProxy.workerObjectDestroyed(); } Which then goes through phases: • if WorkerThread => terminate worker thread • terminate self ---- From the Worker Thread side. The WorkerGlobalScope has close() which terminates the thread but doesn't get rid of the Worker object which is the only object allowed to eventually cause deletion of the Proxy. ---- So it looks like the current behavior is: • Worker creates the Proxy • Worker can be destroyed before the WorkerGlobalScope (WorkerThread) => if the Worker object is GC'd but the worker thread is still alive and running => in this case someone must terminate the worker thread. In that case, it is the responsibility of the Proxy to: • Terminate the Worker Thread • Destroy itself So this really is a situation where there isn't a really good strong owner, since we want GC to be able to delete everything. 307419 2017-04-18 14:55:15 -0700 2017-04-19 13:22:52 -0700 [PATCH] Proposed Fix asan-1.patch text/plain 4105 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCAwZTBlM2JhYzVjZi4uNTU3YWUxMzNlOGYgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL0NoYW5nZUxvZworKysgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMyBAQAorMjAxNy0wNC0xOCAgSm9zZXBoIFBlY29yYXJvICA8cGVjb3Jhcm9AYXBwbGUu Y29tPgorCisgICAgICAgIEFTQU4gQ3Jhc2ggcnVubmluZyBMYXlvdXRUZXN0cy9pbnNwZWN0b3Iv d29ya2VyIHRlc3RzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xNzA5NjcKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzMxMjU2NDM3PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogd29ya2Vycy9Xb3JrZXJN ZXNzYWdpbmdQcm94eS5oOgorICAgICAgICAqIHdvcmtlcnMvV29ya2VyTWVzc2FnaW5nUHJveHku Y3BwOgorICAgICAgICAoV2ViQ29yZTo6V29ya2VyTWVzc2FnaW5nUHJveHk6Oldvcmtlck1lc3Nh Z2luZ1Byb3h5KToKKyAgICAgICAgKFdlYkNvcmU6Oldvcmtlck1lc3NhZ2luZ1Byb3h5Ojp3b3Jr ZXJHbG9iYWxTY29wZURlc3Ryb3llZEludGVybmFsKToKKyAgICAgICAgTWFrZSB0aGUgTWVzc2Fn aW5nUHJveHkgdGhyZWFkIHNhZmUgcmVmIGNvdW50ZWQuIFNpbmNlIGl0IHVzZWQgdG8KKyAgICAg ICAgZGVsZXRlIGl0c2VsZiwgdHVybiB0aGlzIGludG8gYSByZWYgKGltcGxpY2l0IG9uIGNvbnN0 cnVjdGlvbikKKyAgICAgICAgYW5kIGRlcmVmIChyZXBsYWNpbmcgZGVsZXRlIHRoaXMpLgorCisg ICAgICAgIChXZWJDb3JlOjpXb3JrZXJNZXNzYWdpbmdQcm94eTo6cG9zdE1lc3NhZ2VUb1BhZ2VJ bnNwZWN0b3IpOgorICAgICAgICBXaGVuIGRpc3BhdGNoaW5nIGhhdmUgdGhlIGxhbWJkYSBpbXBs aWNpdGx5IHJlZi9kZXJlZiB3aXRoIHRoZQorICAgICAgICBsYW1iZGEgdG8ga2VlcCB0aGUgcHJv eHkgYWxpdmUgd2hpbGUgYSBsYW1iZGEgaXMgcXVldWVkLgorCiAyMDE3LTA0LTEzICBKb3NlcGgg UGVjb3Jhcm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKICAgICAgICAgRW5vdWdoIHdpdGggdGhl c2UgZmF2aWNvbi5pY28gZXJyb3JzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS93b3JrZXJz L1dvcmtlck1lc3NhZ2luZ1Byb3h5LmNwcCBiL1NvdXJjZS9XZWJDb3JlL3dvcmtlcnMvV29ya2Vy TWVzc2FnaW5nUHJveHkuY3BwCmluZGV4IDNjZTg2OThiYjJhLi5kMjEyZjZjMWY5NSAxMDA2NDQK LS0tIGEvU291cmNlL1dlYkNvcmUvd29ya2Vycy9Xb3JrZXJNZXNzYWdpbmdQcm94eS5jcHAKKysr IGIvU291cmNlL1dlYkNvcmUvd29ya2Vycy9Xb3JrZXJNZXNzYWdpbmdQcm94eS5jcHAKQEAgLTU4 LDYgKzU4LDkgQEAgV29ya2VyTWVzc2FnaW5nUHJveHk6Oldvcmtlck1lc3NhZ2luZ1Byb3h5KFdv cmtlciYgd29ya2VyT2JqZWN0KQogewogICAgIEFTU0VSVCgoaXM8RG9jdW1lbnQ+KCptX3Njcmlw dEV4ZWN1dGlvbkNvbnRleHQpICYmIGlzTWFpblRocmVhZCgpKQogICAgICAgICB8fCAoaXM8V29y a2VyR2xvYmFsU2NvcGU+KCptX3NjcmlwdEV4ZWN1dGlvbkNvbnRleHQpICYmIGN1cnJlbnRUaHJl YWQoKSA9PSBkb3duY2FzdDxXb3JrZXJHbG9iYWxTY29wZT4oKm1fc2NyaXB0RXhlY3V0aW9uQ29u dGV4dCkudGhyZWFkKCkudGhyZWFkSUQoKSkpOworCisgICAgLy8gTm9ib2R5IG91dHNpZGUgdGhp cyBjbGFzcyByZWYgY291bnRzIHRoaXMgb2JqZWN0LiBUaGUgb3JpZ2luYWwgcmVmCisgICAgLy8g aXMgYmFsYW5jZWQgYnkgdGhlIGRlcmVmIGluIHdvcmtlckdsb2JhbFNjb3BlRGVzdHJveWVkSW50 ZXJuYWwuCiB9CiAKIFdvcmtlck1lc3NhZ2luZ1Byb3h5Ojp+V29ya2VyTWVzc2FnaW5nUHJveHko KQpAQCAtMTYyLDggKzE2NSw5IEBAIHZvaWQgV29ya2VyTWVzc2FnaW5nUHJveHk6OnBvc3RFeGNl cHRpb25Ub1dvcmtlck9iamVjdChjb25zdCBTdHJpbmcmIGVycm9yTWVzc2FnCiAKIHZvaWQgV29y a2VyTWVzc2FnaW5nUHJveHk6OnBvc3RNZXNzYWdlVG9QYWdlSW5zcGVjdG9yKGNvbnN0IFN0cmlu ZyYgbWVzc2FnZSkKIHsKLSAgICBSdW5Mb29wOjptYWluKCkuZGlzcGF0Y2goW3RoaXMsIG1lc3Nh Z2UgPSBtZXNzYWdlLmlzb2xhdGVkQ29weSgpXSB7Ci0gICAgICAgIG1faW5zcGVjdG9yUHJveHkt PnNlbmRNZXNzYWdlRnJvbVdvcmtlclRvRnJvbnRlbmQobWVzc2FnZSk7CisgICAgUnVuTG9vcDo6 bWFpbigpLmRpc3BhdGNoKFt0aGlzLCBwcm90ZWN0ZWRUaGlzID0gbWFrZVJlZigqdGhpcyksIG1l c3NhZ2UgPSBtZXNzYWdlLmlzb2xhdGVkQ29weSgpXSB7CisgICAgICAgIGlmICghbV9tYXlCZURl c3Ryb3llZCkKKyAgICAgICAgICAgIG1faW5zcGVjdG9yUHJveHktPnNlbmRNZXNzYWdlRnJvbVdv cmtlclRvRnJvbnRlbmQobWVzc2FnZSk7CiAgICAgfSk7CiB9CiAKQEAgLTIzMyw4ICsyMzcsOSBA QCB2b2lkIFdvcmtlck1lc3NhZ2luZ1Byb3h5Ojp3b3JrZXJHbG9iYWxTY29wZURlc3Ryb3llZElu dGVybmFsKCkKIAogICAgIG1faW5zcGVjdG9yUHJveHktPndvcmtlclRlcm1pbmF0ZWQoKTsKIAor ICAgIC8vIFRoaXMgYmFsYW5jZXMgdGhlIG9yaWdpbmFsIHJlZiBpbiBjb25zdHJ1Y3Rpb24uCiAg ICAgaWYgKG1fbWF5QmVEZXN0cm95ZWQpCi0gICAgICAgIGRlbGV0ZSB0aGlzOworICAgICAgICBk ZXJlZigpOwogfQogCiB2b2lkIFdvcmtlck1lc3NhZ2luZ1Byb3h5Ojp0ZXJtaW5hdGVXb3JrZXJH bG9iYWxTY29wZSgpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS93b3JrZXJzL1dvcmtlck1l c3NhZ2luZ1Byb3h5LmggYi9Tb3VyY2UvV2ViQ29yZS93b3JrZXJzL1dvcmtlck1lc3NhZ2luZ1By b3h5LmgKaW5kZXggNzY2Mjk4ODk0NWIuLjhmZDNjZDZhM2Y5IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS93b3JrZXJzL1dvcmtlck1lc3NhZ2luZ1Byb3h5LmgKKysrIGIvU291cmNlL1dlYkNv cmUvd29ya2Vycy9Xb3JrZXJNZXNzYWdpbmdQcm94eS5oCkBAIC0yOCwyMCArMjgsMjAgQEAKICNp bmNsdWRlICJXb3JrZXJHbG9iYWxTY29wZVByb3h5LmgiCiAjaW5jbHVkZSAiV29ya2VyTG9hZGVy UHJveHkuaCIKICNpbmNsdWRlICJXb3JrZXJPYmplY3RQcm94eS5oIgorI2luY2x1ZGUgPHd0Zi9U aHJlYWRTYWZlUmVmQ291bnRlZC5oPgogCiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKIGNsYXNzIERl ZGljYXRlZFdvcmtlclRocmVhZDsKIGNsYXNzIFdvcmtlckluc3BlY3RvclByb3h5OwogCi1jbGFz cyBXb3JrZXJNZXNzYWdpbmdQcm94eSBmaW5hbCA6IHB1YmxpYyBXb3JrZXJHbG9iYWxTY29wZVBy b3h5LCBwdWJsaWMgV29ya2VyT2JqZWN0UHJveHksIHB1YmxpYyBXb3JrZXJMb2FkZXJQcm94eSB7 CitjbGFzcyBXb3JrZXJNZXNzYWdpbmdQcm94eSBmaW5hbCA6IHB1YmxpYyBUaHJlYWRTYWZlUmVm Q291bnRlZDxXb3JrZXJNZXNzYWdpbmdQcm94eT4sIHB1YmxpYyBXb3JrZXJHbG9iYWxTY29wZVBy b3h5LCBwdWJsaWMgV29ya2VyT2JqZWN0UHJveHksIHB1YmxpYyBXb3JrZXJMb2FkZXJQcm94eSB7 CiAgICAgV1RGX01BS0VfRkFTVF9BTExPQ0FURUQ7CiBwdWJsaWM6CiAgICAgZXhwbGljaXQgV29y a2VyTWVzc2FnaW5nUHJveHkoV29ya2VyJik7Ci0KLXByaXZhdGU6CiAgICAgdmlydHVhbCB+V29y a2VyTWVzc2FnaW5nUHJveHkoKTsKIAorcHJpdmF0ZToKICAgICAvLyBJbXBsZW1lbnRhdGlvbnMg b2YgV29ya2VyR2xvYmFsU2NvcGVQcm94eS4KICAgICAvLyAoT25seSB1c2UgdGhlc2UgZnVuY3Rp b25zIGluIHRoZSB3b3JrZXIgb2JqZWN0IHRocmVhZC4pCiAgICAgdm9pZCBzdGFydFdvcmtlckds b2JhbFNjb3BlKGNvbnN0IFVSTCYgc2NyaXB0VVJMLCBjb25zdCBTdHJpbmcmIHVzZXJBZ2VudCwg Y29uc3QgU3RyaW5nJiBzb3VyY2VDb2RlLCBjb25zdCBDb250ZW50U2VjdXJpdHlQb2xpY3lSZXNw b25zZUhlYWRlcnMmLCBib29sIHNob3VsZEJ5cGFzc01haW5Xb3JsZENvbnRlbnRTZWN1cml0eVBv bGljeSwgTW9ub3RvbmljVGltZSB0aW1lT3JpZ2luLCBKU0M6OlJ1bnRpbWVGbGFncykgZmluYWw7 Cg==