Bug 206191 – Web Inspector: crash in DumpRenderTree at com.apple.JavaScriptCore: WTF::RefCountedBase::hasOneRef const

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

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__

[Devin Rousso]

<rdar://problem/58415623>

[Devin Rousso]

Created attachment 387562 [details]
Patch

[Joseph Pecoraro]

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?

[Devin Rousso]

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

[WebKit Commit Bot]

Comment on attachment 387562 [details]
Patch

Clearing flags on attachment: 387562

Committed r254523: <https://trac.webkit.org/changeset/254523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.