1323683 - Use of nsIURIWithQuery.filePath vs nsIURI.path is super error prone

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

As seen in couple of bugs, it is rather guess work when one is supposed to use 
nsIURIWithQuery.filePath and when nsIURI.path

Could we do something to remove this error prone stuff?

Comment 1

[Masatoshi Kimura [:emk]]

Why nsIURIWithQuery has been added in the first place?

var url = new URL("scheme://test/bla/stuff?query#ref");
url.host // should be "test", but ""

Custom protocol URIs should have implemented nsIURL.

Currently Chrome does not implement this correctly, either. But if we just clone Chrome's behavior, stop futile effort to develop Gecko/Servo/whatever and use Blink.

Comment 2

[Masatoshi Kimura [:emk]]

Wow, Edge has a decent URL parser.

Comment 3

[Andrea Marchesini [:baku]]

> var url = new URL("scheme://test/bla/stuff?query#ref");
> url.host // should be "test", but ""

why should it be?

https://url.spec.whatwg.org/#scheme-state section 8: "Otherwise, if remaining starts with an "/", set state to path or authority state, and increase pointer by one."

I still agree with smaug about cleaning up nsIURIWithQuery.filePath and nsIURI.path

Comment 4

[Masatoshi Kimura [:emk]]

(In reply to Andrea Marchesini [:baku] from comment #3)
> > var url = new URL("scheme://test/bla/stuff?query#ref");
> > url.host // should be "test", but ""
> 
> why should it be?

Because the spec is saying so.

> https://url.spec.whatwg.org/#scheme-state section 8: "Otherwise, if
> remaining starts with an "/", set state to path or authority state, and
> increase pointer by one."

Did you read "path or authority state"?

https://url.spec.whatwg.org/#path-or-authority-state
> If c is "/", set state to authority state.

https://url.spec.whatwg.org/#authority-state
> 1. If c is "@", ... (snip)
> 2. Otherwise, if one of the following is true 
>    * c is EOF code point, "/", "?", or "#"
>    * url is special and c is "\"
> then decrease pointer by the number of code points in buffer plus one, set buffer to the empty string, and set state to host state. 
> 3. Otherwise, append c to buffer.

https://url.spec.whatwg.org/#host-state
> 1. If c is ":" and the [] flag is unset, ...(snip)
> 2. Otherwise, if one of the following is true 
>    * c is EOF code point, "/", "?", or "#"
>    * url is special and c is "\"
> then decrease pointer by one, and run these substeps: 
>    1. If url is special and buffer is the empty string, return failure. 
>    2. Let host be the result of host parsing buffer.
>    3. If host is failure, return failure.
>    4. Set url’s host to host, buffer to the empty string, and state to path start state.

Please teach me if I'm missing something.

Comment 5

[Masatoshi Kimura [:emk]]

If the spec and Edge and I are wrong and Chrome is the truth of God, please at least file a spec bug.

Comment 6

[Andrea Marchesini [:baku]]

It seems you are totally right.

Comment 7

[Masatoshi Kimura [:emk]]

So nsIURIWithQuery should be removed in favor of the full nsIURL implementation for reducing confusion and better spec compliance.

Comment 8

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

(In reply to Masatoshi Kimura [:emk] from comment #7)
> So nsIURIWithQuery should be removed in favor of the full nsIURL
> implementation for reducing confusion and better spec compliance.

Spec compliance apart, nsIURIWithQuery was added because a lot of code follows this pattern [1]:
nsCOMPtr<nsIURL> url(do_QueryInterface(aURI));
if (!url) {
  return NS_ERROR_FAILURE;
}
If we want to make nsSimpleURL implement nsIURL, then we need to audit all of these instances [2].

As a side note, I'm considering replacing nsSimpleURI with RustURL (even before we do so for nsStandardURL). The switch should be easier, I think, and the web-compat issues less common than with special schemes.

[1] http://searchfox.org/mozilla-central/source/browser/components/shell/nsMacShellService.cpp#124
[2] http://searchfox.org/mozilla-central/search?q=%3CnsIURL%3E

Comment 9

[Masatoshi Kimura [:emk]]

I did not say nsSimpleURI should implement nsIURL. I said custom protocol URIs should implement nsIURL.
It would preserve the behavior for existing nsSimpleURI URLs (e.g. data:) and it would have even less web-compat issues.

Comment 10

[Andrea Marchesini [:baku]]

> As a side note, I'm considering replacing nsSimpleURI with RustURL (even
> before we do so for nsStandardURL). The switch should be easier, I think,
> and the web-compat issues less common than with special schemes.

Do you think we can wait until this is done or we should propose a quicker fix for this issue?
When do you think this RustURL replacement can happen?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

I wonder why pathIgnoringRef wasn't added to nsIURL. Or is the query part also relevant here?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

That was silly suggestio. I guess the reason is that some code wants (but which code! that is unclear) path without ref or query.

Comment 13

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

(In reply to Olli Pettay [:smaug] from comment #0)
> As seen in couple of bugs, it is rather guess work when one is supposed to
> use 
> nsIURIWithQuery.filePath and when nsIURI.path
> Could we do something to remove this error prone stuff?

I don't see how we would make this _less_ error prone. nsIURIWithQuery.filePath was previously nsIURL.filePath
All of the recent bugs we've had were people using nsIURI.path vs nsIURL.filePath that have existed for a while. These can be avoided if people read the documentation in the IDL files. Suggestions for making it less confusing are welcome.

(In reply to Andrea Marchesini [:baku] from comment #10)
> > As a side note, I'm considering replacing nsSimpleURI with RustURL.
> Do you think we can wait until this is done or we should propose a quicker
> fix for this issue?

This is neither a new issue, and I don't think it to be urgent.

> When do you think this RustURL replacement can happen?
I think bug 1324230 is the main blocker for this. Some of the others blocking bug 1318426 may also need to be fixed.

Regarding the comments about spec compliance, that's also a part of the RustURL project.

(In reply to Masatoshi Kimura [:emk] from comment #9)
> I did not say nsSimpleURI should implement nsIURL. I said custom protocol
> URIs should implement nsIURL.
> It would preserve the behavior for existing nsSimpleURI URLs (e.g. data:)
> and it would have even less web-compat issues.

I think we can definitely make the case for this - whether it is nsStandardURL or RustURL for custom protocols.
If this is urgent, we could go for nsStandardURL, otherwise we can wait for RustURL to be ready for prime time.
I don't have the time to take this right now, but I can probably review the patches if anyone wants to do this.

Comment 14

[Masatoshi Kimura [:emk]]

Attachment: Bug 1323683 - Fold nsIURIWithQuery into nsIURI. .gosu

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

Comment 15

[Masatoshi Kimura [:emk]]

We should not impose another QI only to get the path part qithout query.
Although this is not super-urgent, I would not like to perpetuate nsIURIWithQuery. It should be removed before the next Aurora merge.
This patch does not fix the non-compliant behavior. I'll file a followup bug for the remaining problems.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96d3ed25d5dcc81f527c02d56a7f4f0ac09c943a

Comment 16

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

Comment on attachment 8822194 [details]
Bug 1323683 - Fold nsIURIWithQuery into nsIURI. .gosu

https://reviewboard.mozilla.org/r/101164/#review102002

I don't know if this really improves how error prone this API is, but overall the patch looks fine.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78e1c7ad7140
Fold nsIURIWithQuery into nsIURI. r=valentin.gosu

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78e1c7ad7140