Bug 165497 – DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Summary:
While running LayoutTests/inspector/timeline/setInstruments-programmatic-capture.html there was an ASSERT seen on the bots:

Crash:
> Crashed Thread:        0  Dispatch queue: com.apple.main-thread
> Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
> Exception Codes:       KERN_INVALID_ADDRESS at 0x00000000bbadbeef
> Exception Note:        EXC_CORPSE_NOTIFY
> 
> Application Specific Information:
> CRASHING TEST: inspector/timeline/setInstruments-programmatic-capture.html
> This process is running with libgmalloc.dylib (GuardMalloc) which may have forced the crash due to a memory access error.
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.JavaScriptCore      	0x000000010244c7a7 WTFCrash + 39
> 1   com.apple.JavaScriptCore      	0x00000001015a5c34 JSC::ExecutableBase::isHostFunction() const + 100
> 2   com.apple.JavaScriptCore      	0x0000000101d63f8a JSC::SamplingProfiler::StackFrame::sourceID() + 74
> 3   com.apple.JavaScriptCore      	0x0000000101bdb0e1 Inspector::buildSamples(JSC::VM&, WTF::Vector<JSC::SamplingProfiler::StackTrace, 0ul, WTF::CrashOnOverflow, 16ul>&&) + 193
> 4   com.apple.JavaScriptCore      	0x0000000101bdab30 Inspector::InspectorScriptProfilerAgent::trackingComplete() + 288
> 5   com.apple.JavaScriptCore      	0x0000000101bdaa03 Inspector::InspectorScriptProfilerAgent::stopTracking(WTF::String&) + 83
> 6   com.apple.WebCore             	0x000000010d0a02fa WebCore::InspectorTimelineAgent::toggleScriptProfilerInstrument(WebCore::InspectorTimelineAgent::InstrumentState) + 106
> 7   com.apple.WebCore             	0x000000010d0a01d0 WebCore::InspectorTimelineAgent::toggleInstruments(WebCore::InspectorTimelineAgent::InstrumentState) + 144
> 8   com.apple.WebCore             	0x000000010d09f16b WebCore::InspectorTimelineAgent::stopProgrammaticCapture() + 155
> 9   com.apple.WebCore             	0x000000010d09ebb2 WebCore::InspectorTimelineAgent::stopFromConsole(JSC::ExecState*, WTF::String const&) + 434
> 10  com.apple.WebCore             	0x000000010d029640 WebCore::InspectorInstrumentation::stopProfilingImpl(WebCore::InstrumentingAgents&, JSC::ExecState*, WTF::String const&) + 64
> 11  com.apple.WebCore             	0x000000010def22ad WebCore::InspectorInstrumentation::stopProfiling(WebCore::Page&, JSC::ExecState*, WTF::String const&) + 45
> 12  com.apple.WebCore             	0x000000010def1ce9 WebCore::PageConsoleClient::profileEnd(JSC::ExecState*, WTF::String const&) + 41
> 13  com.apple.JavaScriptCore      	0x00000001013fa02e JSC::consoleProtoFuncProfileEnd(JSC::ExecState*) + 286
> ...

Notes:
- ScriptProfilerAgent::trackingComplete extracts a bunch of SamplingProfiler StackTraces (which hold ExecutableBase pointers) and then processes them
- It seems this ASSERT may be possible if we garbage collect one of those ExecutableBase pointers while processing those samples.

After talking this over with Saam we think deferring GC while processing the samples would be enough.

[Joseph Pecoraro]

<rdar://problem/29538973>

[Joseph Pecoraro]

Created attachment 296342 [details]
[PATCH] Proposed Fix

I was unable to reproduce locally to confirm that this solves the issue. However reasoning about this code it would seem appropriate anyways.

[Saam Barati]

Comment on attachment 296342 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/ChangeLog:12
> +        Defer collection when extracting and processing the samples to avoid
> +        any objects held by the samples from getting collected while processing.

Maybe it's worth adding why the could be collected:
we're calling into functions that can allocate. We must prevent those functions that can allocate from syncing w/ the GC thread.

[Saam Barati]

(In reply to comment #3)
> Comment on attachment 296342 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296342&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        Defer collection when extracting and processing the samples to avoid
> > +        any objects held by the samples from getting collected while processing.
> 
> Maybe it's worth adding why the could be collected:
> we're calling into functions that can allocate. We must prevent those
> functions that can allocate from syncing w/ the GC thread.

they*

[Joseph Pecoraro]

Created attachment 296345 [details]
[PATCH] For Landing

[Mark Lam]

Comment on attachment 296345 [details]
[PATCH] For Landing

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

> Source/JavaScriptCore/ChangeLog:15
> +        GC thread which may collecting other sample data yet to be processed.

/collecting/collect/.

[Joseph Pecoraro]

Created attachment 296348 [details]
[PATCH] For Landing

Heh thanks =)

[WebKit Commit Bot]

Comment on attachment 296348 [details]
[PATCH] For Landing

Clearing flags on attachment: 296348

Committed r209440: <http://trac.webkit.org/changeset/209440>