Bug 206191

Summary: Web Inspector: crash in DumpRenderTree at com.apple.JavaScriptCore: WTF::RefCountedBase::hasOneRef const
Product: WebKit Reporter: Devin Rousso <hi>
Component: Web InspectorAssignee: Devin Rousso <hi>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ddkilzer, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch none

Description Devin Rousso 2020-01-13 13:08:44 PST 
ASSERTION FAILED: !m_deletionHasBegun
/Volumes/Data/worker/macOS-High-Sierra-Debug-Build-EWS/build/WebKitBuild/Debug/usr/local/include/wtf/RefCounted.h(55) : bool WTF::RefCountedBase::hasOneRef() const
1   0x10a8e0639 WTFCrash
2   0x10bf6a73b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x10be2d688 WTF::RefCountedBase::hasOneRef() const
4   0x10b019099 WTF::RefCountedBase::applyRefDerefThreadingCheck() const
5   0x10bfa06d9 WTF::RefCountedBase::ref() const
6   0x10b2521f5 void WTF::refIfNotNull<JSC::SourceProvider>(JSC::SourceProvider*)
7   0x10b2521b4 WTF::RefPtr<JSC::SourceProvider, WTF::DumbPtrTraits<JSC::SourceProvider> >::RefPtr(JSC::SourceProvider*)
8   0x10b25217d WTF::RefPtr<JSC::SourceProvider, WTF::DumbPtrTraits<JSC::SourceProvider> >::RefPtr(JSC::SourceProvider*)
9   0x10bb17713 WTF::RefPtr<JSC::SourceProvider, WTF::DumbPtrTraits<JSC::SourceProvider> >::operator=(JSC::SourceProvider*)
10  0x10bb1735e Inspector::ScriptDebugServer::sourceParsed(JSC::JSGlobalObject*, JSC::SourceProvider*, int, WTF::String const&)
11  0x10b4b3de8 JSC::Debugger::attach(JSC::JSGlobalObject*)
12  0x1197766c0 WebCore::JSWindowProxy::attachDebugger(JSC::Debugger*)
13  0x11979d519 WebCore::WindowProxy::attachDebugger(JSC::Debugger*)
14  0x11a95199a WebCore::Page::setDebugger(JSC::Debugger*)
15  0x11a4c2cb7 WebCore::PageScriptDebugServer::attachDebugger()
16  0x10bb17f40 Inspector::ScriptDebugServer::addListener(Inspector::ScriptDebugListener*)
17  0x10bb2ca32 Inspector::InspectorDebuggerAgent::enable()
18  0x11a5b7f53 WebCore::WebDebuggerAgent::enable()
19  0x11a5ea6a3 WebCore::PageDebuggerAgent::enable()
20  0x10bb2d4d9 Inspector::InspectorDebuggerAgent::enable(WTF::String&)
21  0x10ba8dc9a Inspector::DebuggerBackendDispatcher::enable(long, WTF::RefPtr<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&)
22  0x10ba8db31 Inspector::DebuggerBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&)
23  0x10ba72b29 Inspector::BackendDispatcher::dispatch(WTF::String const&)
24  0x11a48edfc WebCore::InspectorController::dispatchMessageFromFrontend(WTF::String const&)
25  0x11a49c48d WebCore::InspectorBackendDispatchTask::dispatchOneMessage()
26  0x11a49c3ac WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'()::operator()() const
27  0x11a49c159 WTF::Detail::CallableWrapper<WebCore::InspectorBackendDispatchTask::scheduleOneShot()::'lambda'(), void>::call()
28  0x10a906c9d WTF::Function<void ()>::operator()() const
29  0x10a9703ed WTF::RunLoop::performWork()
30  0x10a970d64 WTF::RunLoop::performWork(void*)
31  0x7fff4bcf9581 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
Comment 1 Devin Rousso 2020-01-13 13:08:54 PST 
<rdar://problem/58415623>
Comment 2 Devin Rousso 2020-01-13 13:14:09 PST 
Created attachment 387562 [details]
Patch
Comment 3 Joseph Pecoraro 2020-01-13 13:25:46 PST 
Comment on attachment 387562 [details]
Patch

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

r=me

> Source/JavaScriptCore/debugger/Debugger.cpp:127
> +                    if (function->scope()->globalObject() == globalObject && function->executable()->isFunctionExecutable() && !function->isHostOrBuiltinFunction())

I would find this easier to read and modify in the future if this used early return conditions like it was before. For example:

    if (function->scope()->globalObject() != m_globalObject)
        return IterationStatus::Continue;
    
    if (!function->executable()->isFunctionExecutable())
        return IterationStatus::Continue;

    if (function->isHostOrBuiltinFunction())
        return IterationStatus::Continue;

But I'll leave that style choice up to you.

> Source/JavaScriptCore/debugger/Debugger.cpp:135
> +        sourceParsed(globalObject, sourceProvider.get(), -1, nullString());

Is there a meaningful difference between switching to `nullString` here?
Comment 4 Devin Rousso 2020-01-13 13:45:40 PST 
Comment on attachment 387562 [details]
Patch

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

>> Source/JavaScriptCore/debugger/Debugger.cpp:127
>> +                    if (function->scope()->globalObject() == globalObject && function->executable()->isFunctionExecutable() && !function->isHostOrBuiltinFunction())
> 
> I would find this easier to read and modify in the future if this used early return conditions like it was before. For example:
> 
>     if (function->scope()->globalObject() != m_globalObject)
>         return IterationStatus::Continue;
>     
>     if (!function->executable()->isFunctionExecutable())
>         return IterationStatus::Continue;
> 
>     if (function->isHostOrBuiltinFunction())
>         return IterationStatus::Continue;
> 
> But I'll leave that style choice up to you.

I personally find the single `return` to be easier to read, especially in this case as it makes it super clear that we are _always_ expecting to continue iterating (rather than having to read each `return` to see that).

>> Source/JavaScriptCore/debugger/Debugger.cpp:135
>> +        sourceParsed(globalObject, sourceProvider.get(), -1, nullString());
> 
> Is there a meaningful difference between switching to `nullString` here?

No, as the `-1` effectively causes it to be ignored.  I was just trying to avoid another allocation with `String()`.
Comment 5 WebKit Commit Bot 2020-01-14 12:00:50 PST 
Comment on attachment 387562 [details]
Patch

Clearing flags on attachment: 387562

Committed r254523: <https://trac.webkit.org/changeset/254523>
Comment 6 WebKit Commit Bot 2020-01-14 12:00:51 PST 
All reviewed patches have been landed.  Closing bug.