200504 2019-08-07 08:29:21 -0700 Fix thread safety issue under JSHistory::visitAdditionalChildren() 2019-08-07 13:30:29 -0700 1 1 1 Unclassified WebKit Bindings WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=172738 InRadar P2 Normal --- 200451 1 cdumez cdumez achristensen cdumez commit-queue darin ggaren rniwa simon.fraser webkit-bug-importer oldest_to_newest 1559123 0 cdumez 2019-08-07 08:29:21 -0700 Fix thread safety issue under JSHistory::visitAdditionalChildren(): Thread 7 Crashed:: Heap Helper Thread 0 com.apple.JavaScriptCore 0x0000000109c4e9f0 WTFCrash + 16 (Assertions.cpp:305) 1 com.apple.WebCore 0x000000011a3fb70b WTFCrashWithInfo(int, char const*, char const*, int) + 27 2 com.apple.WebCore 0x000000011a5142a9 WTF::WeakPtr<WebCore::DOMWindow>::operator->() const + 153 (WeakPtr.h:115) 3 com.apple.WebCore 0x000000011d24133f WebCore::DOMWindowProperty::frame() const + 47 (DOMWindowProperty.cpp:42) 4 com.apple.WebCore 0x000000011d2f17f9 WebCore::History::stateInternal() const + 25 (History.cpp:100) 5 com.apple.WebCore 0x000000011d2f1890 WebCore::History::stateChanged() const + 32 (History.cpp:111) 6 com.apple.WebCore 0x000000011d2f1924 WebCore::History::cachedState() + 68 (History.cpp:116) 7 com.apple.WebCore 0x000000011c1782c4 WebCore::JSHistory::visitAdditionalChildren(JSC::SlotVisitor&) + 36 (JSHistoryCustom.cpp:49) 8 com.apple.WebCore 0x000000011b076647 WebCore::JSHistory::visitOutputConstraints(JSC::JSCell*, JSC::SlotVisitor&) + 439 (JSHistory.cpp:405) 9 com.apple.WebCore 0x000000011c11f9d9 WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)::operator()(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const + 105 (DOMGCOutputConstraint.cpp:67) 10 com.apple.WebCore 0x000000011c11f963 WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&)::'lambda'(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind)::operator()(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind) const + 67 (SubspaceInlines.h:105) 11 com.apple.WebCore 0x000000011c11f820 JSC::IterationStatus JSC::MarkedBlock::Handle::forEachMarkedCell<WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&)::'lambda'(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&) + 224 (MarkedBlockInlines.h:546) 12 com.apple.WebCore 0x000000011c11f455 WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&) + 101 (SubspaceInlines.h:102) 13 com.apple.JavaScriptCore 0x000000010ad28620 JSC::MarkingConstraint::doParallelWork(JSC::SlotVisitor&, WTF::SharedTask<void (JSC::SlotVisitor&)>&) + 64 (MarkingConstraint.cpp:87) 14 com.apple.JavaScriptCore 0x000000010ad2a2cd JSC::MarkingConstraintSolver::runExecutionThread(JSC::SlotVisitor&, JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>) + 477 (MarkingConstraintSolver.cpp:232) 15 com.apple.JavaScriptCore 0x000000010ad39cc4 JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>)::$_30::operator()(JSC::SlotVisitor&) const + 68 (MarkingConstraintSolver.cpp:68) 16 com.apple.JavaScriptCore 0x000000010ad39c54 WTF::SharedTaskFunctor<void (JSC::SlotVisitor&), JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>)::$_30>::run(JSC::SlotVisitor&) + 52 (SharedTask.h:91) 17 com.apple.JavaScriptCore 0x000000010ad3db31 JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode, WTF::MonotonicTime) + 1377 (SlotVisitor.cpp:692) 18 com.apple.JavaScriptCore 0x000000010acde7ca JSC::Heap::runBeginPhase(JSC::GCConductor)::$_19::operator()() const + 234 (Heap.cpp:1319) 19 com.apple.JavaScriptCore 0x000000010acde669 WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_19>::run() + 25 (SharedTask.h:91) 20 com.apple.JavaScriptCore 0x0000000109cca1f1 WTF::ParallelHelperClient::runTask(WTF::RefPtr<WTF::SharedTask<void ()>, WTF::DumbPtrTraits<WTF::SharedTask<void ()> > > const&) + 241 (ParallelHelperPool.cpp:115) 21 com.apple.JavaScriptCore 0x0000000109ccb2aa WTF::ParallelHelperPool::Thread::work() + 42 (ParallelHelperPool.cpp:200) 22 com.apple.JavaScriptCore 0x0000000109c65a2f WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const + 671 (AutomaticThread.cpp:223) 23 com.apple.JavaScriptCore 0x0000000109c65619 WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, void>::call() + 25 (Function.h:52) 24 com.apple.JavaScriptCore 0x0000000109c78fad WTF::Function<void ()>::operator()() const + 173 (Function.h:79) 25 com.apple.JavaScriptCore 0x0000000109d12c93 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 419 (Threading.cpp:148) 26 com.apple.JavaScriptCore 0x0000000109d1b4b5 WTF::wtfThreadEntryPoint(void*) + 21 (ThreadingPOSIX.cpp:200) 27 libsystem_pthread.dylib 0x00007fff7432c661 _pthread_body + 340 28 libsystem_pthread.dylib 0x00007fff7432c50d _pthread_start + 377 29 libsystem_pthread.dylib 0x00007fff7432bbf9 thread_start + 13 1559124 1 375699 cdumez 2019-08-07 08:32:28 -0700 Created attachment 375699 Patch 1559154 2 375699 darin 2019-08-07 10:18:41 -0700 Comment on attachment 375699 Patch Do other JSValueInWrappedObject clients have the same kind of mistake? 1559163 3 cdumez 2019-08-07 10:22:28 -0700 (In reply to Darin Adler from comment #2) > Comment on attachment 375699 [details] > Patch > > Do other JSValueInWrappedObject clients have the same kind of mistake? It does not look like it, the other users seem to have simple getters for the JSValueInWrappedObject. 1559172 4 375699 commit-queue 2019-08-07 10:31:17 -0700 Comment on attachment 375699 Patch Clearing flags on attachment: 375699 Committed r248376: <https://trac.webkit.org/changeset/248376> 1559173 5 commit-queue 2019-08-07 10:31:18 -0700 All reviewed patches have been landed. Closing bug. 1559174 6 webkit-bug-importer 2019-08-07 10:32:17 -0700 <rdar://problem/54039531> 1559182 7 darin 2019-08-07 11:04:18 -0700 This bug is evidence that the JSValueInWrappedObject idiom isn’t foolproof enough. Might be worth thinking about how to make sure people are unlikely to make mistakes like this in the future. 1559187 8 375699 ggaren 2019-08-07 11:09:37 -0700 Comment on attachment 375699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review > Source/WebCore/page/History.h:56 > + JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; } I think we decided to call functions like this cachedStateIfExists(). 1559214 9 375699 cdumez 2019-08-07 11:36:42 -0700 Comment on attachment 375699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review >> Source/WebCore/page/History.h:56 >> + JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; } > > I think we decided to call functions like this cachedStateIfExists(). I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date. 1559236 10 375699 darin 2019-08-07 12:13:13 -0700 Comment on attachment 375699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review >>> Source/WebCore/page/History.h:56 >>> + JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; } >> >> I think we decided to call functions like this cachedStateIfExists(). > > I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date. Given the specific behavior, another accurate name besides cachedStateForGC could be cachedStateWithoutRevalidation. As I said, it might be more important to come up with a great idiom for correctly using JSValueInWrappedObject; I’m irritated that it’s easy to accidentally use it wrong, and there might be a simple way to make it better. 1559281 11 375699 rniwa 2019-08-07 13:23:35 -0700 Comment on attachment 375699 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review >>>> Source/WebCore/page/History.h:56 >>>> + JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; } >>> >>> I think we decided to call functions like this cachedStateIfExists(). >> >> I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date. > > Given the specific behavior, another accurate name besides cachedStateForGC could be cachedStateWithoutRevalidation. > > As I said, it might be more important to come up with a great idiom for correctly using JSValueInWrappedObject; I’m irritated that it’s easy to accidentally use it wrong, and there might be a simple way to make it better. In the past, I've suggested using suffix "concurrently" since what's significant here is the fact it can be called concurrently not so much that it's used by GC. Someone may come to this code and change it again to be something not thread safe in the future again if the person didn't understand that our GC does things concurrently. 375699 2019-08-07 08:32:28 -0700 2019-08-07 10:31:17 -0700 Patch bug-200504-20190807083227.patch text/plain 2300 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ4MzY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZTQ2MjFjM2E3MGY5N2Yz NzZkMzQ1ZDRiOTY4MjdkMDE2Y2UyMzI5Yy4uZjAyZTg4ZjFlODkyMTFjNjMyNjY4ZDgyYWE5ZDIw YmViMjUwYWRkZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDE5LTA4LTA3ICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgRml4IHRocmVhZCBzYWZldHkg aXNzdWUgdW5kZXIgSlNIaXN0b3J5Ojp2aXNpdEFkZGl0aW9uYWxDaGlsZHJlbigpCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDA1MDQKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBKU0hpc3Rvcnk6OnZpc2l0 QWRkaXRpb25hbENoaWxkcmVuKCkgaXMgY2FsbGVkIGZyb20gYSBHQyB0aHJlYWQgYW5kIHdhcyBj YWxsaW5nCisgICAgICAgIEhpc3Rvcnk6OmNhY2hlZFN0YXRlKCkgd2hvc2UgaW1wbGVtZW50YXRp b24gZ29lcyBkZWVwIGludG8gV2ViQ29yZQorICAgICAgICAoRnJhbWVMb2FkZXIgLyBIaXN0b3J5 Q29udHJvbGxlcikuIEFtb25nIG90aGVyIHRoaW5ncywgaXQgbnVsbCBjaGVja3MgbV93aW5kb3cK KyAgICAgICAgd2hpY2ggaXMgYSBXZWFrUHRyIGFuZCB0aGVuIGxhdGVyIGRlcmVmZXJlbmNlcyBp dCB0byBnZXQgdGhlIEZyYW1lIG9iamVjdCwgd2hpY2gKKyAgICAgICAgaXMgbm90IHNhZmUgZnJv bSB0aGUgbm9uLW1haW4gdGhyZWFkLgorCisgICAgICAgIFRvIGFkZHJlc3MgdGhlIGlzc3VlLCBp bnRyb2R1Y2UgYSBzaW1wbGVyIGNhY2hlZFN0YXRlRm9yR0MoKSBnZXR0ZXIgd2hpY2ggaXMKKyAg ICAgICAgdGhyZWFkIHNhZmUsIGFuZCB1c2UgaXQgaW4gSlNIaXN0b3J5Ojp2aXNpdEFkZGl0aW9u YWxDaGlsZHJlbigpLgorCisgICAgICAgICogYmluZGluZ3MvanMvSlNIaXN0b3J5Q3VzdG9tLmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OkpTSGlzdG9yeTo6dmlzaXRBZGRpdGlvbmFsQ2hpbGRyZW4p OgorICAgICAgICAqIHBhZ2UvSGlzdG9yeS5oOgorCiAyMDE5LTA4LTA3ICBaYWxhbiBCdWp0YXMg IDx6YWxhbkBhcHBsZS5jb20+CiAKICAgICAgICAgW0xGQ10gSW50cm9kdWNlIExheW91dDo6UGhh c2UgY2xhc3MKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTSGlzdG9y eUN1c3RvbS5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9qcy9KU0hpc3RvcnlDdXN0b20u Y3BwCmluZGV4IGNhNTE3MzY1MTNiYWUyZTA0ZjVjZjI1ZWQ4OGRiZTlhNjVlZWFhNmMuLjFiYTky OGU1ZDhjNGU5Y2U2Yzk2MjhlNjJiNDEzYmY5ODhjZGFhY2QgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL2JpbmRpbmdzL2pzL0pTSGlzdG9yeUN1c3RvbS5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvYmluZGluZ3MvanMvSlNIaXN0b3J5Q3VzdG9tLmNwcApAQCAtNDYsNyArNDYsNyBAQCBKU1Zh bHVlIEpTSGlzdG9yeTo6c3RhdGUoRXhlY1N0YXRlJiBzdGF0ZSkgY29uc3QKIAogdm9pZCBKU0hp c3Rvcnk6OnZpc2l0QWRkaXRpb25hbENoaWxkcmVuKFNsb3RWaXNpdG9yJiB2aXNpdG9yKQogewot ICAgIHdyYXBwZWQoKS5jYWNoZWRTdGF0ZSgpLnZpc2l0KHZpc2l0b3IpOworICAgIHdyYXBwZWQo KS5jYWNoZWRTdGF0ZUZvckdDKCkudmlzaXQodmlzaXRvcik7CiB9CiAKIH0gLy8gbmFtZXNwYWNl IFdlYkNvcmUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BhZ2UvSGlzdG9yeS5oIGIvU291 cmNlL1dlYkNvcmUvcGFnZS9IaXN0b3J5LmgKaW5kZXggYzU2NTRiYWUxZWFhZGU5YjUxZDlkZjgz Njk2ZWIzMDAxMTFhMWYzYS4uMDE1YzY3ZjYzOGZhZTdlYjcwNTliOTg4OTJmMWQ2MWNkM2FjYzBm MCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvcGFnZS9IaXN0b3J5LmgKKysrIGIvU291cmNl L1dlYkNvcmUvcGFnZS9IaXN0b3J5LmgKQEAgLTUzLDYgKzUzLDcgQEAgcHVibGljOgogCiAgICAg U2VyaWFsaXplZFNjcmlwdFZhbHVlKiBzdGF0ZSgpOwogICAgIEpTVmFsdWVJbldyYXBwZWRPYmpl Y3QmIGNhY2hlZFN0YXRlKCk7CisgICAgSlNWYWx1ZUluV3JhcHBlZE9iamVjdCYgY2FjaGVkU3Rh dGVGb3JHQygpIHsgcmV0dXJuIG1fY2FjaGVkU3RhdGU7IH0KIAogICAgIHZvaWQgYmFjaygpOwog ICAgIHZvaWQgZm9yd2FyZCgpOwo=