1876483 - Remove nsIURIMutator.setSpec in favor of NS_NewURI or Services.io.newURI

Status: NEW

(Core :: Networking, task, P2)

Description

[Valentin Gosu [:valentin]]

Since we made NS_NewURI be the entry point for all URL parsing we don't really need SetSpec anymore.
SetSpec has the downside that it allows you to set the spec of a URI implementation it wasn't really meant to handle - for example: creating a data URL and calling setSpec with a HTTP scheme will lead to weird behaviours.
The best thing would be to remove setSpec entirely, and move all of the consumers to use NS_NewURI or Service.io.newURI
All setSpec( instances
If it's not possible to remove SetSpec entirely, we should at least add a linter to make sure it's not used outside NS_NewURI

Comment 1

[Valentin Gosu [:valentin]]

First step is to remove all setSpec instances outside nsNetUtil
That should make things safer already.

Comment 2

[ISHIKAWA, Chiaki]

C-C TB calls setSpec() in a not so few places:
https://searchfox.org/comm-central/search?q=setSpec%28&path=&case=false&regexp=false

But I think it would be good considering setSpec() is called for many invalid URIs and fail.
The following is from local C-C TB xpcshell and mochitest.
I recorded these errors and summarized during the local test runs of C-C TB debug version.

I am told "Local Folders" with a space in it is an invalid URL to begin with. There are some dormant bugs in C-C TB.

From local xpcshell test of DEBUG version of C-C TB:
The number at the beginning of each message shows how many times it is recorded.

========================================
SetSpec Failures/Successes
========================================

Failure first.

   3103 SetSpec failed. : aSpec=imap://Local Folders
    572 SetSpec failed. : aSpec=imap://Local Folder
    201 SetSpec failed. : aSpec=imap://smart mailboxes
      5 SetSpec failed. : aSpec=imap://Smart Mailboxes
      3 SetSpec failed. : aSpec=document.getElementById(
      1 SetSpec failed. : aSpec=imap://Local Folders-1

The following is from local mochitest of DEBUG version of C-C TB:

========================================
SetSpec Failures/Successes
========================================

Failure first.

   2154 SetSpec failed. : aSpec=imap://Local Folders
   2009 SetSpec failed. : aSpec=toolkit/global/arrowscrollbox.ftl
   1663 SetSpec failed. : aSpec=toolkit/global/textActions.ftl
   1276 SetSpec failed. : aSpec=toolkit/main-window/findbar.ftl
   1276 SetSpec failed. : aSpec=messenger/messenger.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/openpgp.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/openpgp-frontend.ftl
   1156 SetSpec failed. : aSpec=messenger/openpgp/msgReadStatus.ftl
   1156 SetSpec failed. : aSpec=messenger/messageheader/headerFields.ftl
   1156 SetSpec failed. : aSpec=calendar/calendar-invitation-panel.ftl
    734 SetSpec failed. : aSpec=messenger/preferences/preferences.ftl
    516 SetSpec failed. : aSpec=messenger/treeView.ftl
    501 SetSpec failed. : aSpec=branding/brand.ftl
    402 SetSpec failed. : aSpec=messenger/addressbook/vcard.ftl
    402 SetSpec failed. : aSpec=messenger/addressbook/aboutAddressBook.ftl
    396 SetSpec failed. : aSpec=messenger/appmenu.ftl
    338 SetSpec failed. : aSpec=toolkit/updates/history.ftl
    338 SetSpec failed. : aSpec=toolkit/about/config.ftl
    338 SetSpec failed. : aSpec=security/certificates/deviceManager.ftl
    338 SetSpec failed. : aSpec=security/certificates/certManager.ftl
    338 SetSpec failed. : aSpec=messenger/syncAccounts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/system-integration.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/sync-dialog.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/receipts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/permissions.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/passwordManager.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/offline.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/notifications.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/new-tag.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/languages.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/fonts.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/dock-options.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/cookies.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/connection.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/colors.ftl
    338 SetSpec failed. : aSpec=messenger/preferences/attachment-reminder.ftl
    338 SetSpec failed. : aSpec=messenger/aboutDialog.ftl
    338 SetSpec failed. : aSpec=calendar/preferences.ftl
    338 SetSpec failed. : aSpec=calendar/category-dialog.ftl
    250 SetSpec failed. : aSpec=imap://Reply Identity Testing
    145 SetSpec failed. : aSpec=imap://smart mailboxes
    120 SetSpec failed. : aSpec=messenger/about3Pane.ftl
     73 SetSpec failed. : aSpec=toolkit/global/mozMessageBar.ftl
     67 SetSpec failed. : aSpec=messenger/otr/chat.ftl
     48 SetSpec failed. : aSpec=toolkit/neterror/netError.ftl
     48 SetSpec failed. : aSpec=toolkit/neterror/certError.ftl
     48 SetSpec failed. : aSpec=toolkit/global/mozSupportLink.ftl
     48 SetSpec failed. : aSpec=toolkit/global/mozFiveStar.ftl
     48 SetSpec failed. : aSpec=toolkit/about/aboutAddons.ftl
     48 SetSpec failed. : aSpec=messenger/extensionPermissions.ftl
     48 SetSpec failed. : aSpec=messenger/aboutAddonsExtra.ftl
     48 SetSpec failed. : aSpec=imap://Test Local Folders
     36 SetSpec failed. : aSpec=toolkit/global/resetProfile.ftl
     36 SetSpec failed. : aSpec=toolkit/global/processTypes.ftl
     36 SetSpec failed. : aSpec=toolkit/about/aboutSupport.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportMail.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportChat.ftl
     36 SetSpec failed. : aSpec=messenger/aboutSupportCalendar.ftl
     26 SetSpec failed. : aSpec=toolkit/global/notification.ftl
     22 SetSpec failed. : aSpec=messenger/aboutImport.ftl
     19 SetSpec failed. : aSpec=imap://Redirect Addresses Testing
     19 SetSpec failed. : aSpec=imap://BCC Reply Testing
     17 SetSpec failed. : aSpec=moz-icon://https://www.mozilla.org/?size=16&contentType=
     12 SetSpec failed. : aSpec=undefined
     12 SetSpec failed. : aSpec=DTD/xhtml1-strict.dtd
      8 SetSpec failed. : aSpec=calendar/calendar-widgets.ftl
      6 SetSpec failed. : aSpec=messenger/accountManager.ftl
      4 SetSpec failed. : aSpec=imap://New Msg Compose Identity Testing
      4 SetSpec failed. : aSpec=imap://Draft Identity Testing
      3 SetSpec failed. : aSpec=toolkit/about/aboutProfiles.ftl
      3 SetSpec failed. : aSpec=
      2 SetSpec failed. : aSpec=notarealaddress
      2 SetSpec failed. : aSpec=moz-icon://chrome://calendar/content/sound.wav?size=16
      2 SetSpec failed. : aSpec=moz-icon://
      1 SetSpec failed. : aSpec=null

But C-C TB may also need to clean up some dormant issues which are now uncovered and have come to the front in
Bug 1906992 .
We may need to cope with these issues in the next few weeks in C-C TB, I think.

Comment 3

[edgul]

Note: valentin may not have time to work on this. Someone can feel free to pick it up.

Comment 4

[Hackbot]

Attachment: Bug 1876483 - Stop calling nsIURIMutator.setSpec outside necko, and add a lint to prevent new callers. r=necko-reviewers

setSpec() lets a caller hand a spec to a URI implementation that was never
meant to parse it (the classic example from comment 0: create a data: URI,
then setSpec an http:// spec on it). Since NS_NewURI() became the single entry
point for URL parsing, no code outside necko needs setSpec at all.

This does the first step from comment 1 - remove all setSpec callers outside
nsNetUtil - and, since setSpec cannot be removed outright (the per-scheme
NewURI implementations reached from NS_NewURI still need it), it also adds
the linter comment 0 asks for as the fallback.

Converted callers:

• netwerk/cache2/Dictionary.cpp - the Exists() probe forced prepath into an
  nsStandardURL, while the sibling read/create path hands the very same string
  to CacheStorage::AsyncOpenURIString(), which already NS_NewURI()s it. Now both
  use the same parser. Compression dictionaries are HTTPS-only on both the read
  (nsHttpHandler::AddAcceptAndDictionaryHeaders, gated on IsHTTPS()) and write
  (nsHttpChannel::ParseDictionary) paths, and prepath is always a normalized
  GetPrePath() string, so the cache entry key (GetAsciiSpec of the ref-stripped
  URI) is byte-identical.

• layout/generic/nsImageFrame.cpp - ismap coordinate appending.

• toolkit/components/places/History.cpp - place.spec is just
  visitedURI->GetSpec() and visitedURI is still in scope, so drop the
  round-trip entirely and pass the URI we already have. Only
  triggeringSponsoredURL (a string from a browser element attribute) still needs
  parsing, and that now goes through NS_NewURI. Note BaseHistory::CanStore is a
  blocklist, so non-http(s) schemes do reach this code; NS_NewURI dispatches them
  to the right implementation instead of forcing a standard-URL parse.

• dom/network/TCPSocket.cpp, dom/media/webrtc/transport/ipc/WebrtcTCPSocket.cpp -
  NS_NewURI() for the spec, then mutate only the port.

• dom/quota/ActorsParent.cpp - EnsureStorageOriginFromOrigin() built a standard
  URL from the origin and then overwrote its scheme, host and port, so nothing of
  the origin survived into the result. Synthesize uuid://<uuid> directly. The
  PopulateFromOrigin() call is kept for its validation side effect.

• mobile/android/.../GeckoViewContentChannelChild.cpp - the parent has already
  parsed the URI and sends it over IPC (non-nullable, and the parent rejects null
  before sending), so adopt it rather than re-parsing its spec.

• browser/components/downloads/DownloadsCommon.sys.mjs and five
  dom/serializers mochitests - Services.io.newURI().

Deliberately not converted, and excluded from the lint:

• nsNetUtil.cpp and the mutator machinery in nsIURIMutator.idl - the one place
  setSpec is meant to be called from.
• The per-scheme construction reached from NS_NewURI (nsDataHandler,
  nsAboutProtocolHandler, nsViewSourceHandler, BlobURLProtocolHandler,
  nsJSProtocolHandler) - these are the parsing entry points, so they cannot go
  through NS_NewURI.
• caps/NullPrincipal.cpp - picks its URI implementation from a pref and chains
  setSpec with setQuery; it also has to work in process types NS_NewURI asserts
  against.
• SMILTimedElement.cpp - unrelated API that happens to be spelled SetSpec().
• Tests that deliberately exercise setSpec or a specific URI implementation.

Two intentional behaviour deltas worth a look:

1. TCPSocket/WebrtcTCPSocket: NS_NewURI() gives the URI its scheme's default
   port, so a requested port of 80/443 is now normalized away (spec
   https://host/, port -1) instead of being stored explicitly
   (https://host:443/). nsHttpConnectionInfo::Init does
   port == -1 ? DefaultPort() : port, so the effective port - and therefore
   proxy resolution and CONNECT tunnelling - is unchanged; the Host header just
   loses a redundant default port, which is the standards-correct form.
   Non-default ports are byte-identical. Verified with a throwaway xpcshell
   comparison across http/https/IPv6-literal and default/non-default ports.

2. Where a spec previously got force-parsed as a standard URL it is now
   dispatched by scheme, which for exotic schemes can now fail where the forced
   parse silently produced a nonsense standard URL. That is the point of the bug.

Comment 2 notes comm-central has its own setSpec callers; the lint uses
include: ['.'] and does not reach comm/, which has a separate lint config, so
Thunderbird is unaffected by this patch.