1810141 - Loading URIs should be done with URI objects by default, rather than strings, to help avoid unnecessary parsing / fixup / allocations

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[:Gijs (he/him)]

See this profile from Florian in bug 1800596 comment 18.

I think one of the causes for this is that there is in fact no public DOM/docshell API that takes an nsIURI - both BrowsingContext and nsIWebNavigation implement this with string arguments.

This means that in practice, we end up doing a lot of repetitive conversions from string to URI, frequently via URIFixup which is even more expensive because it has to cope with arbitrary (non-URL) input.

I think we should add a URI-based alternative on BrowsingContext and nsIWebNavigation, deprecate the other APIs, and gradually switch consumers over.

Nika/Marco, does this sound reasonable, and do you have naming suggestions for the new API? I think ideally we'd move the old API to loadURIString or something, but that may be tricky...

Comment 1

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #0)

Nika/Marco, does this sound reasonable, and do you have naming suggestions for the new API? I think ideally we'd move the old API to loadURIString or something, but that may be tricky...

+needinfo this time...

Comment 2

[Marco Bonardo [:mak]]

Without a full analysis is not trivial to tell if loadURI should take a string or an nsIURI by default, I mean we don't want to end in the opposite situation, where the caller creates a useless nsIURI.
CreateFromLoadURIOptions seems to be wanting a string to be able to fix it up, I wonder if an nsIURI version would cut out many strings that today would be loaded even if they can't be made into nsIURI.
It's also true that from some time we thought about having the docshell stop fixing up things and let that work to the caller, that is what the urlbar is already doing afaict.

Comment 3

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #2)

Without a full analysis is not trivial to tell if loadURI should take a string or an nsIURI by default, I mean we don't want to end in the opposite situation, where the caller creates a useless nsIURI.
CreateFromLoadURIOptions seems to be wanting a string to be able to fix it up, I wonder if an nsIURI version would cut out many strings that today would be loaded even if they can't be made into nsIURI.
It's also true that from some time we thought about having the docshell stop fixing up things and let that work to the caller, that is what the urlbar is already doing afaict.

Right, to be clear, I would still want to do the same level of fixup, not just break places that currently pass strings and expect fixup. I would just want to stop doing the fixup/conversion multiple times - when the address bar and other places already figure out a full URI (also for security checks etc. that happen before we attempt the load / opening new tab / etc.!) then we might as well pass that full URI.

I think in the end we'll not have that many places left that want to be able to pass a string (or at least we can do the conversion earlier in the process, either URL bar or context menu code etc.)

Comment 4

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #2)

Without a full analysis is not trivial to tell if loadURI should take a string or an nsIURI by default, I mean we don't want to end in the opposite situation, where the caller creates a useless nsIURI.

Actually maybe I don't fully understand your point here - when would the URI be useless?

Comment 5

[Marco Bonardo [:mak]]

Sorry if I wasn't clear, I meant we could end up switching an API from a string to an nsIURI, and then find some cases where both the caller and the API just needed a string. Then we'd end up in the opposite situation to this bug.
It doesn't seem to be the case here based on a quick glance, but I can't tell 100% without checking every code path.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Adding a new API to CanonicalBrowsingContext which bypasses URIFixup wouldn't be very difficult to do, so that's definitely something we can add if it'd be useful.

