1320502 - "Paste and Go" does not work if CRLF exists at the end of clipboard url.

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Alice0775 White]

Attachment: url.txt

Affected Nightly53.0a1.
Not affected Aurora52.0a2.

If CRLF exists at the end of clipboard url.
"Paste and Go" does not work. It only perform paste.

Steps To Reproduce:
1. Copy url to Clipboard from notepad
   Open attached with notepad
   Click left side of "h" and hold mouse down and move downward to select a line.
   Copy to clipboard ( Ctrl+C )
   
2. Open New Tab
3. Right Click on Location Bar and Choose "Paste and Go"

Actual Results:
The url is pasted. The trailing CRLF seems to be ignored and stripped.
However, Nothing loads.

Expected Results:
The trailing CRLF shuld be ignored and stripped.
The url should load.

Comment 1

[Alice0775 White]

Regressed by: 37a877e840fc	Gijs Kruitbosch — Bug 1229426 - avoid dnd of js URIs, r=mak

Comment 2

[Alice0775 White]

[Tracking Requested - why for this release]: Broken all function related clipboard of Location Bar.

Steps To Reproduce:
1. Copy url to Clipboard:
     Open attached with notepad.exe
     Click left side of "h" and hold mouse down and move downward to select a line.
     Copy to clipboard ( Ctrl+C )
   
2. Open New Tab
3. Attempt to open with clipboard url:
     Right Click on Location Bar and Choose "Paste & Go"
     Focus Location Bar, paste ( Ctrk+v ) , keypress [Enter] or click Go button[->] 

Actual Results:
The url is pasted. The trailing CRLF seems to be ignored and stripped.
However, Nothing loads.

Expected Results:
The trailing CRLF shuld be ignored and stripped.
The url should load.

Comment 3

[Alice0775 White]

Attachment: test.html

STR
1. Copy url to Clipboard:
     Open attached html
     Click left side of "h" and hold mouse down and move downward to select a line.
     Copy to clipboard ( Ctrl+C )
2. ...
3. ...

Comment 4

[Marco Bonardo [:mak]]

hm, it's the change to stripUnsafeProtocolOnPaste, it now also strips newlines, but that means it will fail the str == fixed_str check we use with it.

Comment 5

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #4)
> hm, it's the change to stripUnsafeProtocolOnPaste, it now also strips
> newlines, but that means it will fail the str == fixed_str check we use with
> it.

Ugh, sorry for missing that. I'll check if I can add a test.

Comment 6

[:Gijs (he/him)]

Attachment: Bug 1320502 - fix paste (and go) issues regressed in bug 1229426,

Review commit: https://reviewboard.mozilla.org/r/96000/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96000/

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 8814941 [details]
Bug 1320502 - fix paste (and go) issues regressed in bug 1229426,

https://reviewboard.mozilla.org/r/96000/#review96076

::: browser/base/content/browser.js:5639
(Diff revision 1)
> -  return pasteData.replace(/\r?\n/g, "").replace(/^(?:\s*javascript:)+/i, "");
> +  let sanitizedPasteData = pasteData.replace(/\r?\n/g, "");
> +  let pasteDataNoJS = sanitizedPasteData.replace(/^(?:\s*javascript:)+/i, "");
> +  if (pasteDataNoJS.length != sanitizedPasteData.length) {
> +    return pasteDataNoJS;
> +  }
> +  return pasteData;

nit: in case you would like to avoid the temp var and length comparison, an alternative could be:

let changed = false;
let pasteDataNoJS = pasteData.replace(/\r?\n/g, "")
                             .replace(/^(?:\s*javascript:)+/i,
                                      () => { changed = true;
                                              return ""; })
return changed ? pasteDataNoJS : pasteData;

::: browser/base/content/test/urlbar/browser_pasteAndGo.js:18
(Diff revision 1)
> +      gURLBar.focus();
> +      yield new Promise((resolve, reject) => {
> +        waitForClipboard(url, function() {
> +          clipboardHelper.copyString(url);
> +        }, resolve,
> +          reject.bind(new Error("Failed to copy string '" + url + "' to clipboard"))

does this really work? I've never seen binding a reject to a new Error...
Either you meant () => reject(New Error
or I learned something new!

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [::mak] from comment #7)
> > +          reject.bind(new Error("Failed to copy string '" + url + "' to clipboard"))
> 
> does this really work? I've never seen binding a reject to a new Error...
> Either you meant () => reject(New Error
> or I learned something new!

Re-reading, I think I meant reject.bind(null, new Error(...)). That should work, I think, though the arrow function might be clearer. Why wouldn't it?

Comment 9

[:Gijs (he/him)]

Comment on attachment 8814941 [details]
Bug 1320502 - fix paste (and go) issues regressed in bug 1229426,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96000/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a64450cc56a
fix paste (and go) issues regressed in bug 1229426, r=mak

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3a64450cc56a

Comment 12

[Marcia Knous [:marcia]]

Tracking 53- as this is resolved fixed.

Comment 13

[Md. Majedul islam]

I have reproduced this bug with Firefox nightly 53.0a1 (2016-11-26)(64-bit) on Windows 10, 64 Bit.

The Bug's fix is now verified on latest nightly 53.0a1.

Build ID 	 20161207030204

User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0.

[bugday-20161207]