Bug 165497 - DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots
Summary: DumpRenderTree ASSERT in JSC::ExecutableBase::isHostFunction seen on bots
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-06 15:55 PST by Joseph Pecoraro
Modified: 2016-12-07 13:35 PST (History)
7 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.17 KB, patch)
2016-12-06 16:00 PST, Joseph Pecoraro
saam: review+
Details | Formatted Diff | Diff
[PATCH] For Landing (3.39 KB, patch)
2016-12-06 16:25 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Landing (3.39 KB, patch)
2016-12-06 16:35 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-12-06 15:55:27 PST
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.
Comment 1 Joseph Pecoraro 2016-12-06 15:55:45 PST
<rdar://problem/29538973>
Comment 2 Joseph Pecoraro 2016-12-06 16:00:04 PST
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.
Comment 3 Saam Barati 2016-12-06 16:13:56 PST
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.
Comment 4 Saam Barati 2016-12-06 16:14:21 PST
(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*
Comment 5 Joseph Pecoraro 2016-12-06 16:25:29 PST
Created attachment 296345 [details]
[PATCH] For Landing
Comment 6 Mark Lam 2016-12-06 16:26:17 PST
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/.
Comment 7 Joseph Pecoraro 2016-12-06 16:35:53 PST
Created attachment 296348 [details]
[PATCH] For Landing

Heh thanks =)
Comment 8 WebKit Commit Bot 2016-12-06 18:53:01 PST
Comment on attachment 296348 [details]
[PATCH] For Landing

Clearing flags on attachment: 296348

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