Bug 121969 – Change ScriptDebugServer to use DebuggerCallFrame instead of JavaScriptCallFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Change ScriptDebugServer to use DebuggerCallFrame instead of JavaScriptCallFrame as the data structure for tracking the current frame of execution.  This entails:

1. Make JavaScriptCallFrame a thin shell around the DebuggerCallFrame.  DebuggerCallFrame now tracks whether it is valid instead of needing JavaScriptCallFrame do it.
2. ScriptDebugServer now invalidates DebuggerCallFrames that have been "popped".
3. ScriptDebugServer now only creates a JavaScriptCallFrame when it actually needs it i.e. when the debugger needs to pause.
4. JavaScriptCallFrame only creates its caller JavaScriptCallFrame when it is needed i.e. when the client calls caller().
5. DebuggerCallFrame's line() and column() now returns a base-zero int.
6. WebScriptDebugDelegate now only caches the functionName of the frame instead of the entire DebuggerCallFrame because that is all that is needed.

[Mark Lam]

Created attachment 212734 [details]
the patch.

[Geoffrey Garen]

Another important detail: If you pause twice in the same function, we'll create two JavaScriptCallFrames instead of one. 

That is the most important detail, since it has the possibility of causing compatibility problems with front-end code, if that code depends on == for JavaScriptCallFrame objects.

How did you test whether this patch caused compatibility issues for the inspector?

[Geoffrey Garen]

> Another important detail: If you pause twice in the same function, we'll create two JavaScriptCallFrames instead of one. 

Actually, it looks like you didn't make this change, so this patch is wrong.

[Geoffrey Garen]

Comment on attachment 212734 [details]
the patch.

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

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:535
>  
>      // Treat stepping over a return statement like stepping out.
>      if (m_currentCallFrame == m_pauseOnCallFrame)
> -        m_pauseOnCallFrame = m_currentCallFrame->caller();
> -    m_currentCallFrame = m_currentCallFrame->caller();
> +        m_pauseOnCallFrame = m_currentCallFrame->caller().get();
> +
> +    setCurrentCallFrame(m_currentCallFrame->caller());
>  }

This is wrong. You've designed the system such that it's a strict requirement that we get a callback from every JavaScript function before it returns.

How are you going to arrange to get those callbacks without harming performance?

[Timothy Hatcher]

