Bug 9885 - Breakpoints on blank lines or comments don't break
Summary: Breakpoints on blank lines or comments don't break
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on: 155325
Blocks:
  Show dependency treegraph
 
Reported: 2006-07-12 21:17 PDT by Timothy Hatcher
Modified: 2016-09-30 16:37 PDT (History)
14 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (256.14 KB, patch)
2016-09-26 21:45 PDT, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (558.08 KB, patch)
2016-09-26 21:49 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-yosemite (1.66 MB, application/zip)
2016-09-26 23:05 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (269.63 KB, patch)
2016-09-27 11:45 PDT, Joseph Pecoraro
ggaren: review-
Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (558.07 KB, patch)
2016-09-27 11:46 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (256.73 KB, patch)
2016-09-27 13:40 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (558.15 KB, patch)
2016-09-27 13:41 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For EWS - Trying to make EWS Process Something (558.15 KB, patch)
2016-09-27 15:02 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - Rebaselined (557.76 KB, patch)
2016-09-28 14:21 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch (556.74 KB, patch)
2016-09-28 14:26 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (256.41 KB, patch)
2016-09-28 14:53 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (557.81 KB, patch)
2016-09-28 14:54 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Bots - Windows Yet Again?! (582.02 KB, patch)
2016-09-28 15:18 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] For Bots - Windows just guessing at this point... (582.09 KB, patch)
2016-09-28 15:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (958.47 KB, application/zip)
2016-09-28 16:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews101 for mac-yosemite (1.02 MB, application/zip)
2016-09-28 16:29 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.57 MB, application/zip)
2016-09-28 16:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2 (6.79 MB, application/zip)
2016-09-28 16:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (950.71 KB, application/zip)
2016-09-28 16:42 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (256.31 KB, patch)
2016-09-28 16:47 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (557.72 KB, patch)
2016-09-28 16:48 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-yosemite-wk2 (1011.80 KB, application/zip)
2016-09-28 18:20 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (257.59 KB, patch)
2016-09-29 11:10 PDT, Joseph Pecoraro
mark.lam: review+
Details | Formatted Diff | Diff
[PATCH] For Bots - All Necessary Parts (558.73 KB, patch)
2016-09-29 11:11 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Timothy Hatcher 2006-07-12 21:17:01 PDT
Breakpoints on blank lines and comments should break on the next statment in that function.
Comment 1 Adam Roben (:aroben) 2008-08-07 16:20:23 PDT
<rdar://problem/6134406>
Comment 2 Brian Burg 2014-11-28 19:44:47 PST
Should these breakpoints be moved by the frontend to the next eligible line? That seems like the easiest fix, rather than the long-standing mysterious behavior we have now.
Comment 3 Joseph Pecoraro 2016-09-26 21:45:20 PDT
Created attachment 289912 [details]
[PATCH] Proposed Fix

This depends on the not-yet-landed debugger stepping modifications. So I'll put a patch up that will please the bots.
Comment 4 Joseph Pecoraro 2016-09-26 21:49:27 PDT
Created attachment 289914 [details]
[PATCH] For Bots - All Necessary Parts
Comment 5 WebKit Commit Bot 2016-09-26 21:53:19 PDT
Attachment 289914 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/parser/Parser.h:852:  The parameter name "debuggerParseData" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 3 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Build Bot 2016-09-26 23:05:53 PDT
Comment on attachment 289914 [details]
[PATCH] For Bots - All Necessary Parts

Attachment 289914 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2152551

