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

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Gijs, Assigned: mystor)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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.
Whiteboard: [qf:?] → [qf]
Michael, please take a look.
Flags: needinfo?(michael)
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 2

2 months ago
Created attachment 8857619 [details] [diff] [review]
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
Attachment #8857619 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

2 months ago
Flags: needinfo?(michael)
(Reporter)

Comment 3

2 months ago
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.
Attachment #8857619 - Flags: review?(gijskruitbosch+bugs)
Attachment #8857619 - Flags: review?(francesco.lodolo)
Attachment #8857619 - Flags: review+
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
Attachment #8857619 - Flags: review?(francesco.lodolo) → review?(l10n)

Comment 5

2 months ago
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.
Flags: needinfo?(michael)
(Assignee)

Comment 6

2 months ago
Created attachment 8857668 [details] [diff] [review]
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
Attachment #8857668 - Flags: review?(l10n)
(Assignee)

Updated

2 months ago
Attachment #8857619 - Attachment is obsolete: true
Attachment #8857619 - Flags: review?(l10n)

Comment 7

2 months ago
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.
Attachment #8857668 - Flags: review?(l10n) → review+

Updated

2 months ago
Flags: needinfo?(michael)
Assignee: nobody → michael

Comment 8

2 months ago
Pushed by michael@thelayzells.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4aa77ef26ad
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Updated

2 months ago
Blocks: 1358369
You need to log in before you can comment on or make changes to this bug.