Bug 121969

Summary: Change ScriptDebugServer to use DebuggerCallFrame instead of JavaScriptCallFrame
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, eflews.bot, fpizlo, ggaren, gyuyoung.kim, joepeck, mhahnenberg, msaboff, oliver, rniwa, sergio, timothy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 121796    
Attachments:
Description Flags
the patch.
ggaren: review-
patch 2: Create a new DebuggerCallFrame for every pause callback to the ScriptDebugServer's client.
mark.lam: review-
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.
mark.lam: review-
patch 4: applied feedback from Geoff and Oliver. Still need tests.
eflews.bot: commit-queue-
patch 5: fixed typo from patch 4.
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
patch 6: with new tests (plus fixing up an old test) ggaren: review+

Description Mark Lam 2013-09-26 12:07:19 PDT
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.
Comment 1 Mark Lam 2013-09-26 12:09:43 PDT
Created attachment 212734 [details]
the patch.
Comment 2 Geoffrey Garen 2013-09-26 12:39:51 PDT
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?
Comment 3 Geoffrey Garen 2013-09-26 12:45:30 PDT
> 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.
Comment 4 Geoffrey Garen 2013-09-26 12:47:39 PDT
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?
Comment 5 Timothy Hatcher 2013-09-26 20:21:37 PDT
(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.
Comment 6 Mark Lam 2013-09-27 19:13:57 PDT
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.
Comment 7 Mark Lam 2013-09-27 19:28:29 PDT
Created attachment 212870 [details]
patch 2: Create a new DebuggerCallFrame for every pause callback to the ScriptDebugServer's client.
Comment 8 Mark Lam 2013-09-27 19:33:59 PDT
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.
Comment 9 Geoffrey Garen 2013-09-27 20:13:02 PDT
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.
Comment 10 Mark Lam 2013-09-27 22:27:10 PDT
(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.
Comment 11 Mark Lam 2013-09-28 02:16:23 PDT
Created attachment 212888 [details]
patch 3: fixed DebuggerCallFrame::invalidate(), and applied the UnsignedWithZeroKeyHashTraits to the LineToBreakpointsMap.
Comment 12 Mark Lam 2013-09-28 10:50:05 PDT
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.
Comment 13 Geoffrey Garen 2013-09-28 12:38:50 PDT
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;
Comment 14 Oliver Hunt 2013-09-29 12:13:05 PDT
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?
Comment 15 Mark Lam 2013-09-30 17:08:23 PDT
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.
Comment 16 Mark Lam 2013-10-03 15:39:15 PDT
Created attachment 213306 [details]
patch 4: applied feedback from Geoff and Oliver. Still need tests.
Comment 17 EFL EWS Bot 2013-10-03 15:42:24 PDT
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
Comment 18 Mark Lam 2013-10-03 15:59:24 PDT
Created attachment 213309 [details]
patch 5: fixed typo from patch 4.
Comment 19 Ryosuke Niwa 2013-10-03 16:42:31 PDT
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 ]
Comment 20 Build Bot 2013-10-03 17:09:15 PDT
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
Comment 21 Build Bot 2013-10-03 17:09:18 PDT
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
Comment 22 Build Bot 2013-10-03 18:02:47 PDT
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
Comment 23 Build Bot 2013-10-03 18:02:52 PDT
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
Comment 24 Build Bot 2013-10-03 18:26:56 PDT
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
Comment 25 Build Bot 2013-10-03 18:26:59 PDT
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
Comment 26 Build Bot 2013-10-03 19:05:33 PDT
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
Comment 27 Build Bot 2013-10-03 19:05:37 PDT
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
Comment 28 Geoffrey Garen 2013-10-04 12:53:53 PDT
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.
Comment 29 Mark Lam 2013-10-04 17:15:05 PDT
Created attachment 213422 [details]
patch 6: with new tests (plus fixing up an old test)
Comment 30 Geoffrey Garen 2013-10-04 17:41:58 PDT
Comment on attachment 213422 [details]
patch 6: with new tests (plus fixing up an old test)

r=me
Comment 31 Mark Lam 2013-10-04 17:51:01 PDT
Thanks for the review.  Landed in r156936: <http://trac.webkit.org/r156936>.
Comment 32 Sergio Correia (qrwteyrutiyoup) 2013-10-05 14:28:14 PDT
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?
Comment 33 Mark Lam 2013-10-07 09:01:10 PDT
(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.
Comment 34 Sergio Correia (qrwteyrutiyoup) 2013-10-07 09:06:19 PDT
(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 :)