Fix file URL reparse issue

[rmisev]

Fixes issue by ensuring file URL's host is not null.

Changes:

• Set url's host to the empty string at the start of file state.
• Updates URL parser and serializer accordingly.

Fixes: #549

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • File URL reparse tests web-platform-tests/wpt#25989
• Implementation bugs are filed:
  • Chrome: no bugs
  • Firefox: no bugs
  • Safari: no bugs

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[annevk]

This only affects file URLs, right? Not all special URLs. I wonder if we should make that more explicit. There's a couple other cases where the differences between non-file special URLs and file URLs have caused issues, though turning specialness into a tri-state at this point does not seem super attractive either.

[rmisev]

This only affects file URLs, right? Not all special URLs.

Yes. Non-file special URLs always have not empty host, so they aren't affected. The condition url’s sheme is not "file:" can be used here as well, but 5.1. step is intended for non-special URLs, so I added url is not special. Which one is clearer?

[annevk]

That's a good point. Let's do the not special check first though so we start from the most significant component.

[annevk]

Perhaps @alwinb also wants to review this? And @domenic would ideally verify the tests using whatwg-url.

Writing this I wonder if another fix here could be that file URLs never have a null host. Did you look at that? (That would also allow for removal of step 3 in the serializer and at least one check elsewhere would be simplified so perhaps that's a bigger change best tackled separately.)

[domenic]

Tested against whatwg-url with web-platform-tests/wpt#25989 and this works. Happy to continue testing other simplifications that folks suggest.

I wonder also if it'd be useful to add a new test suite that uses all of urltestdata.json to test idempotency.

[alwinb]

Hmm. Resolved file URLs should have a non-null host I think. For a moment I thought that maybe I failed to enforce that in the changes with #405, but I'm not sure now, I guess it was possible before. Anyway, if you can end up with resolved file URLs with a non-null host, I think it's better to change that than to change the serialiser.

The ambiguity can only arise with a null host, and the host being non-null is (or should be) a property of resolved special URLs, so I think the serialiser was correct.

[annevk]

Well, the serializer cannot be entirely correct due to step 3 also existing, right? And there's step 2.1.4 of the scheme state which should not have to check for null if that is true. (I think that's all.)

[rmisev]

Ok, updated URL parser to ensure file url's host is not null.

[annevk]

Thank you @rmisev. I like this a lot. I'll wait for @domenic to verify and would appreciate a quick skim from @alwinb as well.

[alwinb]

@annevk that's true. Step 3 does a bit of resolving inside the serializer, and the check for a null host in 2.1.4 is superfluous if you maintain the invariant that (resolved) file URLs have a non-null host. I don't see any other places either, but I'm not that familiar with this parser so I may have missed something. It seems removing step 3. in the serializer will also solve the idempotency issue, just then you allow null hosts in file URLs.

You could choose to merge this request and make larger changes later, for example together with changes such as for #548. I would try to move any resolving logic out of the serializer, but also prepare for moving it out of the parser. That's another, much larger topic though.

[alwinb]

Oh I missed the latest changes! I'l have a look in a moment.

[alwinb]

Yeah looks good.

[domenic]

These changes also pass the web platform tests PR and existing suite.