Closed Bug 1536744 Opened 9 months ago Closed 7 months ago

Make NS_NewURI work off main thread and remove nsIProtocolHandler.newURI

Categories

(Core :: Networking, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(22 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

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

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

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

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

Depends on D30697

This is achieved by adding a RWLock to SubstitutingProtocolHandler

Depends on D30698

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

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

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

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

Depends on D30708

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

Depends on D30711

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

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

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

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

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

Blocks: 1553105

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.

Attachment #9064153 - Attachment is obsolete: true

These should be removed once bug 1553105 lands.

Depends on D30713

Pushed by valentin.gosu@gmail.com:
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
Regressions: 1555302
Regressions: 1555309
Blocks: 1555359
Depends on: 1557677
Regressions: 1560703
Regressions: 1559356
No longer regressions: libdweb-protocol
See Also: → libdweb-protocol
You need to log in before you can comment on or make changes to this bug.