(In reply to comment #2)
> Another important detail: If you pause twice in the same function, we'll create two JavaScriptCallFrames instead of one. 
> 
> That is the most important detail, since it has the possibility of causing compatibility problems with front-end code, if that code depends on == for JavaScriptCallFrame objects.
> 
> How did you test whether this patch caused compatibility issues for the inspector?

I don't think this will affect us. We rebuild the stack UI each time we step.

[Mark Lam]

Status update (based on Step 1 specified at https://bugs.webkit.org/show_bug.cgi?id=121796#c13):
=============================================================
(1a) Add JSLocking to DebuggerCallFrame.  Done.

(1b) Rewrite member functions [of ScriptDebugServer] to use DebuggerCallFrame directly.

- Instead of doing this across the board, I'll only create the DebuggerCallFrame when I need to call back to the ScriptDebugServer's client to pause (or execute relevant actions).  For everything else in ScriptDebugServer, I will continue using JSC::CallFrame directly.  This should be ok because the sections of code that uses CallFrame directly will be the parts moved into JSC::Debugger later.  If we discover a violation of this rule when we do that move, I'll fix it then.

Based on this more defined plan, this task is done.

(1c) Rewrite ScriptDebugServer::dispatchDidPause to create a new JavaScriptCallFrame for each callback. Test to verify that the front-end doesn't depend on object identity here.

Done.  JavaScriptCore tests and layout tests have been run and shows no regression.  I've also tested on the WebInspector by stepping through some JS code.  I verified that the Inspector's shown JS stack trace line numbers do update with the steps.  This confirms that Inspector is using the new JavaScriptCallFrame on every pause.

I also have assertions in DebuggerCallFrame's methods to ensure that an invalidated DebuggerCallFrame is not used.  Previously, I mentioned to Geoff in person that I saw these assertions fail.  That was due to a bug in my management of the DebuggerCallFrame life cycle at that time.  That bug is now fixed, and I no longer see any of these assertions fail.

With the WebInspector, I also set some breakpoints, and did some step in, step over, step outs to so a smoke check of general debugger functionality.  Everything works as expected.

Patch coming soon.

[Mark Lam]

Created attachment 212870 [details]
patch 2: Create a new DebuggerCallFrame for every pause callback to the ScriptDebugServer's client.

[Mark Lam]

Comment on attachment 212870 [details]
patch 2: Create a new DebuggerCallFrame for every pause callback to the ScriptDebugServer's client.

Need to fix DebuggerCallFrame::invalidate() to recursively invalidate all its caller frames as well.

[Geoffrey Garen]

Comment on attachment 212870 [details]
patch 2: Create a new DebuggerCallFrame for every pause callback to the ScriptDebugServer's client.

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

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:110
> +    // We add 1 to the lineNumber before using it as a key to the LineToBreakpointMap
> +    // because lineNumber is 0-based (hence, can be 0) and LineToBreakpointMap (which
> +    // is a HashMap) does not allow 0 as a key value.
>      LineToBreakpointMap::iterator breaksIt = it->value.find(scriptBreakpoint.lineNumber + 1);

Please use UnsignedWithZeroKeyHashTraits. Then you can avoid the + 1 adjustments, and allow 0 in the table.

[Mark Lam]

(In reply to comment #9)
> Please use UnsignedWithZeroKeyHashTraits. Then you can avoid the + 1 adjustments, and allow 0 in the table.

Thanks for the tip.  Will fix.

[Mark Lam]

Created attachment 212888 [details]
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.

[Mark Lam]

Comment on attachment 212888 [details]
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.

Need to fix an uninitialized field in ScriptDebugServer.

[Geoffrey Garen]

Comment on attachment 212888 [details]
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.

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

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:2
> + * Copyright (C) 2008-2013 Apple Inc. All rights reserved.

Nit: "-" should be ",", since you were not working on this file five years contiguously.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:60
> +    lineAndColumnForCallFrame(m_callFrame, m_line, m_column);

This should be "getLineAndColumnForFrame", since it returns out parameters.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:108
>      if (!m_callFrame->codeBlock())
>          return String();

No need for this CodeBlock check, and it's wrong for host functions. Host functions have names, but do not have CodeBlocks.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:221
> +    CodeBlock* codeBlock = callFrame->codeBlock();
> +    if (!codeBlock)
> +        return 0;
> +
> +    JSValue thisValue = callFrame->uncheckedR(codeBlock->thisRegister()).jsValue();
> +    if (!thisValue.isObject())
> +        return 0;

Host functions have 'this' parameters, and they can be retrieved without use of CodeBlock. Just use CallFrame::thisValue(). Please add a test for this.

Also: the isObject() check and asObject() conversion are wrong in strict mode. In strict mode, primitive 'this' values are allowed. You should also remove 'Object' from the function's name and type. Please add a test for this.

Also: Any access to 'this' is required to do a ToThis conversion. If the target function did not use 'this', it has not done the conversion yet, so we need to do it now.  Please add a test for this.

The code here should be something like:

ECMAMode ecmaMode = NotStrictMode;
if (callFrame->codeBlock() && callFrame->codeBlock()->isStrictMode())
    ecmaMode = StrictMode;
JSValue thisValue = callFrame->thisValue().toThis(callFrame, ecmaMode);
return thisValue;

[Oliver Hunt]

Comment on attachment 212888 [details]
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.

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

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:207
> +    if (!callFrame || !callFrame->codeBlock())
> +        return 0;
> +    return callFrame->codeBlock()->ownerExecutable()->sourceID();

Can callFrame be null here? If it isn't can we make it ASSERT(callFrame); then if (!callFrame); then CodeBlock* codeBlock = callFrame->codeBlock(); if (!codeBlock) return 0; return codeBlock->...

> Source/JavaScriptCore/debugger/DebuggerCallFrame.h:2
> + * Copyright (C) 2008-2013 Apple Inc. All rights reserved.

2008, 2013

> Source/WebCore/bindings/js/ScriptDebugServer.cpp:157
> -bool ScriptDebugServer::hasBreakpoint(intptr_t sourceID, const TextPosition& position, ScriptBreakpoint *hitBreakpoint) const
> +bool ScriptDebugServer::hasBreakpoint(intptr_t sourceID, int line, int column, ScriptBreakpoint *hitBreakpoint) const

Why have we switched line/column instead of text position here?

[Mark Lam]

Status update: I've made all the changes from Geoff and Oliver's feedback.  Current tests are passing.

However, I'm still trying to figure out how the plumbing that our various versions of inspector tests works.  Will write new tests as soon as I have a grip on that.

[Mark Lam]

Created attachment 213306 [details]
patch 4: applied feedback from Geoff and Oliver. Still need tests.

[EFL EWS Bot]

Comment on attachment 213306 [details]
patch 4: applied feedback from Geoff and Oliver. Still need tests.

Attachment 213306 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/2948035

[Mark Lam]

Created attachment 213309 [details]
patch 5: fixed typo from patch 4.

[Ryosuke Niwa]

It seems like an inspector test got flaky/failing on EWS:

Unexpected flakiness: text-only failures (1)
  inspector/debugger/watch-expressions-panel-switch.html [ Failure Pass ]


Regressions: Unexpected text-only failures (1)
  inspector/debugger/pause-in-internal-script.html [ Failure ]

[Build Bot]

Comment on attachment 213309 [details]
patch 5: fixed typo from patch 4.

Attachment 213309 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3190006

New failing tests:
inspector/debugger/pause-in-internal-script.html

[Build Bot]

Created attachment 213312 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213309 [details]
patch 5: fixed typo from patch 4.

Attachment 213309 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3189016

New failing tests:
inspector/debugger/pause-in-internal-script.html

[Build Bot]

Created attachment 213320 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213309 [details]
patch 5: fixed typo from patch 4.

Attachment 213309 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3190023

New failing tests:
inspector/debugger/pause-in-internal-script.html

[Build Bot]

Created attachment 213321 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 213309 [details]
patch 5: fixed typo from patch 4.

Attachment 213309 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3127139

New failing tests:
inspector/debugger/pause-in-internal-script.html

[Build Bot]

Created attachment 213323 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Geoffrey Garen]

The tests we need are:

(1) A host function should have a name in the backtrace.

(2) A host function should have a this object (when accessed from within a backtrace).

(3) A StrictMode JS function, if passed a primitive this value, should reflect a primitive this value (when accessed from within a backtrace).

(4) A NotStrictMode JS function, which does not use 'this' in its body, if passed a primitive this value, should reflect an object this value (when accessed from within a backtrace).

(5) A JS function, which does not use 'this' in its body, if passed an activation this value, should reflect a window object this value (when accessed from within a backtrace).

Our goal here is just to exercise this code in the simplest way possible, so someone making the same mistake in the future will see a failing test.

[Mark Lam]

Created attachment 213422 [details]
patch 6: with new tests (plus fixing up an old test)

[Geoffrey Garen]

Comment on attachment 213422 [details]
patch 6: with new tests (plus fixing up an old test)

r=me

[Mark Lam]

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

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 213422 [details]
patch 6: with new tests (plus fixing up an old test)

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

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:186
> +    ASSERT(!callFrame->codeBlock() || functor.column() >= 0);

The return of both functor.line() and functor.column() are unsigned, and hence, always >= 0. Should we just ASSERT(!callFrame->codeBlock()) instead, in here?

[Mark Lam]

(In reply to comment #32)
> (From update of attachment 213422 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213422&action=review
> 
> > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:186
> > +    ASSERT(!callFrame->codeBlock() || functor.column() >= 0);
> 
> The return of both functor.line() and functor.column() are unsigned, and hence, always >= 0. Should we just ASSERT(!callFrame->codeBlock()) instead, in here?

The asserts are stale code that should have been removed.  https://bugs.webkit.org/show_bug.cgi?id=122440 is addressing this issue.

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #33)
> (In reply to comment #32)
> > (From update of attachment 213422 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213422&action=review
> > 
> > > Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:186
> > > +    ASSERT(!callFrame->codeBlock() || functor.column() >= 0);
> > 
> > The return of both functor.line() and functor.column() are unsigned, and hence, always >= 0. Should we just ASSERT(!callFrame->codeBlock()) instead, in here?
> 
> The asserts are stale code that should have been removed.  https://bugs.webkit.org/show_bug.cgi?id=122440 is addressing this issue.

Ah, nice :)