1324251 - Handle windows drive letters using pipes in URL parsing

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Manish Goregaokar [:manishearth]]

https://url.spec.whatwg.org/#windows-drive-letter

> A Windows drive letter
is two code points, of which the first is an ASCII alpha and the second is either ":" or "|". 

`new URL("file:///c|/")` should parse as "file:///c:" without percent-escaping the breakbar.

There are tests for this in https://github.com/w3c/web-platform-tests/blob/be5820ecd36650bff4728208629553931fd42ec2/url/urltestdata.json#L1337

Comment 1

[Manish Goregaokar [:manishearth]]

Chrome and Safari agree with this buggy behavior.

Comment 2

[Manish Goregaokar [:manishearth]]

It looks like we have an explicit |#if defined(XP_WIN)| in nsNoAuthURLParser::ParseAfterScheme here.

We can make this a runtime option for the rust-url parser, I guess. I'd prefer to make Firefox parse these unconditionally. What do you think?

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I always found it odd that we treat it differently. I don't know why it it that way (I'd have to dig through some ancient bugs), but I think it makes sense to make it unconditionally spec compliant.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1324251 - Unconditionally parse windows drive letters in file paths;

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

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

https://reviewboard.mozilla.org/r/99372/#review99656

Looks good. But please make sure we either have tests for this, or add some. Thanks!

Comment 6

[Manish Goregaokar [:manishearth]]

Yeah, it's running on try right now. IIRC there are WPT tests for it so I'm waiting for the try build to turn up any issues.

Comment 7

[Masatoshi Kimura [:emk]]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

https://reviewboard.mozilla.org/r/99372/#review99660

::: netwerk/base/nsURLParsers.cpp:379
(Diff revision 1)
>      case 2:
>          {
>              const char *p = nullptr;
>              if (specLen > 2) {
>                  // looks like there is an authority section
> -#if defined(XP_WIN)
> +

It looks to me like this case handles URLs such as "file://c|/" or "file://c:/" (two slashes) rather than "file:///c|/" or "file:///c:/" (three slashes). Is this patch really correct?

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

https://reviewboard.mozilla.org/r/99372/#review99660

> It looks to me like this case handles URLs such as "file://c|/" or "file://c:/" (two slashes) rather than "file:///c|/" or "file:///c:/" (three slashes). Is this patch really correct?

Actually, this doesn't work at all. The slashes part is okay and should work, but there's no actual code to replace the | with a : as the spec asks us to.

Comment 9

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

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

Comment 10

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1324251 - Test updates for windows drive letters;

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

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

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

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819755 [details]
Bug 1324251 - Test updates for windows drive letters;

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

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8819755 [details]
Bug 1324251 - Test updates for windows drive letters;

https://reviewboard.mozilla.org/r/99394/#review100114

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

https://reviewboard.mozilla.org/r/99372/#review100106

::: netwerk/base/nsStandardURL.cpp:938
(Diff revision 3)
>          i = AppendSegmentToBuf(buf, i, spec, directory, mDirectory,
>                                 &encDirectory, useEncDirectory, &diff);
>          ShiftFromBasename(diff);
>  
> +
> +

nit: No whitespace changes.

::: netwerk/base/nsStandardURL.cpp
(Diff revision 3)
>          mHost.mPos += mAuthority.mPos;
>      }
>  
>      if (mPath.mLen > 0)
>          rv = ParsePath(spec, mPath.mPos, mPath.mLen);
> -

nit: No whitespace changes

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819712 [details]
Bug 1324251 - Unconditionally parse windows drive letters in file paths;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99372/diff/3-4/

Comment 16

[Manish Goregaokar [:manishearth]]

Comment on attachment 8819755 [details]
Bug 1324251 - Test updates for windows drive letters;

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7cf7d0a3cc0e
Unconditionally parse windows drive letters in file paths; r=valentin
https://hg.mozilla.org/integration/autoland/rev/3c8f210688e2
Test updates for windows drive letters; r=valentin

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cf7d0a3cc0e
https://hg.mozilla.org/mozilla-central/rev/3c8f210688e2