New failing tests:
inspector/debugger/breakpoints/resolved-dump-each-line.html
Comment 7 Build Bot 2016-09-26 23:05:57 PDT
Created attachment 289919 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Joseph Pecoraro 2016-09-27 11:38:09 PDT
(In reply to comment #6)
> Comment on attachment 289914 [details]
> [PATCH] For Bots - All Necessary Parts
> 
> Attachment 289914 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/2152551
> 
> New failing tests:
> inspector/debugger/breakpoints/resolved-dump-each-line.html

I should mark the tests as [ Slow ] on Debug, since it appears to have half completed the test.

I will have to investigate the Win build error, it looks like something is missing an include, albeit the error message is completely unclear.
Comment 9 Joseph Pecoraro 2016-09-27 11:45:31 PDT
Created attachment 289987 [details]
[PATCH] Proposed Fix
Comment 10 Joseph Pecoraro 2016-09-27 11:46:13 PDT
Created attachment 289988 [details]
[PATCH] For Bots - All Necessary Parts
Comment 11 WebKit Commit Bot 2016-09-27 11:49:22 PDT
Attachment 289988 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Joseph Pecoraro 2016-09-27 12:14:02 PDT
Comment on attachment 289988 [details]
[PATCH] For Bots - All Necessary Parts

Trying to fix Windows I broke everyone...
Comment 13 Geoffrey Garen 2016-09-27 13:04:38 PDT
Comment on attachment 289987 [details]
[PATCH] Proposed Fix

1 test crash, one timeout on EWS.
Comment 14 Joseph Pecoraro 2016-09-27 13:10:50 PDT
(In reply to comment #13)
> Comment on attachment 289987 [details]
> [PATCH] Proposed Fix
> 
> 1 test crash, one timeout on EWS.

In the attachment, no test crashed. They just both timed out after completing half the test. I marked them as slow on Debug builds.
Comment 15 Joseph Pecoraro 2016-09-27 13:40:10 PDT
Created attachment 290000 [details]
[PATCH] Proposed Fix
Comment 16 Joseph Pecoraro 2016-09-27 13:41:02 PDT
Created attachment 290002 [details]
[PATCH] For Bots - All Necessary Parts
Comment 17 WebKit Commit Bot 2016-09-27 13:44:22 PDT
Attachment 290002 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 18 Joseph Pecoraro 2016-09-27 15:02:58 PDT
Created attachment 290011 [details]
[PATCH] For EWS - Trying to make EWS Process Something
Comment 19 WebKit Commit Bot 2016-09-27 15:06:11 PDT
Attachment 290011 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:38:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 Joseph Pecoraro 2016-09-27 16:16:16 PDT
Comment on attachment 290011 [details]
[PATCH] For EWS - Trying to make EWS Process Something

Still has windows build errors that are mysterious.
Comment 21 Joseph Pecoraro 2016-09-28 14:21:21 PDT
Created attachment 290120 [details]
[PATCH] For Bots - Rebaselined
Comment 22 Alex Christensen 2016-09-28 14:26:52 PDT
Created attachment 290121 [details]
Patch
Comment 23 WebKit Commit Bot 2016-09-28 14:30:12 PDT
Attachment 290121 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Joseph Pecoraro 2016-09-28 14:53:21 PDT
Created attachment 290125 [details]
[PATCH] Proposed Fix

Now with a Windows fix.
Comment 25 Joseph Pecoraro 2016-09-28 14:54:08 PDT
Created attachment 290126 [details]
[PATCH] For Bots - All Necessary Parts
Comment 26 WebKit Commit Bot 2016-09-28 14:56:10 PDT
Attachment 290126 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 Joseph Pecoraro 2016-09-28 15:18:47 PDT
Created attachment 290128 [details]
[PATCH] For Bots - Windows Yet Again?!
Comment 28 WebKit Commit Bot 2016-09-28 15:24:48 PDT
Attachment 290128 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Joseph Pecoraro 2016-09-28 15:50:37 PDT
Created attachment 290132 [details]
[PATCH] For Bots - Windows just guessing at this point...
Comment 30 WebKit Commit Bot 2016-09-28 15:54:01 PDT
Attachment 290132 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 82 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Build Bot 2016-09-28 16:26:43 PDT
Comment on attachment 290126 [details]
[PATCH] For Bots - All Necessary Parts

Attachment 290126 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2164172

New failing tests:
fast/images/pdf-as-image-with-annotations.html
Comment 32 Build Bot 2016-09-28 16:26:47 PDT
Created attachment 290134 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 33 Build Bot 2016-09-28 16:29:35 PDT
Comment on attachment 290128 [details]
[PATCH] For Bots - Windows Yet Again?!

Attachment 290128 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2164201

New failing tests:
js/dom/stack-trace.html
Comment 34 Build Bot 2016-09-28 16:29:39 PDT
Created attachment 290135 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 35 Build Bot 2016-09-28 16:33:24 PDT
Comment on attachment 290128 [details]
[PATCH] For Bots - Windows Yet Again?!

Attachment 290128 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2164178

New failing tests:
js/dom/stack-trace.html
Comment 36 Build Bot 2016-09-28 16:33:28 PDT
Created attachment 290136 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 37 Build Bot 2016-09-28 16:36:58 PDT
Comment on attachment 290128 [details]
[PATCH] For Bots - Windows Yet Again?!

Attachment 290128 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2164182

New failing tests:
js/dom/stack-trace.html
Comment 38 Build Bot 2016-09-28 16:37:03 PDT
Created attachment 290138 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 39 Build Bot 2016-09-28 16:42:45 PDT
Comment on attachment 290128 [details]
[PATCH] For Bots - Windows Yet Again?!

Attachment 290128 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2164234

New failing tests:
js/dom/stack-trace.html
Comment 40 Build Bot 2016-09-28 16:42:49 PDT
Created attachment 290140 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 41 Joseph Pecoraro 2016-09-28 16:47:39 PDT
Created attachment 290142 [details]
[PATCH] Proposed Fix

Drop unique_ptr because I can't see to get Windows to accept it.
Comment 42 Joseph Pecoraro 2016-09-28 16:48:23 PDT
Created attachment 290143 [details]
[PATCH] For Bots - All Necessary Parts
Comment 43 WebKit Commit Bot 2016-09-28 16:51:53 PDT
Attachment 290143 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 Build Bot 2016-09-28 18:20:27 PDT
Comment on attachment 290143 [details]
[PATCH] For Bots - All Necessary Parts

Attachment 290143 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2164787

New failing tests:
fast/images/pdf-as-image-with-annotations.html
Comment 45 Build Bot 2016-09-28 18:20:32 PDT
Created attachment 290157 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 46 Joseph Pecoraro 2016-09-28 19:04:08 PDT
Comment on attachment 290157 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

Obsoleting test results. Failure appears unrelated.
Comment 47 Joseph Pecoraro 2016-09-29 11:10:47 PDT
Created attachment 290220 [details]
[PATCH] Proposed Fix

Since its been so long I merged in some of the fixes I was going to do in follow-ups.
Comment 48 Joseph Pecoraro 2016-09-29 11:11:32 PDT
Created attachment 290221 [details]
[PATCH] For Bots - All Necessary Parts
Comment 49 WebKit Commit Bot 2016-09-29 11:14:52 PDT
Attachment 290221 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/SourceProvider.h:37:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/debugger/DebuggerParseData.cpp:79:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 2 in 76 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 BJ Burg 2016-09-29 13:48:17 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

Agent parts look fine. I have not and probably will not read through all of the test output. I think it will be more informative to see how the baseline changes over time as we fix bugs that we notice.

> LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:130
> + ->   9        a#();

What's going on here with the next few ones? This one in particular I would expect to pause after a (or evaluating an expression foo.a) but prior to the function call.

> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:9
> +    // logs all unique pause locations with the originator line:column.

Holy cow. This is interesting.

> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:52
> +    // logs its pause location. Clears breakpoints each line.

Grammaro.

> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:63
> +                                InspectorTest.log(`INSERTING AT: ${line}:${0}`);

Why ${0}? What does this do?

> LayoutTests/inspector/debugger/resources/log-pause-location.js:28
> +        return sourceCode.requestContent()

NIce.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:305
> +static bool parseLocation(ErrorString& errorString, const InspectorObject& location, JSC::SourceID& sourceID, unsigned& lineNumber, unsigned& columnNumber)

Kind of gross, could we return an Optional struct instead?

EDIT: oh, it's moved code. Meh, either way.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:438
> +    auto debugServerBreakpointIDsIterator = m_breakpointIdentifierToDebugServerBreakpointIDs.find(breakpointIdentifier);

Ugh, I don't like this overuse of 'breakpointId', but this is probably not the time to fix that.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:137
> +    // RefPtr<Inspector::Protocol::Debugger::Location> resolveBreakpoint(const String& breakpointIdentifier, JSC::SourceID, const ScriptBreakpoint&);

Oops.
Comment 51 Joseph Pecoraro 2016-09-29 16:27:45 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:130
>> + ->   9        a#();
> 
> What's going on here with the next few ones? This one in particular I would expect to pause after a (or evaluating an expression foo.a) but prior to the function call.

Breakpoint locations are all the places step-over would stop you. If the input location is before the name it slides to it "|a()" but if you set it after the name it slides to the next pause location, which here is the condition inside the if. This matches when stepping through the code, stepping out of a() would place you before the if's condition.

>> LayoutTests/inspector/debugger/breakpoints/resources/dump.js:63
>> +                                InspectorTest.log(`INSERTING AT: ${line}:${0}`);
> 
> Why ${0}? What does this do?

Heh, that is just 0. I'll clean that up!

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:438
>> +    auto debugServerBreakpointIDsIterator = m_breakpointIdentifierToDebugServerBreakpointIDs.find(breakpointIdentifier);
> 
> Ugh, I don't like this overuse of 'breakpointId', but this is probably not the time to fix that.

Yeah, there are two breakpoint types:

    JSC::Breakpoint
        - this gets resolved to a particular line/column in a JSC::SourceCode
        - JSC::BreakpointID created by JSC::Debugger

    ScriptBreakpoint
        - this is the Inspector's breakpoint
        - it is often set by URL, so ultimately it may spawn multiple JSC::Breakpoints
        - it has a list of breakpoint actions
        - we refer to it by its "breakpointIdentifier"

It is a bit hairy.
Comment 52 Mark Lam 2016-09-29 17:43:37 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

Some comments and questions below.

> Source/JavaScriptCore/ChangeLog:14
> +        get this information without requiring the code have executed (the

requiring the code *to* have executed

> Source/JavaScriptCore/ChangeLog:39
> +        statement that will execute. This skips past the lines definition of

Remove "lines" here.

> Source/JavaScriptCore/ChangeLog:41
> +        have been hoisted, happening before execution of the other statements.

/happening/which would have happened/ ?

> Source/JavaScriptCore/ChangeLog:49
> +        line 6. The debugger will pause whenever execution leaves foo without
> +        execption. This matches stepping behavior. An explicit or implicit

/without exception/due to a return and not an exception/.  "without exception" reads like "always".  BTW, typo in "execption".

What's an "implicit return"?

> Source/JavaScriptCore/ChangeLog:55
> +        At this point op_debug's are still emitted at custom locations during

Please add a comma between "point" and "op_debug's".

> Source/JavaScriptCore/ChangeLog:58
> +        determined were breakpoint locations the Parser sets a "needs debug

Missing comma between "locations" and "the Parser".

> Source/JavaScriptCore/ChangeLog:110
> +        current node. In the SyntaxChecker return an invalid position.

Missing comma between "SyntaxChecker" and "return".

> Source/JavaScriptCore/ChangeLog:202
> +        ScriptDebugServer::Script is effectively duplication most of the data from

/duplication/duplicating/.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3473
> +void BytecodeGenerator::emitDebugHook(DebugHookID debugHookID, unsigned line, unsigned charOffset, unsigned lineStart)

DebugHookID is a bad name.  DebugHookType or DebugHookEvent would better .  I know this is pre-existing code.  So, maybe rename it in a subsequent patch?

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4153
> +            emitDebugHook(forLoopNode->expr(), WillExecuteStatement);

Nit: why not have the DebugHookID before the expr?  Except for the statement version, all the other emitDebugHook()s take the DebugHookID first.

> Source/JavaScriptCore/debugger/Debugger.cpp:340
> +DebuggerParseData Debugger::debuggerParseData(SourceID sourceID, SourceProvider* provider)

DebuggerParseData contains a DebuggerPausePositions which contains a Vector of DebuggerPausePosition.  Wouldn't returning by value trigger a copy of the Vector?  Why not return a const DebuggerParseData& here?  Remember to return the reference of the value in the map below instead of the temp used to initialize it.

> Source/JavaScriptCore/debugger/Debugger.cpp:359
> +    // FIXME: Clean this up with TextPosition which has oneBasedInt and zeroBasedInt.

Add a bug url?

> Source/JavaScriptCore/debugger/Debugger.cpp:376
> +BreakpointID Debugger::setBreakpoint(Breakpoint& breakpoint, bool& existing)

Sad that this setBreakpoint() is for the most part identical to the other setBreakpoint below.  Would be nice if we can share the common parts and parameterize the differences somehow.  I'm ok with leaving to a later refactoring step.

Oh wait, is the other setBreakpoint() still in use?  If not, let's delete it.

> Source/JavaScriptCore/debugger/Debugger.cpp:506
> -
> +    

Please undo empty space.

> Source/JavaScriptCore/debugger/Debugger.h:78
>      BreakpointID setBreakpoint(Breakpoint, unsigned& actualLine, unsigned& actualColumn);

Ditto.  Delete if no longer used.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:94
> +    bool shouldEnterFunction = firstSlidePosition.position.line == line;

Should this also require that column be unspecified or column < firstSlidePosition.position.column?

Consider this scenario:

   <user wants to set break point here> function foo() { ... } y = 10;

versus this one:

   function foo() { ... } <user wants to set break point here> y = 10;

In the first, we should enter the function.  In the second, we should not.  The only difference between the 2 is where the user specifies the column of the desired breakpoint to be.

> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:113
> +        // Start skipping functions.
> +        if (slidePosition.type == DebuggerPausePositionType::Enter) {
> +            entryStackSize++;
> +            continue;
> +        }

Is this correct?  What if we have the following scenario:

    function foo() { function goo() { // requested breakpoint on this line.
        ... }
    }

The breakpoint should be in goo(), not foo(), right?

> Source/JavaScriptCore/debugger/DebuggerParseData.h:2
> - * Copyright (C) 2013 Apple Inc. All Rights Reserved.
> + * Copyright (C) 2016 Apple Inc. All rights reserved.

Did you svn cp DebuggerParseData.h from SourceProvider.h?  The diffs here implies that you replaced SourceProvider with DebuggerParseData, which is not the case.  To keep the svn history clean, I suggest you save a copy of DebuggerParseData.h to the side, svn revert it, and then svn add it.  That way, it gets a clean history, and won't be showing as a diff from SourceProvider.h.

> Source/JavaScriptCore/debugger/DebuggerParseData.h:23
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

Please remove empty space at the end.

> Source/JavaScriptCore/inspector/ScriptDebugServer.cpp:197
> +    // FIXME: Eliminate all the extra properties that derive from SourceProvider.

Add bug url?

> Source/JavaScriptCore/inspector/ScriptDebugServer.h:53
> +    // FIXME: Move BreakpointAction handling into JSC::Debugger or InspectorDebuggerAgent.

Add bug url?

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:505
> +    JSC::Breakpoint breakpoint(sourceID, lineNumber, columnNumber, condition, autoContinue, ignoreCount);
> +    Script& script = scriptIterator->value;
> +    resolveBreakpoint(script, breakpoint);
> +    if (!breakpoint.resolved) {
> +        m_scriptDebugServer.continueProgram();
> +        errorString = ASCIILiteral("Could not resolve breakpoint");
> +        return;
> +    }

If the user is pointing at a location and asking the debugger to continue to that location, is it possible to ever fail to resolve a breakpoint at that location?

> Source/JavaScriptCore/parser/Parser.h:27
> +#include "Interpreter.h"

Do you need this?
Comment 53 Joseph Pecoraro 2016-09-29 18:46:45 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/ChangeLog:49
>> +        execption. This matches stepping behavior. An explicit or implicit
> 
> /without exception/due to a return and not an exception/.  "without exception" reads like "always".  BTW, typo in "execption".
> 
> What's an "implicit return"?

The implicit "return undefined" when leaving a function.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3473
>> +void BytecodeGenerator::emitDebugHook(DebugHookID debugHookID, unsigned line, unsigned charOffset, unsigned lineStart)
> 
> DebugHookID is a bad name.  DebugHookType or DebugHookEvent would better .  I know this is pre-existing code.  So, maybe rename it in a subsequent patch?

I agree, I'll do this in a follow-up.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:4153
>> +            emitDebugHook(forLoopNode->expr(), WillExecuteStatement);
> 
> Nit: why not have the DebugHookID before the expr?  Except for the statement version, all the other emitDebugHook()s take the DebugHookID first.

This will be eliminated in a later patch.

>> Source/JavaScriptCore/debugger/Debugger.cpp:340
>> +DebuggerParseData Debugger::debuggerParseData(SourceID sourceID, SourceProvider* provider)
> 
> DebuggerParseData contains a DebuggerPausePositions which contains a Vector of DebuggerPausePosition.  Wouldn't returning by value trigger a copy of the Vector?  Why not return a const DebuggerParseData& here?  Remember to return the reference of the value in the map below instead of the temp used to initialize it.

Thanks! I had this exact question earlier. I changed this code to get Windows to build. I'll make changes.

>> Source/JavaScriptCore/debugger/Debugger.cpp:376
>> +BreakpointID Debugger::setBreakpoint(Breakpoint& breakpoint, bool& existing)
> 
> Sad that this setBreakpoint() is for the most part identical to the other setBreakpoint below.  Would be nice if we can share the common parts and parameterize the differences somehow.  I'm ok with leaving to a later refactoring step.
> 
> Oh wait, is the other setBreakpoint() still in use?  If not, let's delete it.

Oh, you're right I probably left the dead code in here.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:94
>> +    bool shouldEnterFunction = firstSlidePosition.position.line == line;
> 
> Should this also require that column be unspecified or column < firstSlidePosition.position.column?
> 
> Consider this scenario:
> 
>    <user wants to set break point here> function foo() { ... } y = 10;
> 
> versus this one:
> 
>    function foo() { ... } <user wants to set break point here> y = 10;
> 
> In the first, we should enter the function.  In the second, we should not.  The only difference between the 2 is where the user specifies the column of the desired breakpoint to be.

In your example we wouldn't have gotten to this part of the algorithm. We would have returned in the immediately preceding block.

At this point in the algorithm we've determined that the next pause point immediately after the input position (line:column) is a function Entry. So if it is on the same line it must be a >= column.

>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:113
>> +        }
> 
> Is this correct?  What if we have the following scenario:
> 
>     function foo() { function goo() { // requested breakpoint on this line.
>         ... }
>     }
> 
> The breakpoint should be in goo(), not foo(), right?

Hmm. This is true but I see both behaviors as being acceptable, So I think the simpler the better. Which is what we currently have:

      (1) <here> function foo() { function goo() {
      (2) function foo() { <here> function goo() {

(1) would pause in foo
(2) would pause in goo

Fortunately, pretty printing in Web Inspector makes setting breakpoints at these locations possible, and visually the user would see foo and goo as being on different lines.

>> Source/JavaScriptCore/debugger/DebuggerParseData.h:2
>> + * Copyright (C) 2016 Apple Inc. All rights reserved.
> 
> Did you svn cp DebuggerParseData.h from SourceProvider.h?  The diffs here implies that you replaced SourceProvider with DebuggerParseData, which is not the case.  To keep the svn history clean, I suggest you save a copy of DebuggerParseData.h to the side, svn revert it, and then svn add it.  That way, it gets a clean history, and won't be showing as a diff from SourceProvider.h.

This is just git diff making assumptions. It happens.
Comment 54 Joseph Pecoraro 2016-09-29 19:30:05 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:505
>> +    }
> 
> If the user is pointing at a location and asking the debugger to continue to that location, is it possible to ever fail to resolve a breakpoint at that location?

Yes. The only case I can think of is a line past the last statement. For example, on line 2 in this script:

    1. var x;
    2.

We could make end of program a pause point so it would always resolve to something.
Comment 55 Mark Lam 2016-09-30 10:35:39 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

>>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:94
>>> +    bool shouldEnterFunction = firstSlidePosition.position.line == line;
>> 
>> Should this also require that column be unspecified or column < firstSlidePosition.position.column?
>> 
>> Consider this scenario:
>> 
>>    <user wants to set break point here> function foo() { ... } y = 10;
>> 
>> versus this one:
>> 
>>    function foo() { ... } <user wants to set break point here> y = 10;
>> 
>> In the first, we should enter the function.  In the second, we should not.  The only difference between the 2 is where the user specifies the column of the desired breakpoint to be.
> 
> In your example we wouldn't have gotten to this part of the algorithm. We would have returned in the immediately preceding block.
> 
> At this point in the algorithm we've determined that the next pause point immediately after the input position (line:column) is a function Entry. So if it is on the same line it must be a >= column.

Good point.  I missed that.

>>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:113
>>> +        }
>> 
>> Is this correct?  What if we have the following scenario:
>> 
>>     function foo() { function goo() { // requested breakpoint on this line.
>>         ... }
>>     }
>> 
>> The breakpoint should be in goo(), not foo(), right?
> 
> Hmm. This is true but I see both behaviors as being acceptable, So I think the simpler the better. Which is what we currently have:
> 
>       (1) <here> function foo() { function goo() {
>       (2) function foo() { <here> function goo() {
> 
> (1) would pause in foo
> (2) would pause in goo
> 
> Fortunately, pretty printing in Web Inspector makes setting breakpoints at these locations possible, and visually the user would see foo and goo as being on different lines.

I agree that with pretty printing (which is on by default, right?), this issue will be moot.  And when pretty printing is off, the ambiguity is probably rare.  After thinking about this some more, I can't imagine a seriously bad consequence of going with your current approach.  So, I'm ok with it until there is evidence to the contrary.

On a separate note, I keep feeling that this dance with shouldEnterFunction and entryStackSize is a bit tricky to follow.  I wonder if this can be expressed in a simpler way.  How about:

    int entryStackSize = -1;
    for (unsigned i = start; i < m_positions.size(); ++i) {
        DebuggerPausePosition& slidePosition = m_positions[i];

        if (slidePosition.type == DebuggerPausePositionType::Enter) {
            entryStackSize++;
            continue;
        }

        ASSERT(entryStackSize >= 0);
        if (!entryStackSize)
            return Optional<JSTextPosition>(slidePosition.position);

        if (slidePosition.type == DebuggerPausePositionType::Leave)
            entryStackSize--;
    }

This removes the need for shouldEnterFunction.  It also ensures that we break on the Leave position if there's no other positions between Enter and Leave.

>>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:505
>>> +    }
>> 
>> If the user is pointing at a location and asking the debugger to continue to that location, is it possible to ever fail to resolve a breakpoint at that location?
> 
> Yes. The only case I can think of is a line past the last statement. For example, on line 2 in this script:
> 
>     1. var x;
>     2.
> 
> We could make end of program a pause point so it would always resolve to something.

I feel that end of program should be a pause point.  In which case, let's assert here that the breakpoint is resolved.

I haven't looked carefully enough but do we need to make a similar change at the other places in this patch that checks if a breakpoint is resolved?
Comment 56 Mark Lam 2016-09-30 11:42:13 PDT
Comment on attachment 290220 [details]
[PATCH] Proposed Fix

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

r=me with outstanding issues resolved.

>>>> Source/JavaScriptCore/debugger/DebuggerParseData.cpp:113
>>>> +        }
>>> 
>>> Is this correct?  What if we have the following scenario:
>>> 
>>>     function foo() { function goo() { // requested breakpoint on this line.
>>>         ... }
>>>     }
>>> 
>>> The breakpoint should be in goo(), not foo(), right?
>> 
>> Hmm. This is true but I see both behaviors as being acceptable, So I think the simpler the better. Which is what we currently have:
>> 
>>       (1) <here> function foo() { function goo() {
>>       (2) function foo() { <here> function goo() {
>> 
>> (1) would pause in foo
>> (2) would pause in goo
>> 
>> Fortunately, pretty printing in Web Inspector makes setting breakpoints at these locations possible, and visually the user would see foo and goo as being on different lines.
> 
> I agree that with pretty printing (which is on by default, right?), this issue will be moot.  And when pretty printing is off, the ambiguity is probably rare.  After thinking about this some more, I can't imagine a seriously bad consequence of going with your current approach.  So, I'm ok with it until there is evidence to the contrary.
> 
> On a separate note, I keep feeling that this dance with shouldEnterFunction and entryStackSize is a bit tricky to follow.  I wonder if this can be expressed in a simpler way.  How about:
> 
>     int entryStackSize = -1;
>     for (unsigned i = start; i < m_positions.size(); ++i) {
>         DebuggerPausePosition& slidePosition = m_positions[i];
> 
>         if (slidePosition.type == DebuggerPausePositionType::Enter) {
>             entryStackSize++;
>             continue;
>         }
> 
>         ASSERT(entryStackSize >= 0);
>         if (!entryStackSize)
>             return Optional<JSTextPosition>(slidePosition.position);
> 
>         if (slidePosition.type == DebuggerPausePositionType::Leave)
>             entryStackSize--;
>     }
> 
> This removes the need for shouldEnterFunction.  It also ensures that we break on the Leave position if there's no other positions between Enter and Leave.

I take this back.  Joe pointed out that the original also accounts for the need to slide past the function if the requested breakpoint is not on the line declaring the function.  My alternative broke that.  With that in mind, Joe's loop makes more sense to me now.
Comment 57 Joseph Pecoraro 2016-09-30 12:07:56 PDT
> > We could make end of program a pause point so it would always resolve to something.
> 
> I feel that end of program should be a pause point.  In which case, let's
> assert here that the breakpoint is resolved.

I agree. I'll do this in a follow-up so it can have its own tests.
Comment 58 Joseph Pecoraro 2016-09-30 12:37:19 PDT
<https://trac.webkit.org/changeset/206653>
Comment 59 Joseph Pecoraro 2016-09-30 16:37:29 PDT
Follow-up for Module tests:
https://trac.webkit.org/changeset/206671