With regards to the naming, technically we could perhaps even do it as an overload (at least for the method on CanonicalBrowsingContext - the one on nsIDocShell would be more difficult, but hopefully we aren't calling that much anymore), where you could pass either a string or a nsIURI, but that feels a bit error-prone, as some strings which parse as a nsIURI might want URIFixup (e.g. to fix a scheme typo, because of windows file paths, or other checks I skipped over in my quick skim), but it would be skipped.

Changing out calls to loadURI to instead call loadURIString feels a bit error-prone and tedious (a quick search in searchfox maxes out the number of possible results, and it seems like many go through other wrappers like browser.loadURI which would also need to be changed https://searchfox.org/mozilla-central/search?q=.loadURI&path=&case=true&regexp=false)

The easiest would probably be to add a new method for this purpose with a different name, like loadURINoSchemeFixup, which makes everything quite explicit, and then switch things over (though it'll probably never become the default method folks call in new code with a name like that :p)

Comment 7

[Marco Bonardo [:mak]]

So maybe change name of the old one to fixupAndLoadURI() and the new one will just be loadURI?

Comment 8

[:Gijs (he/him)]

(In reply to Marco Bonardo [:mak] from comment #7)

So maybe change name of the old one to fixupAndLoadURI() and the new one will just be loadURI?

Yeah... It'll be tedious and a little error prone but I'm hopeful we have enough test coverage. I think the maxing out of search results on searchfox is because of BrowserTestUtils.loadURI which we could keep as-is for brevity's sake. From a quick look I'd estimate there are <100 actual JS calls to loadURI methods on browsers.

Comment 9

[:Gijs (he/him)]

https://treeherder.mozilla.org/jobs?repo=try&revision=bdc5c0eb1ee508fb9245c4152e9946f010704518

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1810141 - make browsingcontext's loadURI actually take a URI rather than a string, r?nika

Comment 11

[:Gijs (he/him)]

Attachment: Bug 1810141 - update consumers of CanonicalBrowsingContext.loadURI to use fixup or pass a URI object if they have it, r?#webdriver-reviewers!,mak!

Depends on D168389

Comment 12

[:Gijs (he/him)]

Attachment: Bug 1810141 - make docshell/nsIWebNavigation's loadURI actually take a URI object rather than a string, r?nika

Depends on D168390

Comment 13

[:Gijs (he/him)]

Attachment: Bug 1810141 - fix tabbrowser.js and browser.js loadURI to match new webnavigation interfaces, r?dao,mak

This moves the somewhat out-of-place _loadURI method and its dependencies
into tabbrowser, and deals with receiving either a string or a URI.

Depends on D168391

Comment 14

[:Gijs (he/him)]

Attachment: Bug 1810141 - fix replaceUrlInTab webextension utility to deal with loadURI changes, r?rpl

Depends on D168392

Comment 15

[:Gijs (he/him)]

Attachment: Bug 1810141 - fix loadURI callers that already have an nsIURI reference or where creating one is clearly safe, r?Mossop

There are 3 types of changes in this commit:

• from loadURI(foo.spec) to loadURI(foo), as there's already a URI
• from loadURI("string") to loadURI(Services.io.newURI("string")) as the URL is hardcoded
• one or two where there is perhaps an intermediate variable but the patch
  context should still make it trivial to ascertain the change is correct.

Depends on D168393

Comment 16

[:Gijs (he/him)]

Attachment: Bug 1810141 - switch consumers where there isn't an obvious URI object to use to fixupAndLoadURIString, r?Mossop

This also updates layoutdebug.js, which in some ways pretends to be like a browser window but
is its own special snowflake. I kept the method naming conventions similar to the main
browser window.

Depends on D168394

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1810141 - fix pageloader.js to just load URIs,r?#perftest-reviewers

Depends on D168395

Comment 18

[:Gijs (he/him)]

Attachment: Bug 1810141 - fix tests to deal with changes to loadURI, r?Mossop

Depends on D168396

Comment 19

[:Gijs (he/him)]

Attachment: Bug 1810141 - remove superfluous extra global loadURI method from browser.js, r?Mossop

Depends on D168397

Comment 20

[:Gijs (he/him)]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #6)

The easiest would probably be to add a new method for this purpose with a different name, like loadURINoSchemeFixup, which makes everything quite explicit, and then switch things over (though it'll probably never become the default method folks call in new code with a name like that :p)

So I opted for something like this, but to avoid the "what should be the default" issue, I renamed the original to fixupAndLoadURI and made loadURI actually load a URI object, which hopefully makes it more obvious that that's what people should use unless there's a good reason not to.

My hope is that we can gradually move more callsites to use loadURI directly - I didn't change all the callsites in this bug because it's not always entirely trivial to work out if this is safe for the reasons you outlined in your comment and because of indirection on the JS side. Still, I'm fairly sure that most callsites do in fact have a URI object, and the common path of going through browser.loadURI was already doing fixup on the JS side, so can pass a URI if it finds one. That still ends up saving an extra roundtrip through fixup, so already helps with the original aim of this bug of reducing the number of times we pass through fixup / URL parsing code, but there's more work to be done. I'll adjust the summary to indicate this.

Once this lands I intend to audit the uses of fixupAndLoadURI and file follow-up bugs to switch them over.

Comment 21

[Phabricator Automation]

Comment on attachment 9315123 [details]
Bug 1810141 - remove superfluous extra global loadURI method from browser.js, r?Mossop

Revision D168398 was moved to bug 1815439. Setting attachment 9315123 [details] to obsolete.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 24

[:Gijs (he/him)]

:-\

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/569de94781e4
make browsingcontext's loadURI actually take a URI rather than a string, r=nika
https://hg.mozilla.org/integration/autoland/rev/50b57ba1a061
update consumers of CanonicalBrowsingContext.loadURI to use fixup or pass a URI object if they have it, r=mak,webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/4dc41d90dbb3
make docshell/nsIWebNavigation's loadURI actually take a URI object rather than a string, r=nika
https://hg.mozilla.org/integration/autoland/rev/dce3aa683445
fix tabbrowser.js and browser.js loadURI to match new webnavigation interfaces, r=dao
https://hg.mozilla.org/integration/autoland/rev/ab5d76846e10
fix replaceUrlInTab webextension utility to deal with loadURI changes, r=rpl
https://hg.mozilla.org/integration/autoland/rev/118f131a524a
fix loadURI callers that already have an nsIURI reference or where creating one is clearly safe, r=mossop,geckoview-reviewers,extension-reviewers,settings-reviewers,mconley,m_kato
https://hg.mozilla.org/integration/autoland/rev/3852fbe290f4
switch consumers where there isn't an obvious URI object to use to fixupAndLoadURIString, r=mossop,geckoview-reviewers,extension-reviewers,settings-reviewers,mconley,owlish
https://hg.mozilla.org/integration/autoland/rev/131037295784
fix pageloader.js to just load URIs,r=perftest-reviewers,mossop,sparky
https://hg.mozilla.org/integration/autoland/rev/8781a0d1254d
fix tests to deal with changes to loadURI, r=mossop,perftest-reviewers,geckoview-reviewers,extension-reviewers,sparky,owlish

Comment 26

[Cristina Horotan [:chorotan]]

Backed out 9 changesets (bug 1810141) for several test failures

Backout: https://hg.mozilla.org/integration/autoland/rev/cfdad8e969244c0b01e55305454ed7dde1ce10b2

Failure push: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=8781a0d1254da7ecb61bbc7e15b10ab0a3ae805e&selectedTaskRun=efRGDA5sTBG4RalDjMx_4g.0

Failure logs:

bc failure log: https://treeherder.mozilla.org/logviewer?job_id=405435987&repo=autoland&lineNumber=7928
reftest failure log: https://treeherder.mozilla.org/jobs?repo=autoland&revision=8781a0d1254da7ecb61bbc7e15b10ab0a3ae805e&selectedTaskRun=bplBDOUmS6-7Ml1fzcjNOA.0
crashtest failure log: https://treeherder.mozilla.org/logviewer?job_id=405437101&repo=autoland&lineNumber=1823

Comment 27

[:Gijs (he/him)]

(In reply to Cristina Horotan [:chorotan] from comment #26)

Backed out 9 changesets (bug 1810141) for several test failures

Oops. Repushing with fixes for these...

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/860cc3d9d79d
make browsingcontext's loadURI actually take a URI rather than a string, r=nika
https://hg.mozilla.org/integration/autoland/rev/bcfc04099be6
update consumers of CanonicalBrowsingContext.loadURI to use fixup or pass a URI object if they have it, r=mak,webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/eda565b7f771
make docshell/nsIWebNavigation's loadURI actually take a URI object rather than a string, r=nika
https://hg.mozilla.org/integration/autoland/rev/281c4d9b53f9
fix tabbrowser.js and browser.js loadURI to match new webnavigation interfaces, r=dao
https://hg.mozilla.org/integration/autoland/rev/5a4fd4773ff4
fix replaceUrlInTab webextension utility to deal with loadURI changes, r=rpl
https://hg.mozilla.org/integration/autoland/rev/2e061b22d800
fix loadURI callers that already have an nsIURI reference or where creating one is clearly safe, r=mossop,geckoview-reviewers,extension-reviewers,settings-reviewers,mconley,m_kato
https://hg.mozilla.org/integration/autoland/rev/a2c5de7a14b7
switch consumers where there isn't an obvious URI object to use to fixupAndLoadURIString, r=mossop,geckoview-reviewers,extension-reviewers,settings-reviewers,mconley,owlish
https://hg.mozilla.org/integration/autoland/rev/9f7375aa6f76
fix pageloader.js to just load URIs,r=perftest-reviewers,mossop,sparky
https://hg.mozilla.org/integration/autoland/rev/1fe10fe2909d
fix tests to deal with changes to loadURI, r=mossop,perftest-reviewers,geckoview-reviewers,extension-reviewers,sparky,owlish

Comment 29

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/860cc3d9d79d
https://hg.mozilla.org/mozilla-central/rev/bcfc04099be6
https://hg.mozilla.org/mozilla-central/rev/eda565b7f771
https://hg.mozilla.org/mozilla-central/rev/281c4d9b53f9
https://hg.mozilla.org/mozilla-central/rev/5a4fd4773ff4
https://hg.mozilla.org/mozilla-central/rev/2e061b22d800
https://hg.mozilla.org/mozilla-central/rev/a2c5de7a14b7
https://hg.mozilla.org/mozilla-central/rev/9f7375aa6f76
https://hg.mozilla.org/mozilla-central/rev/1fe10fe2909d

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit auto_nag documentation.