1358369 - nsExternalHelperAppService::GetTypeFromExtension() can do main thread IO

Status: RESOLVED DUPLICATE of bug 1352348

(Firefox :: File Handling, defect, P1)

Description

[(no longer active)]

See this hang reported by the BHR data: <https://people-mozilla.org/~mlayzell/bhr/20170405/277.html#0>

This is a call stack after the main thread was unresponsive for 8 seconds.  It could be that this call itself hadn't returned for 8 seconds, or that this was the tail of a long running execution...

Comment 1

[:Paolo Amadini]

It would be really interesting to know the reason why GetTypeFromExtension was called in this case, it's not clear to me by reading the profile as I don't see the JavaScript stack.

We might not even *need* to get the handler application (and file version information) at all... this is related to something Gijs has started to investigate in bug 1352348. Here is something pretty scary that he found out about JS and JSM extensions:

""" That is to say, I set a breakpoint in GetMIMEInfoFromOS, and it gets hit (repeatedly) by the script loader because it hits nsBaseChannel::AsyncOpen, which hits nsFileChannel::MakeFileInputStream, which hits this code asking for mime info for the "js" and "jsm" extensions. I'll file a separate bug for that, I guess... """

Gijs, did you end up filing a bug for that issue specifically?

Ehsan, there was a needinfo for you on that bug, but I'll move it here since this is more closely related and the other bug is resolved.

Comment 2

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #1)
> It would be really interesting to know the reason why GetTypeFromExtension
> was called in this case, it's not clear to me by reading the profile as I
> don't see the JavaScript stack.
> 
> We might not even *need* to get the handler application (and file version
> information) at all... this is related to something Gijs has started to
> investigate in bug 1352348. Here is something pretty scary that he found out
> about JS and JSM extensions:
> 
> """ That is to say, I set a breakpoint in GetMIMEInfoFromOS, and it gets hit
> (repeatedly) by the script loader because it hits nsBaseChannel::AsyncOpen,
> which hits nsFileChannel::MakeFileInputStream, which hits this code asking
> for mime info for the "js" and "jsm" extensions. I'll file a separate bug
> for that, I guess... """
> 
> Gijs, did you end up filing a bug for that issue specifically?

Yes, and it's fixed on Nightly - bug 1352684.

Comment 3

[:Gijs (he/him)]

I don't see any non-test JS callsites outside the preferences - https://dxr.mozilla.org/mozilla-central/search?q=getTypeFromExtension+-path%3Atest&redirect=true . The JS handler service stuff that that hits is different. There's a bunch of add-ons that call this though.

I'll also add that in bug 1352348 we made it so that calls to getTypeByExtension no longer get full mime info objects on windows, only the mime type string (which is enough to be able to return from getTypeByExtension), so that will in and of itself have fixed this, I think? Hard to verify if we don't have STR for comment #0 though...

The prefs only seem to call this with "exe", here:

https://dxr.mozilla.org/mozilla-central/rev/8b854986038cf3f3f240697e27ef48ea65914c13/browser/components/preferences/in-content/applications.js#1472-1480

but doesn't cache this. From the doc of that method, it gets called whenever an entry is selected. We could potentially cache that, but equally this code isn't hot - it gets called when a row is selected, according to the comments. I tried to look at whether you could get rows to be selected repeatedly by navigating with the arrow keys in the list, but that doesn't seem to be possible (which might be a separate bug, tbh).

Anyway, as noted, I think this codepath is no longer followed post-1352348, so maybe this should be WFM? Ehsan can confirm.

Comment 4

[(no longer active)]

Comment 3 is right, this one code path is mostly gone, but still nsExternalHelperAppService::GetTypeFromExtension is used from many other places and it seems wasteful that this function reads a whole bunch of information synchronously from the disk and it never actually uses it.  I think adding some caching here is also a good idea.  We'd get the cache entry wrong when the file associations change, but that should be a very rare occurrence, and I don't know to what extent we should care about that.  Michael said he may be inteterested to have a look at this one since he has looked at the other bug also.

Comment 5

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

I think that bug 1352348 does everything that we need to do to remove the disastrous IO situation, so I am inclined to close this as a dup.