1308507 - Remove all usages of nsIURL and NetworkHelper

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Ricky Chien [:rickychien] (inactive)]

There are helper functions like getUriHostPort or getUriNameWithQuery. Move them, and all code that uses nsIURL, to a helper module (url-utils.js). Then, de-chrome the implementation by using standard URL class instead of nsIURL.

Comment 1

[Ricky Chien [:rickychien] (inactive)]

These helper functions getUriHostPort and getUriNameWithQuery have been moved into request-utils.js, the only thing to do is to get rid of all XPCOM APIs.

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

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

Comment 3

[Ricky Chien [:rickychien] (inactive)]

There are APIs and comment revisions for request-utils.js.

Comment 4

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review90350

It would be nice to get rid of all usages of nsIURL and NetworkHelper in one sweep. Then, few more utility functions will be probably added to the request-utils.js file. What do you think?

::: devtools/client/netmonitor/request-utils.js:123
(Diff revision 1)
> - * @return string
> + * @return {string} unicode url.
>   */
> -exports.getUriNameWithQuery = function (url) {
> -  if (!(url instanceof Ci.nsIURL)) {
> -    url = NetworkHelper.nsIURL(url);
> -  }
> +exports.getUrlNameWithQuery = function (urlString) {
> +  let url = new URL(urlString);
> +  return decodeURIComponent(
> +    (url.pathname.replace(/\S*\//, "") || url.pathname || "/") + url.search);

nit: regexps like this are worth explaining in a comment. Maybe it's just me, but I don't remember all the metacharacters like \S.

::: devtools/client/netmonitor/request-utils.js
(Diff revision 1)
> -exports.getUriNameWithQuery = function (url) {
> -  if (!(url instanceof Ci.nsIURL)) {
> -    url = NetworkHelper.nsIURL(url);
> -  }
> +exports.getUrlNameWithQuery = function (urlString) {
> +  let url = new URL(urlString);
> +  return decodeURIComponent(
> +    (url.pathname.replace(/\S*\//, "") || url.pathname || "/") + url.search);
> -
> -  let name = NetworkHelper.convertToUnicode(

Are you sure you can remove all the convertToUnicode calls? Won't it break Unicode URLs? Is behavior of nsIURL and URL really different in how they deal with Unicode in URLs? Or is it a difference between decodeURIComponent and unescape? Is it a good idea to call decodeURIComponent on the whole URL? Won't it break something in the domain name, for example?

::: devtools/client/netmonitor/request-utils.js:175
(Diff revision 1)
>  exports.loadCauseString = function (causeType) {
> -  return LOAD_CAUSE_STRINGS[causeType] || "unknown";
> +  return LOAD_CAUSE_STRINGS[parseInt(causeType, 10)] || "unknown";
>  };

You're effectively hardcoding the values of the nsIContentPolicy constants here. This is ugly, likely to break and will get even worse when the range of values will get discontinous. For example, bug 1310493 will introduce a TYPE_PREFETCH constant that has value of 42 (after all the internal types).

Let's postpone this to bug 1308509, where it will be done the right way - by moving this mapping function to server.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review90350

> Are you sure you can remove all the convertToUnicode calls? Won't it break Unicode URLs? Is behavior of nsIURL and URL really different in how they deal with Unicode in URLs? Or is it a difference between decodeURIComponent and unescape? Is it a good idea to call decodeURIComponent on the whole URL? Won't it break something in the domain name, for example?

I'm trying to decouple NetworkHelper since it uses lots of XPCOM APIs such as the nsIURL convertToUnicode() [1]. Indeed, I'm not familiar with the implementaion of convertToUnicode() but I suspect that it's doing something like decodeURIComponent to convert %xx to real character.

I just eyes check the result and it looks good. However, I know it cannot cover all possibilities. I'm going to ask Honza perhaps he knows about this better than us.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIScriptableUnicodeConverter#ConvertToUnicode()

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review90350

Agree, I'll update my patch to remove all usages of nsIURL and NetworkHelper for netmonitor.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

(In reply to Ricky Chien [:rickychien] from comment #5)
> Comment on attachment 8807495 [details]
> Bug 1308507 - de-chrome request helper APIs
> 
> https://reviewboard.mozilla.org/r/90642/#review90350
> 
> > Are you sure you can remove all the convertToUnicode calls? Won't it break Unicode URLs? Is behavior of nsIURL and URL really different in how they deal with Unicode in URLs? Or is it a difference between decodeURIComponent and unescape? Is it a good idea to call decodeURIComponent on the whole URL? Won't it break something in the domain name, for example?
> 
> I'm trying to decouple NetworkHelper since it uses lots of XPCOM APIs such
> as the nsIURL convertToUnicode() [1]. Indeed, I'm not familiar with the
> implementaion of convertToUnicode() but I suspect that it's doing something
> like decodeURIComponent to convert %xx to real character.
> 
> I just eyes check the result and it looks good. However, I know it cannot
> cover all possibilities. I'm going to ask Honza perhaps he knows about this
> better than us.
> 
> [1]
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIScriptableUnicodeConverter#ConvertToUnicode()

Honza, I'm unsure how to replace convertToUnicode() with the standard APIs. What I did in patch just to replace it by decodeURIComponent() directly, but I'm afraid that it doesn't cover all the cases we want.

Do you have better solution for that?

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #7)
> (In reply to Ricky Chien [:rickychien] from comment #5)
> > Comment on attachment 8807495 [details]
> > Bug 1308507 - de-chrome request helper APIs
> > 
> > https://reviewboard.mozilla.org/r/90642/#review90350
> > 
> > > Are you sure you can remove all the convertToUnicode calls? Won't it break Unicode URLs? Is behavior of nsIURL and URL really different in how they deal with Unicode in URLs? Or is it a difference between decodeURIComponent and unescape? Is it a good idea to call decodeURIComponent on the whole URL? Won't it break something in the domain name, for example?
> > 
> > I'm trying to decouple NetworkHelper since it uses lots of XPCOM APIs such
> > as the nsIURL convertToUnicode() [1]. Indeed, I'm not familiar with the
> > implementaion of convertToUnicode() but I suspect that it's doing something
> > like decodeURIComponent to convert %xx to real character.
> > 
> > I just eyes check the result and it looks good. However, I know it cannot
> > cover all possibilities. I'm going to ask Honza perhaps he knows about this
> > better than us.
> > 
> > [1]
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIScriptableUnicodeConverter#ConvertToUnicode()
> 
> Honza, I'm unsure how to replace convertToUnicode() with the standard APIs.
> What I did in patch just to replace it by decodeURIComponent() directly, but
> I'm afraid that it doesn't cover all the cases we want.
Good question. I have asked on IRC yesterday and it looks like we (the Netmonitor.html project) is the first doing this. But, we might want to get some inspiration in existing libs for web-develoopers, what about: https://www.npmjs.com/package/iconv-js

Honza

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

In devtools.html, I needed something similar at the transport layer and used the "utf8.js" library:

https://github.com/joewalker/devtools.html/blob/cac414d8ea8361df1f988cb3ac609fae8fe480df/shared/transport/utf8.js

Not sure if it's the API you need, but worth checking out at least.

Comment 10

[Ricky Chien [:rickychien] (inactive)]

After asking platform engineer Thinker, he told me that because javascript processes string with unicode by default, so it makes sense that encodeURIComponent() will convert url (ASCII) string to unicode. As a result, encodeURIComponent() would be the good candidate to replace convertToUnicode().

I'll begin to convert the rest of the part to get rid of NetworkHelper soon.

Comment 11

[Jarda Snajdr [:jsnajdr]]

I did some research and this is what I found:

Our task is to take an escaped Unicode URL like this: http://example.org/%E2%98%A2

and display it as: http://example.org/☢ (you should see a 'radioactive' unicode char here)

This involves two steps:
1. Unescape the %E2%98%A2 part into three UTF-8 bytes
2. Convert the 3 UTF-8 bytes into one Unicode character (0x2622)

The current code does it with:
1. The unescape() JS function (marked as deprecated on MDN) produces the three UTF-8 bytes
2. An instance of nsIScriptableUnicodeConverter converts them to one Unicode char

There are two ways how to dechrome this:
1. Use decodeURI or decodeURIComponent functions that do both steps at once.
2. Use unescape() followed by a call to a TextDecoder instance.

decodeURI looks like the obvious option, but there is a twist. What about invalid input? Turns out that decodeURI is not very tolerant about it.

First, the escaped string can be invalid, like "%E" (there must be exactly two hex chars after the %)

unescape() recovers from this and returns "%E". decodeURI() doesn't recover and throws a "malformed URI sequence" exception.

Second, the UTF-8 bytes can be invalid. String "%E2%98" has two bytes, but they don't constitute a valid UTF-8 sequence. decodeURI() throws an exception on this input. On the other hand, TextDecoder.decode recovers and returns some best guess.

By the way, the currently used nsIScriptableUnicodeConverter also fails on invalid UTF-8 input. The current Netmonitor has a bug - if you do fetch("%E2%98") in the Console, Netmonitor will display an empty "File" column for this request.

So, if you decide to use decodeURI, handle the exceptions and recover from them properly.

Comment 12

[Jarda Snajdr [:jsnajdr]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #9)
> In devtools.html, I needed something similar at the transport layer and used
> the "utf8.js" library:
> 
> https://github.com/joewalker/devtools.html/blob/
> cac414d8ea8361df1f988cb3ac609fae8fe480df/shared/transport/utf8.js
> 
> Not sure if it's the API you need, but worth checking out at least.

There is a TextEncoder/TextDecoder web platform API available:

https://developer.mozilla.org/en-US/docs/Web/API/TextDecoder/TextDecoder

so polyfills like this shouldn't be needed.

Comment 13

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

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

Comment 14

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

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

Comment 15

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review93108

Very nice work! NetworkHelper and nsIURL are gone from the frontend. I just have a few nits about naming things etc.

::: devtools/client/netmonitor/custom-request-view.js:117
(Diff revision 3)
>     *
>     * @param object url
>     *        The URL to extract query string from.
>     */
>    updateCustomQuery: function (url) {
> -    let paramsArray = NetworkHelper.parseQueryString(
> +    const paramsArray = parseQueryString((new URL(url).search));

nit: create a helper function "getParsedUrlQuery" that takes a URL as argument.

::: devtools/client/netmonitor/request-utils.js:110
(Diff revision 3)
> + * If there is a malformed URI sequence, it returns input string.
> + *
> + * @param {string} url - a string
> + * @return {string} unicode string
> + */
> +function convertToUnicode(string) {

This function should have a better name: it's not a generic function for converting strings to unicode, but it works with URLs. What about "decodeUnicodeUrl"?

::: devtools/client/netmonitor/request-utils.js:163
(Diff revision 3)
> +  const baseName = getUrlBaseName(url);
> +  return convertToUnicode(baseName + (new URL(url)).search);

nit: It's not very nice that the URL is parsed twice in this function.

::: devtools/client/netmonitor/request-utils.js:193
(Diff revision 3)
> + * Parse a url's query string into its components
> + *
> + * @param {string} url - url string
> + * @return {array} array of query params { name, value }
> + */
> +function parseQueryString(url) {

The parameter for this function is not a URL - it's only the query string portion.

Comment 16

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review93108

> nit: create a helper function "getParsedUrlQuery" that takes a URL as argument.

I just reuse getUrlQuery() instead.

Comment 17

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

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

Comment 18

[Jarda Snajdr [:jsnajdr]]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

https://reviewboard.mozilla.org/r/90642/#review93448

Looks good, thanks for working on this!

In the try run, the browser_net_statistics-03.js has failed twice. Before landing, make sure it's really an unrelated intermittent.

Comment 19

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8807495 [details]
Bug 1308507 - Remove all usages of nsIURL and NetworkHelper

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

Comment 20

[Ricky Chien [:rickychien] (inactive)]

After retriggering browser_net_statistics-03.js many times, it looks like an irrelevant intermittent to me and this patch could be landed safely.

Attached my try run as well. I've re-triggered 6 times dt3 after the orange dt3. Everything looks great to me.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8baa03393d3e99a9ca52e10ac3bfa23b8eec69

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3478d56c55a2
Remove all usages of nsIURL and NetworkHelper r=jsnajdr

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3478d56c55a2

Comment 23

[areya]

https://hg.mozilla.org/mozilla-central/rev/3478d56c55a2

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Will ride the train