Issue 1801 - Use URL objects to process URLs in the background page

Currently we pass URL strings around, using regular expressions (e.g. to check for the protocol) or parsing them every time we need to extract the domain. In regard to correct handling of IDN domains (currently most of the code ignores them, for more details read the issue) things become even less optimal with the current approach.

With this patch, URLs are parsed immediately, and are stored/passed as URL object. Therefore I removed lib/basedomain.js, and moved/added helper functions to lib/url.js, which operate on URL objects and handle IDN domains correctly.

[kzar, 2015-01-26 10:27:38]

I like these changes, mostly seems to make the code easier to read :) Can't see
anything obviously wrong, I think maybe Wladimir should double check though.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:76: for (; frame; frame = frame.parent)
Took me a second to grok this loop, wouldn't a while loop be clearer here?

while (frame)
{
  if (frame.url)
  {
    let hostname = getDecodedHostname(frame.url);
    if (hostname)
      return hostname;
  }

  frame = frame.parent;
}

return "";

[Sebastian Noack, 2015-01-26 10:38:39]

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:76: for (; frame; frame = frame.parent)
On 2015/01/26 10:27:38, kzar wrote:
> Took me a second to grok this loop, wouldn't a while loop be clearer here?
> 
> while (frame)
> {
>   if (frame.url)
>   {
>     let hostname = getDecodedHostname(frame.url);
>     if (hostname)
>       return hostname;
>   }
> 
>   frame = frame.parent;
> }
> 
> return "";

I think this a typical case for-lops exist for, to check for a condition and
advance the state in the header. This isn't necessarily limited to counters.

[kzar, 2015-01-26 17:19:05]

Again, think Wladimir should check this (and also comment on the loop) but LGTM.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:76: for (; frame; frame = frame.parent)
On 2015/01/26 10:38:39, Sebastian Noack wrote:
> On 2015/01/26 10:27:38, kzar wrote:
> > Took me a second to grok this loop, wouldn't a while loop be clearer here?
> > 
> > while (frame)
> > {
> >   if (frame.url)
> >   {
> >     let hostname = getDecodedHostname(frame.url);
> >     if (hostname)
> >       return hostname;
> >   }
> > 
> >   frame = frame.parent;
> > }
> > 
> > return "";
> 
> I think this a typical case for-lops exist for, to check for a condition and
> advance the state in the header. This isn't necessarily limited to counters.

Hmm well I guess it's subjective which form is easier to read. I'll leave the
deciding vote to Wladimir :D

[Sebastian Noack, 2015-01-26 17:26:30]

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:76: for (; frame; frame = frame.parent)
On 2015/01/26 17:19:05, kzar wrote:
> On 2015/01/26 10:38:39, Sebastian Noack wrote:
> > On 2015/01/26 10:27:38, kzar wrote:
> > > Took me a second to grok this loop, wouldn't a while loop be clearer here?
> > > 
> > > while (frame)
> > > {
> > >   if (frame.url)
> > >   {
> > >     let hostname = getDecodedHostname(frame.url);
> > >     if (hostname)
> > >       return hostname;
> > >   }
> > > 
> > >   frame = frame.parent;
> > > }
> > > 
> > > return "";
> > 
> > I think this a typical case for-lops exist for, to check for a condition and
> > advance the state in the header. This isn't necessarily limited to counters.
> 
> Hmm well I guess it's subjective which form is easier to read. I'll leave the
> deciding vote to Wladimir :D

As long as you don't consider for-loops generally less readable than while-loops
like below, there is objectively no reason why you shouldn't use for-loops here.

var i = 0;
while (i < 10)
{
  ...
  i++;
}

[Wladimir Palant, 2015-02-09 12:54:29]

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chro...
chrome/ext/background.js:32: Object.defineProperty(this, "url", {value: new
URL(tab.url), enumerable: true});
Code that belongs together should really be together. Please move that logic
back into the getter - with caching if you prefer, but I don't really see
caching being necessary here.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:109: return /[^\d.]/.test(hostname) && hostname.indexOf(":") == -1;
new URL("http://0x5bec7a38/").hostname - gives you "0x5bec7a38" in Safari.
Chrome normalizes IPv4 addresses however. There is also
"http://0x5b.236.122.56/" by the way.

