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.
Created attachment 251049 [details] the patch.
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.
Created attachment 251088 [details] patch 2: a new fix with no race condition as requested.
Created attachment 251089 [details] patch 3: no race condition + build file fixes
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.
Created attachment 251181 [details] patch 4: applied Geoff's feedback.
Thanks for the review. Landed in r183161: <http://trac.webkit.org/r183161>.