In local profiles, I'm seeing about 23ms spent in ComponentLoaderInfo::EnsureScriptChannel before firstpaint. That's happening because we create a channel for the URI passed to Cu.import every time it's called, regardless of whether the module is cached. We also do a lot of unnecessary extra work to resolve the URI to a local file in those cases.
With a few quick hacks, I see that overhead all but disappear, so I think this is something we should be able to easily fix.

Comment on attachment 8888967[details]Bug 1383215: Part 2 - Split out URI resolution code into ResolveURI helper.
https://reviewboard.mozilla.org/r/159982/#review165370> The new code was calls GetSpec() in the resource case, but the old code did not. Is there some reason that's okay?
The old code just used the spec returned from nsIResProtocolHandler::ResolveURI prior to it being converted into a URI. We wind up with the same spec and URI now as we did before, albeit slightly less efficienty for the resource: URI case.

(In reply to Andrew McCreight [:mccr8] from comment #10)
> For part 3, if mKey is equal to mLocation, can you delete mKey and
> EnsureKey, and make Key() return mLocation (and change the return type)?
I was considering getting rid of mKey, yeah. I decided against it, but not for any particular reason, so I'm happy to make that change.
I'd like to keep EnsureKey and just make it an alias for EnsureLocation, though, just in case we decide to change it later.

(In reply to Kris Maglione [:kmag] from comment #11)
> I was considering getting rid of mKey, yeah. I decided against it, but not
> for any particular reason, so I'm happy to make that change.
>
> I'd like to keep EnsureKey and just make it an alias for EnsureLocation,
> though, just in case we decide to change it later.
Sounds good, thanks.

Doesn't this break legacy add-ons?
This caused TB a lot of bustage since we were using imports inconsistently (bug 1384218).
One of my add-ons also got broken since it used
Components.utils.import('resource://gre/modules/iteratorUtils.jsm');
Components.utils.import('resource://gre/modules/mailServices.js');
where the rest of TB uses this without the "gre".
Wouldn't that also break FF add-ons when this ships in FF 56 beta and then in release?

(In reply to Andrew McCreight [:mccr8] from comment #20)
> Should this wait for 57? Given that it also broke a ton of tests, that seems
> a little concerning.
I don't think so. Nearly all of the tests it broke were xpcshell tests that were importing resource:///modules/Services.jsm or resource:///modules/XPCOMUtils.jsm, which we stopped supporting ordinary Firefox builds years ago.
The others were just tests for the old module aliasing behavior, and one SDK loader test which only broke because it was testing the behavior of the loader separately for file: URLs and resource: URLs that point to the same place, but without unloading the module in-between.

(In reply to Kris Maglione [:kmag] from comment #21)
> (In reply to Andrew McCreight [:mccr8] from comment #20)
> > Should this wait for 57? Given that it also broke a ton of tests, that seems
> > a little concerning.
>
> I don't think so. Nearly all of the tests it broke were xpcshell tests that
> were importing resource:///modules/Services.jsm or
> resource:///modules/XPCOMUtils.jsm, which we stopped supporting ordinary
> Firefox builds years ago.
Thunderbird does still allow this, though, so it may affect Thunderbird add-ons, but those aren't really affected by the 57 timeline one way or the other.

(In reply to Jorg K (GMT+2) from comment #23)
> Sorry about the (silly) question: What does the "gre" stand for? No
> explanation here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/
> Language_Bindings/Components.utils.import
>
> Any idea why TB is using imports of its own modules without the "gre"?
There are two ways that we currently support packaging omnijar:
1) Separate JAR files for toolkit (GRE) content and app-specific content.
2) One JAR file containing both app-specific and toolkit content.
Firefox uses the former (but used to use the latter), and Thunderbird uses the latter.
In case 2, resource:/// and resource://gre/ point to the same place, so it's technically possible to refer to app or toolkit content by two separate URLs, and it's easy to carelessly use the wrong one. We had a bunch of these issues (especially with add-ons) when we switched layouts.
But the code that's using resource://gre/ URLs for app content, or vice versa, is still technically wrong.

Thanks for the explanation.
In bug 1384218 I fixed a lot of the "technically wrong": I fixed a few resource:///modules/Services.jsm and resource:///modules/XPCOMUtils.jsm by adding the "gre" and removed the "gre" from a URLs for TB/app content.

The changes from here made it so that it's no longer possible to open HTML pages from jetpack addons as regular web pages.
(I'm getting "Access to the file was denied" since Nightly 20170726100241) Is this intentional? I'm interested to know because my addon uses a packaged HTML file as its UI, so it depends on the old behavior to work properly.
I backed out all the changesets from here and my addon works again.
Here is the error in the console when this happens:
NS_ERROR_FILE_ACCESS_DENIED: Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIWebNavigation.loadURIWithOptions] browser-child.js:354