Also, please add a comment - it's not obvious what you are testing here.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/whitelisting.js:20: let {stringifyURL, getDecodedHostname, isThirdParty} =
require("url");
Shouldn't this import extractHostFromFrame as well?

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/whitelisting.js:29: getDecodedHostname(url), false
Please provide a sitekey parameter (null).

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
File qunit/tests/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:34: equal(getDecodedHostname(new
URL("http://example.com/:8080")), "example.com", "with port");
Port number before slash please.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:38: equal(getDecodedHostname(new URL("data:text/plain,foo")),
"", "data: URL");
Could you add back tests that you removed? In particular, I would consider ftp
URL and IP addresses important.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:79: equal(isThirdParty(new URL("http://foo.example.com/"),
"bar.example.com"), false, "same basedomain (example.com) isn't third-party");
Please add back http://foo.com/, www.foo.com scenario - domain + subdomain is
different from the case of two subdomains.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:86: });
The original unit tests had a bunch more scenarios:

* IPv6 addresses (also dotted-decimal inside IPv6).
* Hexadecimal encoding in IPv4 addresses.
* "Invalid" IP addresses (e.g. 0xab.example.com).
* Trailing dot.

If you don't want to test base domain extraction directly, you need to cover
these cases here.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:87: })();
Nit: there is a reason why the original unit tests had lists with test data - to
avoid the boilerplace. E.g. equal(isThirdParty(new URL())) is all boilerplace
and obscures the parameters that are changing here. Up to you whether you want
to change that.

[Sebastian Noack, 2015-02-11 10:55:51]

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/chro...
chrome/ext/background.js:32: Object.defineProperty(this, "url", {value: new
URL(tab.url), enumerable: true});
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Code that belongs together should really be together. Please move that logic
> back into the getter - with caching if you prefer, but I don't really see
> caching being necessary here.

Whatever.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:109: return /[^\d.]/.test(hostname) && hostname.indexOf(":") == -1;
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> new URL("http://0x5bec7a38/").hostname - gives you "0x5bec7a38" in Safari.
> Chrome normalizes IPv4 addresses however. There is also
> "http://0x5b.236.122.56/" by the way.
> 
> Also, please add a comment - it's not obvious what you are testing here.

Done.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/whitelisting.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/whitelisting.js:20: let {stringifyURL, getDecodedHostname, isThirdParty} =
require("url");
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Shouldn't this import extractHostFromFrame as well?

You are right. It works though, since extractHostFromFrame is imported as a
global variable in background.js.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/whitelisting.js:29: getDecodedHostname(url), false
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Please provide a sitekey parameter (null).

Done.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
File qunit/tests/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:34: equal(getDecodedHostname(new
URL("http://example.com/:8080")), "example.com", "with port");
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Port number before slash please.

Done.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:38: equal(getDecodedHostname(new URL("data:text/plain,foo")),
"", "data: URL");
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Could you add back tests that you removed? In particular, I would consider ftp
> URL and IP addresses important.

Not really removed them, since the function we test here is new. But I agree,
there are some cases that previously were tested for extractHostFromURL() that
makes sense to cover here as well.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:79: equal(isThirdParty(new URL("http://foo.example.com/"),
"bar.example.com"), false, "same basedomain (example.com) isn't third-party");
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Please add back http://foo.com/, http://www.foo.com scenario - domain +
subdomain is
> different from the case of two subdomains.

Done.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:86: });
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> The original unit tests had a bunch more scenarios:
> 
> * IPv6 addresses (also dotted-decimal inside IPv6).
> * Hexadecimal encoding in IPv4 addresses.
> * "Invalid" IP addresses (e.g. http://0xab.example.com).
> * Trailing dot.
> 
> If you don't want to test base domain extraction directly, you need to cover
> these cases here.

Done.

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/quni...
qunit/tests/url.js:87: })();
On 2015/02/09 12:54:29, Wladimir Palant wrote:
> Nit: there is a reason why the original unit tests had lists with test data -
to
> avoid the boilerplace. E.g. equal(isThirdParty(new URL())) is all boilerplace
> and obscures the parameters that are changing here. Up to you whether you want
> to change that.

