Bug 131747 - Crash in CodeBlock::setOptimizationThresholdBasedOnCompilationResult() when the debugger activates
Summary: Crash in CodeBlock::setOptimizationThresholdBasedOnCompilationResult() when t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-04-16 11:03 PDT by Mark Lam
Modified: 2014-04-16 16:11 PDT (History)
7 users (show)

See Also:


Attachments
the patch (2.60 KB, patch)
2014-04-16 15:20 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2: with bug for fixme. (2.64 KB, patch)
2014-04-16 15:50 PDT, Mark Lam
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2014-04-16 11:03:42 PDT
When the debugger is about to activate (e.g. enter stepping mode), it first waits for all DFG compilations to complete.  However, when the DFG completes, if compilation is successful, it will install a new DFG codeBlock.  The CodeBlock installation process is required to register codeBlocks with the debugger.  Debugger::registerCodeBlock() will eventually call CodeBlock::addBreakpoint() and/or CodeBlock::setSteppingMode() which may jettison the DFG codeBlock that we’re trying to install.  Thereafter, chaos ensues.
Comment 1 Mark Lam 2014-04-16 11:04:42 PDT
<rdar://problem/16278811>
Comment 2 Mark Lam 2014-04-16 14:57:45 PDT
Some notes about the fix I will post soon:

1. Debugger::registerCodeBlock() eventually calls:
    a. CodeBlock::addBreakpoint()
    b. CodeBlock::setSteppingMode().

    These 2 functions can jettison the code block, and this poses a problem if that happens during installation of said code block.

2. operationOptimize() will check if the debugger is stepping mode, or if the CodeBlock has any pending breakpoints before allowing a code block to be DFG compiled.

    If the base code block already has breakpoints enabled in it, we'll never optimize that code block, and hence, we'll never get to the scenario where we'll jettison the DFG code block for the reason of it having active breakpoints.

    If the debugger is already in stepping mode, we'll never optimize that code block, and hence, we'll never get to the scenario where we'll jettison the DFG code block for the reason of the debugger being in stepping mode.

    Which leaves ... 

3. What happens if a DFG compilation is already in progress in a compiler thread and the debugger switches to stepping mode.

    The debugger is supposed to wait for all compilations to complete before switching to stepping mode.  However, currently, the debugger is setting the stepping mode flag before compilation completes.  This is the root cause of this bug.

4. What happens if a DFG compilation is already in progress in a compiler thread and the debugger adds a new breakpoint to the function for that code block.

    The debugger is supposed to wait for all compilations to complete before it applies the new breakpoint to the code blocks.  And the debugger does behave correctly here (in Debugger::toggleBreakpoint()).
Comment 3 Mark Lam 2014-04-16 15:20:13 PDT
Created attachment 229486 [details]
the patch
Comment 4 Filip Pizlo 2014-04-16 15:43:35 PDT
Comment on attachment 229486 [details]
the patch

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

> Source/JavaScriptCore/debugger/Debugger.cpp:250
> +    // FIXME: We should never have to jettison a code block (due to pending breakpoints
> +    // or stepping mode) that is being registered. operationOptimize() should have
> +    // prevented the optimizing of such code blocks in the first place. Find a way to
> +    // express this with greater clarity in the code.

Can you file a bugzilla bug for this and reference it here?
Comment 5 Mark Lam 2014-04-16 15:50:31 PDT
Created attachment 229490 [details]
patch 2: with bug for fixme.
Comment 6 Mark Lam 2014-04-16 16:11:26 PDT
Landed in r167396: <http://trac.webkit.org/r167396>.