1536744 - Make NS_NewURI work off main thread and remove nsIProtocolHandler.newURI

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

Now that bug 1532253 has landed we may be able to "tie up all the loose ends".

(In reply to Valentin Gosu [:valentin] from bug 1532253 comment #5)

The protocols that are left:

• moz-gio - depends on prefs
• moz-icon - calls IOService.newURI
• view-source - calls NS_NewURI for inner part of the URI - we make it call NS_NewURIOnAnyThread
• resource - extends SubstitutingProtocolHandler (substitution flags + jar)
• moz-extension - extends SubstitutingProtocolHandler (substitution flags + jar)
• jar - nsJARURI::SetSpecWithBase calls ioServ->NewURI
• about - calls NS_GetAboutModule (can be JS implemented)

Apart from these there's nsExternalProtocolHandler::NewURI which simply instantiates a new nsSimpleURI, but we can't fall back to it if the above protocols aren't handled.

NS_NewURI should be replaced by NS_NewURIOnAnyThread.
nsIOService.newURI should call directly into NS_NewURIOnAnyThread
nsIProtocolHandler.newURI should be removed. All calls should be replaced with either NS_NewURIOnAnyThread in C++ or Services.io.newURI in JS.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

So I've got a very unpolished set of patches that moves the creation of most protocol types to NS_NewURIOnAnyThread.
However, I don't think we can work around the protocol handlers for resource and moz-extension URIs.
So the strategy would be to make those protocol handlers (just .newURI() actually) threadsafe. This would be accomplished by making SubstitutingProtocolHandler use NS_INLINE_DECL_THREADSAFE_REFCOUNTING, then making sure that all member variables of SubstitutingProtocolHandler, nsResProtocolHandler, ExtensionProtocolHandler that are touched by newURI be accessed under a lock (when read off main thread or modified. Reading them on the main thread doesn't really need to lock them if they can only be changed on the main thread).

nsResProtocolHandler::ResolveSpecialCases

• nsResProtocolHandler::mAppURI
• nsResProtocolHandler::mGREURI
• nsResProtocolHandler::mApkURI

ExtensionProtocolHandler::ResolveSpecialCases ->
SubstitutingProtocolHandler::HasSubstitution

• SubstitutingProtocolHandler::mSubstitutions

ExtensionProtocolHandler::ResolveSpecialCases ->
ExtensionPolicyService::GetGeneratedBackgroundPageUrl ->

• ExtensionPolicyService::mExtensionHosts

• maybe WebExtensionPolicy::mBackgroundScripts - but it doesn't seem to change after the constructor, so this may be OK.
  SubstitutingProtocolHandler::GetSubstitution

• SubstitutingProtocolHandler::mSubstitutions

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add nsResProtocolHandler::GetSingleton() r=mayhemer,baku

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add nsAboutProtocolHandler::CreateNewURI() r=mayhemer,baku

Depends on D30692

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Make SubstitutingProtocolHandler::NewURI only call ResolveJARURI if the host is android r=mayhemer,baku

Depends on D30693

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - NS_NewURIOnAnyThread should construct nsStandardURLs for smb and sftp URLs r=mayhemer,baku

These URL types need both specific applications to be installed on Linux, and for the pref to list them as supported.
We do these checks in nsGIOProtocolHandler::NewChannel instead of performing them when creating the URLs.

Depends on D30694

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Make nsIOService.newURI call NS_NewURI, and NS_NewURI call NS_NewURIOnAnyThread r=mayhemer,baku

This way all of the nsIURI creation will go through NS_NewURIOnAnyThread.
NS_NewURIOnAnyThread will be renamed to NS_NewURI in the final patch.

Depends on D30695

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add {nsAboutProtocolHandler,nsViewSourceHandler,SubstitutingProtocolHandler}::CreateNewURI() static methods r=mayhemer,baku

Depends on D30696

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add "resource" and "moz-extension" to NS_NewURIOnAnyThread. r=mayhemer,baku

• Add main thread assertions for "resource" and "moz-extension" protocols
• Use ::GetSingleton() for resource and moz-extension protocol handlers

Depends on D30697

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Make resource protocol handler thread safe r=mayhemer,baku

This is achieved by adding a RWLock to SubstitutingProtocolHandler

Depends on D30698

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add "jar" to NS_NewURIOnAnyThread r=mayhemer,baku

Depends on D30699

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add "about" protocol to NS_NewURIOnAnyThread r=mayhemer,baku

Depends on D30700

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add "moz-icon" to NS_NewURIOnAnyThread r=mayhemer,baku

Depends on D30701

Comment 13

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Remove nsIProtocolHandler.newURI r=mayhemer,baku

Depends on D30702

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add indexeddb and android schemes to NS_NewURIOnAnyThread r=mayhemer,baku

Depends on D30703

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Fix test by adding a proper implementation of nsSimpleURI::Resolve that uses MozURL r=mayhemer,baku

nsSimpleURI doesn't really have the concept of a relative URI.
Since unknown protocol schemes will ultimately fall back to using nsSimpleURI,
we need to make sure that resolving a URI with certain base still works as
before, when those URIs were nsStandardURLs, created by the protocol handlers.

To achieve this we check to see if the "relative path" has a scheme. If it
does, we just return it, to be parsed by NS_NewURI. Otherwise, we use
MozURL (based on rust-url) to figure out the correct relative URL we should
return. This by itself manages to fix several failing web-platform tests.

Depends on D30704

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add nsGIOProtocolHandler::GetSingleton(). Make sure nsIOService::GetProtocolHandler only returns it when it's supported. r=mayhemer,baku

Before, we'd try to create a URI using the GIO protocol handler, and if that
succeeded, we'd return the protocol handler.
Now we can't return it if NS_NewURI succeeds, because NS_NewURI doesn't check
the protocol handler anymore. So instead we just instantiate the handler,
check if the scheme is supported, and if so return it.

Depends on D30705

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add ssh scheme to NS_NewURIOnAnyThread r=mayhemer,baku

This is needed to pass a web-platform-test. Since unknown protocols use
nsSimpleURI and nsSimpleURI::GetHost returns an error code, that will fail.
Instead, we instantiate an nsStandardURL for the ssh scheme.
Once we change nsSimpleURI to be backed by MozURL we can probably remove this.

Depends on D30706

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Fix expected result in test_url.html r=mayhemer,baku

Depends on D30707

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Remove expected FAILs from URL web-platform-tests r=mayhemer,baku

These were fixed by the improvements added to nsSimplerURI::Resolve()

Depends on D30708

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Fix test_import.js by calling nsIIOService.newURI instead of nsIResProtocolHandler.newURI r=mayhemer,baku

Depends on D30709

Comment 21

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Fix test_search_suggestions.js - Creating an ftp URL doesn't fail if the ftp protocol handler is disabled r=mayhemer,baku

Depends on D30710

Comment 22

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Make sure we only return an origin for schemes that are allowed in the URL spec r=mayhemer,baku

There's a web-platform-test expecting the origin for a ssh URI to be null

Depends on D30711

Comment 23

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Rename NS_NewURIOnAnyThread to NS_NewURI. r=mayhemer,baku

The only protocol that can't be created off the main thread at the moment is
moz-extension, and that can be handled at a later time.

Depends on D30712

Comment 24

[Masatoshi Kimura [:emk]]

This change would have a huge impact on c-c.

Comment 25

[Jorg K (CEST = GMT+2)]

Thanks for the heads-up. You don't intend to land this during the 68 cycle, right? I'm a bit confused by the large amount of changes here. What would be the impact? Looks like we have seven protocol handlers which implement NewURI():
https://searchfox.org/comm-central/search?q=%3A%3ANewURI(&case=false&regexp=false&path=mailnews%2F

Comment 26

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jorg K (GMT+2) from comment #25)

Thanks for the heads-up. You don't intend to land this during the 68 cycle, right?

This is definitely not going to land during 68. If all goes well it will be in 69. Just reviewing the patches will likely take a while.

I'm a bit confused by the large amount of changes here. What would be the impact? Looks like we have seven protocol handlers which implement NewURI():
https://searchfox.org/comm-central/search?q=%3A%3ANewURI(&case=false&regexp=false&path=mailnews%2F

To accommodate c-c it seems we can ifdef most of those schemes in NS_NewURI{OnAnyThread}.
It seems nsImapService::NewURI and nsPop3Service::NewURI are slightly more complex, and but we can handle them in a similar fashion to the resource and moz-extension URLs

What I'd propose is that at the end of NS_NewURI we add something like:

#if defined(MOZ_THUNDERBIRD) || defined(MOZ_SUITE)
   return NS_NewThunderbirdURI(aURI, aSpec, aCharset, aBaseURI, aIOService);
#endif

Then NS_NewThunderbirdURI can be used to deal with the any of the quirks of those implementations.
Jorg, can you file a separate bug for porting this bug to c-c? I think this might be a good opportunity to remove some of the extra protocol handlers, and make the c-c URIs a bit nicer. I have some ideas for that.

Comment 27

[Jorg K (CEST = GMT+2)]

Thanks, I filed bug 1550945. Can you please point out what the troublesome bits are.

Comment 28

[Phabricator Automation]

Comment on attachment 9064153 [details]
Bug 1536744 - Make sure we only return an origin for schemes that are allowed in the URL spec r=mayhemer,baku

Revision D30712 was moved to bug 1553105. Setting attachment 9064153 [details] to obsolete.

Comment 29

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1536744 - Add WPT expect FAIL for ssh origin tests r=baku

These should be removed once bug 1553105 lands.

Depends on D30713

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/08145972450d
Add nsResProtocolHandler::GetSingleton() r=baku
https://hg.mozilla.org/integration/autoland/rev/fe9ee62644df
Add nsAboutProtocolHandler::CreateNewURI() r=baku
https://hg.mozilla.org/integration/autoland/rev/579eff229d65
Make SubstitutingProtocolHandler::NewURI only call ResolveJARURI if the host is android r=baku
https://hg.mozilla.org/integration/autoland/rev/e0ebd1ac8ef4
NS_NewURIOnAnyThread should construct nsStandardURLs for smb and sftp URLs r=baku
https://hg.mozilla.org/integration/autoland/rev/91e6edcfa40c
Make nsIOService.newURI call NS_NewURI, and NS_NewURI call NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/3ff470f6fa58
Add {nsAboutProtocolHandler,nsViewSourceHandler,SubstitutingProtocolHandler}::CreateNewURI() static methods r=baku
https://hg.mozilla.org/integration/autoland/rev/0a27f68d2387
Add "resource" and "moz-extension" to NS_NewURIOnAnyThread. r=baku
https://hg.mozilla.org/integration/autoland/rev/2b7f7b802744
Make resource protocol handler thread safe r=baku
https://hg.mozilla.org/integration/autoland/rev/7c0e5643c209
Add "jar" to NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/dcdc05e86666
Add "about" protocol to NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/bbb8bd167fbc
Add "moz-icon" to NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/bf85e54d26dd
Remove nsIProtocolHandler.newURI r=baku
https://hg.mozilla.org/integration/autoland/rev/1c6b4a75a605
Add indexeddb and android schemes to NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/d418e0d5594a
Fix test by adding a proper implementation of nsSimpleURI::Resolve that uses MozURL r=baku
https://hg.mozilla.org/integration/autoland/rev/2dead78339e0
Add nsGIOProtocolHandler::GetSingleton(). Make sure nsIOService::GetProtocolHandler only returns it when it's supported. r=baku
https://hg.mozilla.org/integration/autoland/rev/def28b67d7d8
Add ssh scheme to NS_NewURIOnAnyThread r=baku
https://hg.mozilla.org/integration/autoland/rev/15153522a5d9
Fix expected result in test_url.html r=baku
https://hg.mozilla.org/integration/autoland/rev/d348b926aa2a
Remove expected FAILs from URL web-platform-tests r=baku
https://hg.mozilla.org/integration/autoland/rev/feb205a5e548
Fix test_import.js by calling nsIIOService.newURI instead of nsIResProtocolHandler.newURI r=baku
https://hg.mozilla.org/integration/autoland/rev/4156260290c8
Fix test_search_suggestions.js - Creating an ftp URL doesn't fail if the ftp protocol handler is disabled r=baku
https://hg.mozilla.org/integration/autoland/rev/e29e51a1dfd4
Rename NS_NewURIOnAnyThread to NS_NewURI. r=baku
https://hg.mozilla.org/integration/autoland/rev/ec6db16dbb4e
Add WPT expect FAIL for ssh origin tests r=baku

Comment 31

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/08145972450d
https://hg.mozilla.org/mozilla-central/rev/fe9ee62644df
https://hg.mozilla.org/mozilla-central/rev/579eff229d65
https://hg.mozilla.org/mozilla-central/rev/e0ebd1ac8ef4
https://hg.mozilla.org/mozilla-central/rev/91e6edcfa40c
https://hg.mozilla.org/mozilla-central/rev/3ff470f6fa58
https://hg.mozilla.org/mozilla-central/rev/0a27f68d2387
https://hg.mozilla.org/mozilla-central/rev/2b7f7b802744
https://hg.mozilla.org/mozilla-central/rev/7c0e5643c209
https://hg.mozilla.org/mozilla-central/rev/dcdc05e86666
https://hg.mozilla.org/mozilla-central/rev/bbb8bd167fbc
https://hg.mozilla.org/mozilla-central/rev/bf85e54d26dd
https://hg.mozilla.org/mozilla-central/rev/1c6b4a75a605
https://hg.mozilla.org/mozilla-central/rev/d418e0d5594a
https://hg.mozilla.org/mozilla-central/rev/2dead78339e0
https://hg.mozilla.org/mozilla-central/rev/def28b67d7d8
https://hg.mozilla.org/mozilla-central/rev/15153522a5d9
https://hg.mozilla.org/mozilla-central/rev/d348b926aa2a
https://hg.mozilla.org/mozilla-central/rev/feb205a5e548
https://hg.mozilla.org/mozilla-central/rev/4156260290c8
https://hg.mozilla.org/mozilla-central/rev/e29e51a1dfd4
https://hg.mozilla.org/mozilla-central/rev/ec6db16dbb4e