Bug 219953

Summary: CachedRefPtr should adoptRef before calling ref to appease RefCounted's debug assertions
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: JavaScriptCoreAssignee: Saam Barati <saam>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=219909
Attachments:
Description Flags
patch
tzagallo: review+
patch for landing none

Description Ryan Haddad 2020-12-16 10:57:46 PST 
616 tests are asserting on the debug JSC bot

https://build.webkit.org/builders/Apple-Catalina-Debug-JSC-Tests/builds/2053/steps/jscore-test/logs/stdio

ASSERTION FAILED: !m_adoptionIsRequired
/Volumes/Data/slave/catalina-debug/build/WebKitBuild/Debug/usr/local/include/wtf/RefCounted.h(47) : void WTF::RefCountedBase::ref() const
1   0x10d832cc9 WTFCrash
2   0x10f00ef5b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x10f1dd28c WTF::RefCountedBase::ref() const
4   0x10e2fd391 WTF::DefaultRefDerefTraits<JSC::TDZEnvironmentLink>::refIfNotNull(JSC::TDZEnvironmentLink*)
5   0x10f0a4818 JSC::CachedRefPtr<JSC::CachedTDZEnvironmentLink, JSC::TDZEnvironmentLink, WTF::RawPtrTraits<JSC::TDZEnvironmentLink> >::decode(JSC::Decoder&) const
6   0x10f0a4775 JSC::CachedRefPtr<JSC::CachedTDZEnvironmentLink, JSC::TDZEnvironmentLink, WTF::RawPtrTraits<JSC::TDZEnvironmentLink> >::decode(JSC::Decoder&, WTF::RefPtr<JSC::TDZEnvironmentLink, WTF::RawPtrTraits<JSC::TDZEnvironmentLink>, WTF::DefaultRefDerefTraits<JSC::TDZEnvironmentLink> >&) const
7   0x10f0a45c1 JSC::CachedFunctionExecutableRareData::decode(JSC::Decoder&) const
8   0x10f0a435b JSC::UnlinkedFunctionExecutable::RareData* JSC::CachedPtr<JSC::CachedFunctionExecutableRareData, JSC::UnlinkedFunctionExecutable::RareData>::decode<>(JSC::Decoder&, bool&) const
9   0x10f0a4281 JSC::UnlinkedFunctionExecutable::RareData* JSC::CachedPtr<JSC::CachedFunctionExecutableRareData, JSC::UnlinkedFunctionExecutable::RareData>::decode<>(JSC::Decoder&) const
10  0x10f0a4016 JSC::CachedFunctionExecutable::rareData(JSC::Decoder&) const
11  0x10f0a3b0f JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(JSC::Decoder&, JSC::CachedFunctionExecutable const&)
12  0x10f0a3555 JSC::UnlinkedFunctionExecutable::UnlinkedFunctionExecutable(JSC::Decoder&, JSC::CachedFunctionExecutable const&)
13  0x10f0a3417 JSC::CachedFunctionExecutable::decode(JSC::Decoder&) const
14  0x10f0a323b JSC::UnlinkedFunctionExecutable* JSC::CachedPtr<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>::decode<>(JSC::Decoder&, bool&) const
15  0x10f0a3051 JSC::UnlinkedFunctionExecutable* JSC::CachedPtr<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>::decode<>(JSC::Decoder&) const
16  0x10f0a2ff5 JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>::decode(JSC::Decoder&, JSC::WriteBarrier<JSC::UnlinkedFunctionExecutable, WTF::RawPtrTraits<JSC::UnlinkedFunctionExecutable> >&, JSC::JSCell const*) const
17  0x10f0a2b50 std::__1::enable_if<!(std::is_same<JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>, JSC::SourceTypeImpl<JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>, void>::type>::value), void>::type JSC::decode<JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>, JSC::UnlinkedCodeBlock*>(JSC::Decoder&, JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable> const&, JSC::SourceTypeImpl<JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>, void>::type&, JSC::UnlinkedCodeBlock*)
18  0x10f09c3f3 void JSC::CachedVector<JSC::CachedWriteBarrier<JSC::CachedFunctionExecutable, JSC::UnlinkedFunctionExecutable>, 0ul, WTF::CrashOnOverflow, WTF::FastMalloc>::decode<JSC::UnlinkedCodeBlock*>(JSC::Decoder&, WTF::RefCountedArray<JSC::WriteBarrier<JSC::UnlinkedFunctionExecutable, WTF::RawPtrTraits<JSC::UnlinkedFunctionExecutable> >, WTF::RawPtrTraits<JSC::WriteBarrier<JSC::UnlinkedFunctionExecutable, WTF::RawPtrTraits<JSC::UnlinkedFunctionExecutable> > > >&, JSC::UnlinkedCodeBlock*) const
19  0x10f091c0d JSC::CachedCodeBlock<JSC::UnlinkedProgramCodeBlock>::decode(JSC::Decoder&, JSC::UnlinkedCodeBlock&) const
20  0x10f091913 JSC::CachedProgramCodeBlock::decode(JSC::Decoder&) const
21  0x10f0916fb JSC::UnlinkedProgramCodeBlock* JSC::CachedPtr<JSC::CachedProgramCodeBlock, JSC::UnlinkedProgramCodeBlock>::decode<>(JSC::Decoder&, bool&) const
22  0x10f08e2d1 JSC::UnlinkedProgramCodeBlock* JSC::CachedPtr<JSC::CachedProgramCodeBlock, JSC::UnlinkedProgramCodeBlock>::decode<>(JSC::Decoder&) const
23  0x10f060479 JSC::CacheEntry<JSC::UnlinkedProgramCodeBlock>::decode(JSC::Decoder&, std::__1::pair<JSC::SourceCodeKey, JSC::UnlinkedProgramCodeBlock*>&) const
24  0x10f06020e JSC::GenericCacheEntry::decode(JSC::Decoder&, std::__1::pair<JSC::SourceCodeKey, JSC::UnlinkedCodeBlock*>&) const
25  0x10f061023 JSC::decodeCodeBlockImpl(JSC::VM&, JSC::SourceCodeKey const&, WTF::Ref<JSC::CachedBytecode, WTF::RawPtrTraits<JSC::CachedBytecode> >)
26  0x10f0bc725 JSC::UnlinkedProgramCodeBlock* JSC::decodeCodeBlock<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&, WTF::Ref<JSC::CachedBytecode, WTF::RawPtrTraits<JSC::CachedBytecode> >)
27  0x10f0bc5ab JSC::UnlinkedProgramCodeBlock* JSC::CodeCacheMap::fetchFromDiskImpl<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&)
28  0x10f0bc1c5 std::__1::enable_if<(std::is_base_of<JSC::UnlinkedCodeBlock, JSC::UnlinkedProgramCodeBlock>::value) && (!(std::is_same<JSC::UnlinkedProgramCodeBlock, JSC::UnlinkedEvalCodeBlock>::value)), JSC::UnlinkedProgramCodeBlock*>::type JSC::CodeCacheMap::fetchFromDisk<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&)
29  0x10f0bbcd8 JSC::UnlinkedProgramCodeBlock* JSC::CodeCacheMap::findCacheAndUpdateAge<JSC::UnlinkedProgramCodeBlock>(JSC::VM&, JSC::SourceCodeKey const&)
30  0x10f064353 JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getUnlinkedGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, JSC::JSParserScriptMode, WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&, JSC::EvalContextType)
31  0x10f0641c9 JSC::CodeCache::getUnlinkedProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserStrictMode, WTF::OptionSet<JSC::CodeGenerationMode>, JSC::ParserError&)
Comment 1 Radar WebKit Bug Importer 2020-12-16 10:58:00 PST 
<rdar://problem/72391255>
Comment 2 Ryan Haddad 2020-12-16 15:27:28 PST 
Maybe https://trac.webkit.org/changeset/270870/webkit?
Comment 3 Saam Barati 2020-12-18 11:12:57 PST 
Created attachment 416531 [details]
patch
Comment 4 Tadeu Zagallo 2020-12-18 11:48:57 PST 
Comment on attachment 416531 [details]
patch