The reason I didn't like that approach was that it made the test code rather
obscure IMO. Since it used a multidimensional array, it wasn't obvious which
item of the inner arrays were used how. Also the old tests didn't bother to use
meaningful messages documenting what we actually protecting against. I think a
better approach to eliminate duplication here would be to move the common
boilerplate into a function.

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
chrome/ext/background.js:38: // but sometimes we only have the tab id when we
create a Page object.
All changes below are due to rebase.

[Wladimir Palant, 2015-02-11 13:45:45]

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5718998062727168/lib/...
lib/url.js:76: for (; frame; frame = frame.parent)
On 2015/01/26 17:19:05, kzar wrote:
> Hmm well I guess it's subjective which form is easier to read. I'll leave the
> deciding vote to Wladimir :D

I think I wrote the for loop in the original code, it went exactly like this one
:)

In general, I do like using for loops wherever it makes sense - meaning that the
increment doesn't have to be trivial. I would probably have concerns if it were
some complicated construct, but this one isn't.

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
chrome/ext/background.js:33: // usually our Page objects are created from
Chrome's Tab objects, which
Nit: A sentence is usually started by a capital letter (here and in the comment
below).

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/lib/...
lib/url.js:110: if (/^((0x[\da-f]+|\d+)(\.|$)){4}$/i.test(hostname))
This won't accept "0x5bec7a38" - should be {1,4} I guess? Or maybe just keep the
number of octets arbitrary, for simplicity? Meaning:
/^((0x[\da-f]+|\d+)(\.|$))+$/i

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/quni...
File qunit/tests/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/quni...
qunit/tests/url.js:125: testThirdParty("1.0xff.1.1", "2.0xff.1.1", true,
"different IPv4 address with hexadecimal octet is third-party");
I guess we should test "0x01ff0101" as well.

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/quni...
qunit/tests/url.js:132: testThirdParty("example.com..", "example.com....",
false, "traling dots are ignored");
Interestingly, all browsers will indeed accept an arbitrary number of dots even
though DNS resolution only allows one.

[Sebastian Noack, 2015-02-11 17:07:06]

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
File chrome/ext/background.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/chro...
chrome/ext/background.js:33: // usually our Page objects are created from
Chrome's Tab objects, which
On 2015/02/11 13:45:45, Wladimir Palant wrote:
> Nit: A sentence is usually started by a capital letter (here and in the
comment
> below).

Note that I just reverted those lines including the comments to as their current
version. So those comments aren't matter of this change anymore. I generally
agree, but it's probably not worth an unrelated change here. ;)

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/lib/...
File lib/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/lib/...
lib/url.js:110: if (/^((0x[\da-f]+|\d+)(\.|$)){4}$/i.test(hostname))
On 2015/02/11 13:45:45, Wladimir Palant wrote:
> This won't accept "0x5bec7a38" - should be {1,4} I guess? Or maybe just keep
the
> number of octets arbitrary, for simplicity? Meaning:
> /^((0x[\da-f]+|\d+)(\.|$))+$/i

Even better; arbitrary and optional. So we also bail out if there is no
hostname, like for about:blank URLs.

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/quni...
File qunit/tests/url.js (right):

http://codereview.adblockplus.org/5564089086509056/diff/5704837555552256/quni...
qunit/tests/url.js:125: testThirdParty("1.0xff.1.1", "2.0xff.1.1", true,
"different IPv4 address with hexadecimal octet is third-party");
On 2015/02/11 13:45:45, Wladimir Palant wrote:
> I guess we should test "0x01ff0101" as well.

Done.

[Wladimir Palant, 2015-02-11 18:33:11]

LGTM