Fix assertion failure and race condition in Options::dumpSourceAtDFGTime().

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

CodeBlock::dumpSource() will access SourceCode strings in a way that requires ref'ing of the underlying StringImpls. This is unsafe to do from arbitrary compilation threads because StringImpls are not thread safe. There is an assertion in StringImpl code that checks for this.

However, Options::dumpSourceAtDFGTime() is only used for DFG debugging. For its purpose, we can live with the race condition introduced in the ref'ing of the SourceCode strings. Hence, we introduce an UnsafeHideCompilationScope here to temporarily and locally suppress the StringImpl assertion in order for Options::dumpSourceAtDFGTime() to also work on debug builds.

[Mark Lam]

Created attachment 251049 [details]
the patch.

[Geoffrey Garen]

Comment on attachment 251049 [details]
the patch.

I don't think we can live with the race condition. I, for one, would not want turning on source code dumping to add bugs to my code while I was debugging.

[Mark Lam]

Created attachment 251088 [details]
patch 2: a new fix with no race condition as requested.

[Mark Lam]

Created attachment 251089 [details]
patch 3: no race condition + build file fixes

[Geoffrey Garen]

Comment on attachment 251089 [details]
patch 3: no race condition + build file fixes

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

> Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp:39
> +    if (UNLIKELY(Options::dumpSourceAtDFGTime()))
> +        dumpCompiledSources();

It's strange that you guard this code with an options check at the call site and a null check in the callee. The two checks are redundant.

Renamed dumpCompiledSources to dumpCompiledSourcesIfNeeded, and remove the check at the call site.

> Source/JavaScriptCore/bytecode/DeferredCompilationCallback.cpp:53
> +Vector<SourceDumpInfo>& DeferredCompilationCallback::sourceDumpInfo()

This should be named "ensureSourceDumpInfo" since it is not a simple accessor.

> Source/JavaScriptCore/bytecode/SourceDumpInfo.h:36
> +class SourceDumpInfo {

Let's call this DeferredSourceDump. "Info" is a meaningless word.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:4043
> +        Vector<SourceDumpInfo>& dumpList = m_graph.m_plan.callback->sourceDumpInfo();

This local variable should have the same name as the accessor function that produces it. As I've said to you many times over the years, it is bad to give two names to one thing.

[Mark Lam]

Created attachment 251181 [details]
patch 4: applied Geoff's feedback.

[Mark Lam]

Thanks for the review.  Landed in r183161: <http://trac.webkit.org/r183161>.