Closed Bug 1308507 Opened 3 years ago Closed 3 years ago

Remove all usages of nsIURL and NetworkHelper

Categories

(DevTools :: Netmonitor, defect, P1)

defect

Tracking

(firefox53 fixed)

RESOLVED FIXED
Firefox 53
Iteration:
53.1 - Nov 28
Tracking Status
firefox53 --- fixed

People

(Reporter: rickychien, Assigned: rickychien)

References

(Blocks 1 open bug)

Details

(Whiteboard: [netmonitor])

Attachments

(1 file)

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.
Whiteboard: [devtools-html]
Whiteboard: [netmonitor]
Flags: qe-verify-
Priority: -- → P2
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 14
Priority: P2 → P1
Summary: Move URL helper functions to their own module → de-chrome request helper APIs
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.
There are APIs and comment revisions for request-utils.js.
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.
Attachment #8807495 - Flags: review?(jsnajdr)
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 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.
(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?
Flags: needinfo?(odvarko)
Summary: de-chrome request helper APIs → Remove all usages of nsIURL and NetworkHelper
(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
Flags: needinfo?(odvarko)
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.
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.
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.
(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.
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
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.
Attachment #8807495 - Flags: review?(jsnajdr)
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 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.
Attachment #8807495 - Flags: review?(jsnajdr) → review+
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
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3478d56c55a2
Remove all usages of nsIURL and NetworkHelper r=jsnajdr
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3478d56c55a2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Duplicate of this bug: 1308508
Will ride the train
Depends on: 1363553
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.