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...

(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...

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.

(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.)

(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?

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.

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)

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

(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.

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

Depends on D168389

Depends on D168390

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

Depends on D168392

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

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

Depends on D168395

Depends on D168396

Depends on D168397

(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 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.

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

For more information, please visit auto_nag documentation.

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

For more information, please visit auto_nag documentation.

:-\

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

(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...

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

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