1352684 - On Windows we hit the registry for mimetype info for many ScriptLoaderRunnable and nsScriptloader::StartLoad requests loading a js or jsm file from file:/

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[:Gijs (he/him)]

Spinning this off from bug 1352348, see bug 1352348 comment 15.

Basically, on a local build I see us hitting https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#491 for the .js and .jsm extensions of os_file_async_worker.js and all the js/jsm files it requires, every time we load them (which seems to be repeatedly, which might be another bug).

It looks like this was previously bug 632490, but that workaround was only put in one place.

We also seem to hit the os helper app service for .json and .ico (e.g. for search plugin icons).

I'm actually tempted to suggest that, rather than hardcoding those mimetypes into various consumers (along the lines of bug 632490), maybe we should teach nsExternalHelperAppService some of the mimetypes for extensions we use ourselves, to avoid hitting this path all the time. Would that make sense?

It does seem like the impact of this thing is (much?) smaller on the packaged builds we ship, because they use nsJARChannel for most files, which for whatever reason doesn't seem to be doing the same thing.

Comment 1

[Andrew Overholt [:overholt]]

Michael, please take a look.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup

This is a simple patch which does the minimal work to potentially mitigate this
issue. It adds a bunch of file extensions which I noticed from attaching a
debugger we try to ask for the mime type of during startup or typing in the
awesomebar. This list of types and extensions is checked before asking the OS in
GetTypeFromExtension.

js, json and jsm files are obvious, they are loaded during startup frequently,
and making sure we don't ask the OS for their MIME type seems like a good idea.

ico files are loaded by the awesomebar to show completion information when
typing. I figured that that was probably performance sensitive and so we should
not do the extra IO to get the MIME type in that situation.

the properties and locale files are used for localization. In our normal
localization code we explicitly set the MIME type of the channel before opening
it to text/plain, but in some code (such as devtools), we use a different
implementation of localization, which doesn't set this. I wasn't sure what a
good default for these types was so I chose application/x-java-properties (the
internet suggested this MIME type...), but text/plain might make more sense
there.

This code doesn't change the behaviour of GetFromTypeAndExtension which returns
a nsIMIMEInfo* object. Fabricating one of those from scratch without asking the
operating system seemed much trickier, so I decided not to take that route. In
the process of starting up I hit a breakpoint in that function only 2 times,
both looking for the nsIMIMEInfo for application/pdf in PdfJs.jsm
(isDefaultHandler()).

MozReview-Commit-ID: 9vqyWtDe11h

Comment 3

[:Gijs (he/him)]

Comment on attachment 8857619 [details] [diff] [review]
Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup

Review of attachment 8857619 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. I wonder if we should put the items in with the "asked about during startup" bits at the top.

I don't know about the .locale / .properties files... asking Flod for a second opinion.

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8857619 [details] [diff] [review]
Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup

Review of attachment 8857619 [details] [diff] [review]:
-----------------------------------------------------------------

It feels safer to redirect to Axel.

I don't think I've never seen a .locale file, and I have the feeling text/plain would be correct for .properties

Comment 5

[Axel Hecht [:Pike]]

Michael, what happens if you load chrome://browser/locale/browser.properties with the patch applied?

I found the same mime type as you did, but I'd prefer to avoid a download action when trying to view a .properties file in the content area. Every now and then, it's useful for debugging.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup

I've changed both properties and locale to use text/plain. They were triggering a download when opened in content before and keeping the old behaviour is preferable IMO.

In addition locale has only one file AFAIK (update.locale) which was defintely just a plain text file (For example on my copy of nightly it contains just the string "en-US"). I figured we could include it in the list to hit the OS just a bit less, because it doesn't really hurt, but there is only one load which will ever use it. I can remove it if we decide we don't want it.

MozReview-Commit-ID: 9vqyWtDe11h

Comment 7

[Axel Hecht [:Pike]]

Comment on attachment 8857668 [details] [diff] [review]
Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup

Review of attachment 8857668 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for checking, r=me.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4aa77ef26ad
Add json, js, jsm, iso, properties, and locale to defaultMimeEntries, as they are used at startup, r=pike

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e4aa77ef26ad