utilityOverlay's trimURL should use Services.uriFixup instead of manually reverse-engineering its effects and trying to determine what to do

RESOLVED FIXED in Firefox 36

Status

()

defect
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

unspecified
Firefox 36
Points:
2
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Assignee

Description

5 years ago
This will have a small perf implication because XPCOM (until we move uriFixup into JS...), but I don't believe this method is hot anyway, so I think we should do that rather than continue to risk making wrong decisions whenever we alter the fixup code.
Assignee

Comment 1

5 years ago
Does this look sane to you, Dão? Marco told me you'd better review this. :-)

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=adf2b8e83426
Attachment #8517451 - Flags: review?(dao)
Assignee

Updated

5 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee

Comment 2

5 years ago
Posted patch use uriFixup for trimURL, (obsolete) — Splinter Review
... and once more, bzexport messes with what I want to do and uploads the empty trypush...
Attachment #8517452 - Flags: review?(dao)
Assignee

Updated

5 years ago
Attachment #8517451 - Attachment is obsolete: true
Attachment #8517451 - Flags: review?(dao)
(In reply to :Gijs Kruitbosch from comment #0)
> This will have a small perf implication because XPCOM (until we move
> uriFixup into JS...),

Twice... once for makeURI and once for createFixupURI.

> but I don't believe this method is hot anyway,

It's called during pageload.

It sounds like this isn't urgent, so I think we should postpone this until uriFixup is implemented in JS and chrome and content are in separate processes.
Assignee

Comment 4

5 years ago
(In reply to Dão Gottwald [:dao] from comment #3)
> (In reply to :Gijs Kruitbosch from comment #0)
> > This will have a small perf implication because XPCOM (until we move
> > uriFixup into JS...),
> 
> Twice... once for makeURI and once for createFixupURI.

Yes. The only reason we need the makeURI call here is encodings; the URI creation process normalizes these, and so the result for mözilla or similar things doesn't match the input. It's possible to rearch this to mostly avoid that path by just checking against aURL first. I didn't think that was worth it.

> 
> > but I don't believe this method is hot anyway,
> 
> It's called during pageload.

I'm fairly sure that at this point, xpcom overhead is such that a single call to this method will make no discernible difference (ie on the order of less than 10ms, quite likely less than 1ms, obviously depending on hardware etc.). I don't think this is enough overhead to justify not doing it.

> 
> It sounds like this isn't urgent, so I think we should postpone this until
> uriFixup is implemented in JS

I could live with this, although I do think this is likely to come back to bite us sooner rather than later. I don't know how quickly uriFixup will move to JS, but it may still take months.

> and chrome and content are in separate
> processes.

This is at least a year off, if not more, AIUI. I don't think we should live with the technical debt that long.
Comment on attachment 8517452 [details] [diff] [review]
use uriFixup for trimURL,

(In reply to :Gijs Kruitbosch from comment #4)
> I'm fairly sure that at this point, xpcom overhead is such that a single
> call to this method will make no discernible difference (ie on the order of
> less than 10ms, quite likely less than 1ms, obviously depending on hardware
> etc.). I don't think this is enough overhead to justify not doing it.

I'm having a hard time justifying even a small overhead as long as it doesn't fix something that's clearly broken.

> > It sounds like this isn't urgent, so I think we should postpone this until
> > uriFixup is implemented in JS
> 
> I could live with this, although I do think this is likely to come back to
> bite us sooner rather than later.

Seems rather unlikely to me since we lived with the regular expression for years. But when it comes to that, we can always still decide to take this patch.
Attachment #8517452 - Flags: review?(dao)
Assignee

Comment 6

5 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> Comment on attachment 8517452 [details] [diff] [review]
> use uriFixup for trimURL,
> 
> (In reply to :Gijs Kruitbosch from comment #4)
> > I'm fairly sure that at this point, xpcom overhead is such that a single
> > call to this method will make no discernible difference (ie on the order of
> > less than 10ms, quite likely less than 1ms, obviously depending on hardware
> > etc.). I don't think this is enough overhead to justify not doing it.
> 
> I'm having a hard time justifying even a small overhead as long as it
> doesn't fix something that's clearly broken.

I think duplicating the same algorithm in different parts of the code base is pretty clearly broken. I'd prefer correct over a tiny bit of speed and sometimes making the wrong decision.

> > > It sounds like this isn't urgent, so I think we should postpone this until
> > > uriFixup is implemented in JS
> > 
> > I could live with this, although I do think this is likely to come back to
> > bite us sooner rather than later.
> 
> Seems rather unlikely to me since we lived with the regular expression for
> years.

And it was broken for a long time (also years)! And then we broke it some more for 33. There are still more changes to the url bar under consideration (like making a typed trailing slash make a difference in how we treat "foo" vs. "foo/", because other browsers do that and it's breaking people's muscle memory that we don't). I'd like to avoid accidentally breaking it again.

Anyway, I went ahead and measured this.

Measuring a 100-times repeated run of all the testVal tests (~35) in the browser_urlbarTrimURLs tests, it seems this change added less than 0.2ms per call to testVal (which implicitly, somewhere down the line, calls trimURL). And that's before bothering to optimize the existing patch any further. There's no disk IO involved here, so I don't think perf is really a very strong argument against this change. Yes, it's slightly slower than the 2 regex replaces that are there now, but by far not enough to be noticed.
Assignee: gijskruitbosch+bugs → nobody
Blocks: 1057531
Status: ASSIGNED → NEW
Assignee

Comment 7

5 years ago
Egh, I decided against unassigning after measuring.
Assignee: nobody → gijskruitbosch+bugs
No longer blocks: 1057531
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Attachment #8517452 - Flags: review?(dao)
Comment on attachment 8517452 [details] [diff] [review]
use uriFixup for trimURL,

>+  // We don't want to remove 'http://' if the host starts with "ftp\d*\." or contains "@"
>+  let httpURLWithFTPOrAtSignRegex = /^http:\/\/(?:ftp\d*\.|[^\/]*@)/i;

As I understand it, the URI fixup check should take care of this.

>+  } catch (ex) { /* Don't care if this fails */ }

Why not? Shouldn't we return url rather than urlWithoutProtocol if this fails?
Attachment #8517452 - Flags: review?(dao) → review-
Assignee

Comment 9

5 years ago
(In reply to Dão Gottwald [:dao] from comment #8)
> Comment on attachment 8517452 [details] [diff] [review]
> use uriFixup for trimURL,
> 
> >+  // We don't want to remove 'http://' if the host starts with "ftp\d*\." or contains "@"
> >+  let httpURLWithFTPOrAtSignRegex = /^http:\/\/(?:ftp\d*\.|[^\/]*@)/i;
> 
> As I understand it, the URI fixup check should take care of this.

ftp, yes, because the fixup service will give you ftp://ftp instead of http://ftp, and so they won't match, so we'll keep http that way.

usernames, no, because:

Services.uriFixup.createFixupURI("foo@www.mozilla.org", Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP | Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS).spec

returns the correct URL (with http) so that will allow the removal of the protocol to happen. I'll add a test for this (the current ones all specify passwords, too, which triggers different behaviour).

> >+  } catch (ex) { /* Don't care if this fails */ }
> 
> Why not? Shouldn't we return url rather than urlWithoutProtocol if this
> fails?

I don't think there are cases where these wouldn't be the same where the next check would (wrongly) let us return urlWithoutProtocol, but sure. (if you know of such cases, please let me know and I can add them to the test, too)
Assignee

Comment 10

5 years ago
Posted patch use uriFixup for trimURL, (obsolete) — Splinter Review
Attachment #8521634 - Flags: review?(dao)
Assignee

Updated

5 years ago
Attachment #8517452 - Attachment is obsolete: true
Assignee

Comment 11

5 years ago
Posted patch use uriFixup for trimURL, (obsolete) — Splinter Review
Now with fewer non-capturing groups.
Attachment #8521674 - Flags: review?(dao)
Assignee

Updated

5 years ago
Attachment #8521634 - Attachment is obsolete: true
Attachment #8521634 - Flags: review?(dao)
(In reply to :Gijs Kruitbosch from comment #9)
> usernames, no, because:
> 
> Services.uriFixup.createFixupURI("foo@www.mozilla.org",
> Services.uriFixup.FIXUP_FLAG_ALLOW_KEYWORD_LOOKUP |
> Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS).spec
> 
> returns the correct URL (with http) so that will allow the removal of the
> protocol to happen.

That's fine, as hitting enter in the location bar when foo@www.mozilla.org is displayed without the protocol will correctly restore http://. Where http:// must not be removed is http://user:pass@mozilla.org, and createFixupURI should take care of that too.
Attachment #8521674 - Flags: review?(dao) → review-
Assignee

Comment 13

5 years ago
Hmm, I thought leaving the protocol for urls of type foo@domain.org was also done to reduce confusion with e.g. email addresses and such, but OK.
Attachment #8522186 - Flags: review?(dao)
Assignee

Updated

5 years ago
Attachment #8521674 - Attachment is obsolete: true
Comment on attachment 8522186 [details] [diff] [review]
use uriFixup for trimURL,

>+  // remove http://
>+  let urlWithoutProtocol = url.substring("http://".length);

I'd prefer:

let urlWithoutProtocol = url.substring(7); // remove http://

>+  let expectedURLSpec = aURL;

let expectedURLSpec;
Attachment #8522186 - Flags: review?(dao) → review+
(In reply to Dão Gottwald [:dao] from comment #14)
> >+  // remove http://
> >+  let urlWithoutProtocol = url.substring("http://".length);
> 
> I'd prefer:
> 
> let urlWithoutProtocol = url.substring(7); // remove http://

or even:

  // remove single trailing slash for http/https/ftp URLs
  let url = aURL.replace(/^((?:http|https|ftp):\/\/[^/]+)\/$/, "$1");

  // remove http://
  if (!url.startsWith("http://")) {
    return url;
  }
  let urlWithoutProtocol = url.substring(7);
Assignee

Comment 17

5 years ago
Marco, can you add this to the spreadsheet as well, please?
Iteration: --- → 36.3
Points: --- → 2
Flags: qe-verify-
Flags: needinfo?(mmucci)
Flags: in-testsuite+
Flags: firefox-backlog+
Added to IT 36.3
Flags: needinfo?(mmucci)
https://hg.mozilla.org/mozilla-central/rev/5b4fe2faa70d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36

Updated

3 years ago
Depends on: 1254503
You need to log in before you can comment on or make changes to this bug.