r=me
Comment 5 Darin Adler 2020-12-18 12:00:25 PST 
Comment on attachment 416531 [details]
patch

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

> Source/JavaScriptCore/runtime/CachedTypes.cpp:560
> +        RefPtr<Source, PtrTraits> result = adoptRef(decodedPtr);

Two related questions:

1) Is it OK that this calls adoptRef without template arguments rather than calling adoptRef<Source, PtrTraits>?
2) Would you consider using auto here instead of writing out the RefPtr type?
Comment 6 Saam Barati 2020-12-18 14:40:34 PST 
(In reply to Darin Adler from comment #5)
> Comment on attachment 416531 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=416531&action=review
> 
> > Source/JavaScriptCore/runtime/CachedTypes.cpp:560
> > +        RefPtr<Source, PtrTraits> result = adoptRef(decodedPtr);
> 
> Two related questions:
> 
> 1) Is it OK that this calls adoptRef without template arguments rather than
> calling adoptRef<Source, PtrTraits>?
Probably not. Good call, I will fix. This is probably fine for the particular types we currently use it for, but it'd probably not work for future types that make use of non default PtrTraits.

> 2) Would you consider using auto here instead of writing out the RefPtr type?
Auto seems ok
Comment 7 Saam Barati 2020-12-18 14:43:59 PST 
Created attachment 416547 [details]
patch for landing
Comment 8 EWS 2020-12-18 15:15:19 PST 
Committed r270991: <https://trac.webkit.org/changeset/270991>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416547 [details].