Bug 186453 – Web Inspector: REGRESSION (r195723): Improve support for resources with '\r' and '\r\n' line endings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
CodeMirrorEditor shouldn't force line endings to "\n". This causes files with Windows line endings (CRLF) to be converted, leaving a trailing CR on every line. This messes up text selection and copy/pasting text from resources.

Steps to Reproduce:
1. Inspect https://app.cloudbilling.nl
2. Select some HTML from the editor in the Resources tab
3. Copy
4. Paste somewhere else
  => Pasted text doesn't match the selection

This regressed in https://bugs.webkit.org/show_bug.cgi?id=153529.

[Matt Baker]

<rdar://problem/39689180>

[Matt Baker]

Created attachment 342354 [details]
Patch

[Matt Baker]

Hmm, it looks like not all type tokens are being added. Need to investigate more.

[Joseph Pecoraro]

Comment on attachment 342354 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:123
> -            if (this._codeMirror.getValue() !== newString) {
> +            if (this._codeMirror.getValue() !== newString)
>                  this._codeMirror.setValue(newString);
> -                console.assert(this.string.length === newString.length, "A lot of our code depends on precise text offsets, so the string should remain the same.");
> -            } else {
> +            else {

Hmm, I have a bad feeling about this. I'm pretty sure we make a bunch of assumptions that lines are up to \n in backend and frontend code. This feels very risky to me.

[EWS Watchlist]

Comment on attachment 342354 [details]
Patch

Attachment 342354 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8094509

New failing tests:
http/tests/security/local-video-source-from-remote.html

[EWS Watchlist]

Created attachment 342358 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Matt Baker]

Comment on attachment 342354 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:123
>> +            else {
> 
> Hmm, I have a bad feeling about this. I'm pretty sure we make a bunch of assumptions that lines are up to \n in backend and frontend code. This feels very risky to me.

I'm not sure this is the case. The backend seems to be largely line-ending agnostic: Inspector protocol Debugger.Location, ScriptProfiler.ExpressionLocation, and ScriptProfiler.StackFrame use line/column, as do commands for setting breakpoints. Pause location, error/warning widgets, CodeMirrorCompletionController, and CodeMirrorTokenTrackingController also deal with lines/columns.

One area where we rely on the original source and CodeMirror-transformed source being identical are the execution highlight and code coverage highlight. I'll see if this can be based on line/column as well.

[Matt Baker]

Created attachment 342492 [details]
[Image] Incorrect execution highlight

Incorrect ranges for code coverage and execution highlight in scripts with Windows (CRLF) line endings.

[Matt Baker]

Created attachment 342597 [details]
WIP

[Matt Baker]

(In reply to Matt Baker from comment #9)
> Created attachment 342597 [details]
> WIP

This fixes missing type token annotations. Currently working on fixing execution and basic block highlighting.

[EWS Watchlist]

Comment on attachment 342597 [details]
WIP

Attachment 342597 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8161803

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 342645 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Matt Baker]

A few clarifying notes:

This bug was caused by https://webkit.org/b/153529. Prior to this change,

a) CodeMirror would automatically detect line endings
b) In CodeMirror a script with Windows line endings (\r\n) would have a different length, and thus different token offsets, than the equivalent script in the backend/Esprima
c) The text editor in the frontend always stripped all line separator characters from the displayed text, preventing the issue raised in this bug

As a result of b) above, the assertion in TextEditor.js:123 would have failed for Windows scripts, had it existed at the time.

The solution is to let CodeMirror auto-detect line endings again. Three parts of the frontend will need to be updated to use line/column based locations instead of offsets when dealing with the AST before this change can be made:

1) Execution highlighting <https://webkit.org/b/187532>
2) Type token annotations <https://webkit.org/b/187612>
3) Code coverage highlighting <https://webkit.org/b/187613>

[Joseph Pecoraro]

(In reply to Matt Baker from comment #13)
> A few clarifying notes:
> 
> This bug was caused by https://webkit.org/b/153529. Prior to this change,
> 
> a) CodeMirror would automatically detect line endings
> b) In CodeMirror a script with Windows line endings (\r\n) would have a
> different length, and thus different token offsets, than the equivalent
> script in the backend/Esprima
> c) The text editor in the frontend always stripped all line separator
> characters from the displayed text, preventing the issue raised in this bug
> 
> As a result of b) above, the assertion in TextEditor.js:123 would have
> failed for Windows scripts, had it existed at the time.
> 
> The solution is to let CodeMirror auto-detect line endings again. Three
> parts of the frontend will need to be updated to use line/column based
> locations instead of offsets when dealing with the AST before this change
> can be made:
> 
> 1) Execution highlighting <https://webkit.org/b/187532>
> 2) Type token annotations <https://webkit.org/b/187612>
> 3) Code coverage highlighting <https://webkit.org/b/187613>

I think we should be explicit about lineEndings instead of using CodeMirror's auto behavior (even though we will match its current behavior I want to make sure if CodeMirror changes we still get our expectation).

These sound like a good change. They should get us to work better with files containing various line endings (1) \n (2) \r\n (3) \r (4) mixed. Instead of right now where we only do a good job with (1).

There is at least one additional change needed in the backend for (3) in ContentSearchUtilities lineEndings(), but that can come later since \r only is so rare, that we can basically ignore it at the moment.

[Joseph Pecoraro]

Comment on attachment 342597 [details]
WIP

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

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: REGRESSION (r195723): CodeMirrorEditor shouldn't force line endings to "\n"

I really like the direction of this patch.

I think a better name for it would be "Web Inspector: Improve support for resources with '\r' and '\r\n' line endings" because that is really what we are starting to do here, starting with the most impactful: making the editor features (selection, copy) work with such files.

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorEditor.js:-31
> -        if (options.lineSeparator === undefined)
> -            options.lineSeparator = "\n";

This is where I'd prefer we be explicit with a comment, instead of defaulting to CodeMirror's default (because what if CodeMirror changes).

    // Default line endings match typical expected line endings for JavaScript (at least those supported by JavaScriptCore).
    if (options.lineSeparator === undefined)
        options.lineSeparator = /\r\n?|\n/;

This doesn't handle the exotic line endings (U+2028 LINE SEPARATOR, U+2029 PARAGRAPH SEPARATOR) that ECMAScript specifies / Esprima supports, but given how unlikely they are its safe to not focus on those.
https://tc39.github.io/ecma262/#sec-line-terminators

On top of these, CSS also supports \f:
https://www.w3.org/TR/css-syntax-3/#newline-diagram

I don't think we need to given how unlikely that is but we could specify different line endings for CSS resources. Either way, this better handle \r, and \r\n with CSS resources as well, in case we needed to match up with line numbers we get from the backend for those resources (assuming the backend respects those line endings, and CSSTokenizer appears to). I didn't check but this is almost certainly an improvement for viewing HTML resources as well, I just didn't see where it determines line numbers for any error messages it produces.

[Matt Baker]

Created attachment 344956 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344956 [details]
Patch

Clearing flags on attachment: 344956

Committed r233824: <https://trac.webkit.org/changeset/233824>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.