Summary: | Fix assertion failure and race condition in Options::dumpSourceAtDFGTime(). | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | basile_clement, benjamin, fpizlo, ggaren, mmirman, msaboff, saam | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Mark Lam
2015-04-17 14:30:29 PDT
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>. |