Handle windows drive letters using pipes in URL parsing

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: manishearth, Assigned: manishearth)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(2 attachments)

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
(Assignee)

Comment 1

2 years ago
Chrome and Safari agree with this buggy behavior.
(Assignee)

Comment 2

2 years ago
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?
Flags: needinfo?(valentin.gosu)
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.
Flags: needinfo?(valentin.gosu)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → manishearth
Status: NEW → ASSIGNED

Comment 5

2 years ago
mozreview-review
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!
Attachment #8819712 - Flags: review?(valentin.gosu) → review+
(Assignee)

Comment 6

2 years ago
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

2 years ago
mozreview-review
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?
(Assignee)

Comment 8

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8819755 [details]
Bug 1324251 - Test updates for windows drive letters;

https://reviewboard.mozilla.org/r/99394/#review100114
Attachment #8819755 - Flags: review?(valentin.gosu) → review+

Comment 14

2 years ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 years ago
Pushed by manishearth@gmail.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7cf7d0a3cc0e
https://hg.mozilla.org/mozilla-central/rev/3c8f210688e2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.