1352348 - Firefox fails to get content-type from the OS

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[Marek Sierociński]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36 OPR/43.0.2442.1144

Steps to reproduce:

Trying upload certificate file on Windows 10 (e.g. crt or pem file). Firefox on Windows says that it is "application/octet-stream" MIME type.
On Firefox > 51 adding proper entries to mimeTypes.rdf helped.


Actual results:

After updating to Firefox 51 (and newer) mimeTypes.rdf entries are not respected during uploading process.


Expected results:

Firefox should still respecting mimeTypes.rdf entries during uploading process and/or should introduce and document properly new method of adding custom MIME types (and files extensions) that will be respected during file uploading process.

Comment 1

[Loic]

Could you write detailed steps to reproduce (providing a random cert, how to edit mimeTypes.rdf, etc) to reproduce excatly the issue, it would help to find a regression range.

Comment 2

[Marek Sierociński]

Attachment: X509 certificate of bugzillla.mozilla.org from 31/03/2017

Comment 3

[Marek Sierociński]

Attachment: Effect in Google Chrome

Comment 4

[Marek Sierociński]

Attachment: Effect in Firefox 50 with patched mimeTypes.rdf

Comment 5

[Marek Sierociński]

Attachment: Effect in Firefox 51 with patched mimeTypes.rdf

Comment 6

[Marek Sierociński]

Steps for Windows 10

Case 1. Google Chrome (as reference)
1. Open Google Chrome
2. Go to http://mime.ritey.com/
3. Upload attached in bug certificate ("X509 certificate of bugzillla.mozilla.org from 31/03/2017")
4. Check if MIME type is "application/x-x509-ca-cert"


Case 2. Mozilla Firefox 50 without patched mimeTypes.rdf
1. Open Mozilla Firefox 50
2. Go to http://mime.ritey.com/
3. Upload attached in bug certificate ("X509 certificate of bugzillla.mozilla.org from 31/03/2017")
4. Check if MIME type is "application/x-x509-ca-cert"

Case 3. Mozilla Firefox 50 WITH patched mimeTypes.rdf
1. Patch mimeTypes.rdf with proper entries. 
- Read mimeTypes.rdf manual from mozillazine
- You can use patcher that I wrote: https://github.com/bitbar/firefox-missing-cert-mimetype-patcher/raw/master/bin/Release/FirefoxMissingCertMimeTypePatcher.exe
- Source code here: https://github.com/bitbar/firefox-missing-cert-mimetype-patcher
2. Open Mozilla Firefox 50
3. Go to http://mime.ritey.com/
4. Upload attached in bug certificate ("X509 certificate of bugzillla.mozilla.org from 31/03/2017")
5. Check if MIME type is "application/x-x509-ca-cert"

Case 4. Mozilla Firefox 51 WITH patched mimeTypes.rdf
1. Patch mimeTypes.rdf with proper entries. 
- Read mimeTypes.rdf manual from mozillazine
- You can use patcher that I wrote: https://github.com/bitbar/firefox-missing-cert-mimetype-patcher/raw/master/bin/Release/FirefoxMissingCertMimeTypePatcher.exe
- Source code here: https://github.com/bitbar/firefox-missing-cert-mimetype-patcher
2. Open Mozilla Firefox 51
3. Go to http://mime.ritey.com/
4. Upload attached in bug certificate ("X509 certificate of bugzillla.mozilla.org from 31/03/2017")
5. Check if MIME type is "application/x-x509-ca-cert"


Results:
Case 1: Success
Case 2: Fail (result is "application/octet-stream")
Case 3: Success
Case 4: Fail (result is "application/octet-stream")

Comment 7

[Marek Sierociński]

Here are entires that my patched adds (according to http://kb.mozillazine.org/MimeTypes.rdf):

  <RDF:Seq RDF:about="urn:mimetypes:root">
    <RDF:li RDF:resource="urn:mimetype:application/pdf"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-pem-file"/>
    <RDF:li RDF:resource="urn:mimetype:application/pkix-cert"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-x509-user-cert"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-x509-ca-cert"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-pkcs7-certificates"/>
    <RDF:li RDF:resource="urn:mimetype:application/pkcs7-mime"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-pkcs12"/>
    <RDF:li RDF:resource="urn:mimetype:application/x-pkcs12"/>
  </RDF:Seq>
  <RDF:Description RDF:about="urn:mimetype:application/x-pem-file"
                   NC:fileExtensions="pem"
                   NC:description="Certificate (.pem)"
                   NC:value="application/x-pem-file"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-pem-file"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/pkix-cert"
                   NC:fileExtensions="cer"
                   NC:description="Certificate (.cer)"
                   NC:value="application/pkix-cert"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/pkix-cert"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/x-x509-user-cert"
                   NC:fileExtensions="crt"
                   NC:description="Certificate (.crt)"
                   NC:value="application/x-x509-user-cert"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-x509-user-cert"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/x-x509-ca-cert"
                   NC:fileExtensions="der"
                   NC:description="Certificate (.der)"
                   NC:value="application/x-x509-ca-cert"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-x509-ca-cert"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/x-pkcs7-certificates"
                   NC:fileExtensions="p7b"
                   NC:description="Certificate (.p7b)"
                   NC:value="application/x-pkcs7-certificates"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-pkcs7-certificates"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/pkcs7-mime"
                   NC:fileExtensions="p7c"
                   NC:description="Certificate (.p7c)"
                   NC:value="application/pkcs7-mime"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/pkcs7-mime"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/x-pkcs12"
                   NC:fileExtensions="p12"
                   NC:description="Certificate (.p12)"
                   NC:value="application/x-pkcs12"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-pkcs12"/>
  </RDF:Description>
  <RDF:Description RDF:about="urn:mimetype:application/x-pkcs12"
                   NC:fileExtensions="pfx"
                   NC:description="Certificate (.pfx)"
                   NC:value="application/x-pkcs12"
                   NC:editable="true">
    <NC:handlerProp RDF:resource="urn:mimetype:handler:application/x-pkcs12"/>
  </RDF:Description>

Comment 8

[Loic]

Does your patcher find itself the path of mimeTypes.rdf in the profile? How does it behave when there is more 1 profile for Firefox (I have 3 profiles for he current release and 10 for different versions)?

Comment 9

[Marek Sierociński]

Hmm to be honest I don't know - I use only one profile so didn't thought about that. Maybe for your good don't use it then and please just copy-paste entries that I provided above. Today I'm out of office so I will look at multi profiles support for my patcher at Monday :) but we are starting small offtopic I guess ;) so please try to manually edit your file (respecting mozillazine docs off course).

Comment 10

[Loic]

Attachment: mimeTypes.rdf

Comment 11

[Loic]

STR:
1) Open the profile folder to save the attached mimeTypes.rdf
2) Load http://mime.ritey.com/
3) Upload the attached X509 certificate

Actual result:
The MIME type for your file is: application/octet-stream

Expected result: 
The MIME type for your file is: application/x-x509-ca-cert

Reg range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bd7645928990649c84609d3f531e803c2d41f269&tochange=8bcfd9dda91cd024bdebba5c599b8710e55ee829

Regressed by Bug 306471.

Xidorn, could you check this regression in 51+ after your bugfix in Bug 306471, please.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

This is expected that mimeTypes.rdf is no longer respected for uploading. This is because that file can be affected by download, and lots of users complained about Firefox uploading with insensible mime-type just because they ever downloaded a file somewhere with a wrong content-type specified by the website. You can see bug 373621 and bug 332690 for this long time complaint.

I think the real issue here is that, we fail to get the mime-type from the system. I'm on Windows, and I checked my registry, and it seems there exists item for .der file with correct content-type. Firefox is supposed to use that information. This is something we need to fix.

Comment 13

[Marek Sierociński]

For the God sake! YES! YES YES! :D
I just wrote it to Loic that this is the real issue here - that ONLY Firefox has problem with MIME-types supported by OS. Other reference browsers do it properly.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

But I probably don't have time to work on this recently...

The start point should be the code in nsExternalHelperAppService::GetTypeFromExtension [1]. To fix this, we probably need to investigate what's going wrong with the GetMIMEInfoFromOS function on different systems. Probably different system needs different fix, and maybe some are not affected at all...

Gijs, could you have a look at this issue, or help finding someone to work on this?


[1] https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/nsExternalHelperAppService.cpp#2723-2728

Comment 15

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> But I probably don't have time to work on this recently...
> 
> The start point should be the code in
> nsExternalHelperAppService::GetTypeFromExtension [1]. To fix this, we
> probably need to investigate what's going wrong with the GetMIMEInfoFromOS
> function on different systems. Probably different system needs different
> fix, and maybe some are not affected at all...
> 
> Gijs, could you have a look at this issue, or help finding someone to work
> on this?

I had a quick look. I think the issue is that all the internal APIs here are awful.

More seriously, AFAICT:

1) https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#491
nsOSHelperAppService::GetMIMEInfoFromOS supports being called with both/either a mimetype or a file extension. I'm... not sure why it's been written that way.

2) we call it with an empty string for a mimetype (of course, we want to find out the mime type!) and an extension. So we enter all this code:

https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#507-521

and try to do a registry lookup to find an extension for the mimetype "". Needless to say, that doesn't work very well. I think we should just check for that string being empty and don't bother with any of that stuff if so.

3) later on we apparently realize we've only got an extension to go on and we make the 'right' call, here:
https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#540-543

to GetByExtension.

This last bit is sensible, more or less.

4) Even better, GetByExtension initially does the right thing, and this code: https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#412-432 correctly determines the right mimetype (!).

5) Then we try to find the Windows descriptive name (which I'm sure is not the right terminology, but whatever) for the mimetype and file extension, using this blob of code: https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#441-471 .

This is my second issue with this code: why return nullptr if finding this fails? We still got a mimetype, and we don't actually care about any of the rest here...

In any case, we just about manage to do this, apparently this is a "CERFile". Fine.

6) Now we try to find the default app info for this file... which, to be clear, I don't think the original consumer here cares about at all...

https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#473-480

7) Unfortunately this fails. Specifically, we successfully read some more registry stuff:

https://dxr.mozilla.org/mozilla-central/rev/00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/nsOSHelperAppService.cpp#308-342

... but we end up with an expanded rundll executable with some relative dll paths. We then strip off the rundll bits, split off some more parameters, and end up with "cryptext.dll", try and fail to init a file with that as its absolute path, and then return an error. Which then means the NS_ENSURE_SUCCESS fails, which means we end up bailing out of the block cited in (6) and return nullptr for the mimeinfo.

This means nsExternalHelperAppService gets a null nsIMIMEInfo object which means it won't have a mime type to use which means it falls back to octet-stream.


There's... a few things we could do better here, I think, but I'm not 100% sure at what level of abstraction we should refactor things because the way these lookups work is quite different on other OSes. In particular, I wouldn't be at all surprised if returning nsIMIMEInfo objects without app information as soon as we find a mimetype will break other consumers of ::GetMIMEInfoFromOS.

To me, two options that could work would be:
- add a flag to GetMIMEInfoFromOS that allows it to only fetch a mimetype for an extension (bail out before doing all the other stuff)
- expose getting a mimetype from an extension as a separate method on the OS helper app service (unix/mac/android implementations might need to just return NS_ERROR_NOT_IMPLEMENTED or whatever, or they can forward to the extant GetMIMEInfoFromOS thing if that could potentially do something sensible).

Xidorn, which of these seems more sensible to you? I can try to find time to fix this, I guess. :-\

Now for the kicker here... this code is actually not entirely cold (ie doesn't just get hit for e.g. file uploads). :|

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... It looks like it's relatively straightforward to avoid for the file channel case, less sure about jar channels (packaged builds). Needinfo'ing Ehsan to ask if Quantum Flow is looking at eliminating unnecessary registry access just like unnecessary IO, in which case I'd expect this type of stuff to turn up...

Comment 16

[:Gijs (he/him)]

Oops, ni Xidorn as well for the question in the lower end of comment #15.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

I'm... totally not familiar with this stuff. I happened to fix bug 306471 just because I saw that became a topic on Twitter and follow it, then got annoyed by the traffic there, so eventually try to figure out a fix...

Given the APIs is awful, I guess we actually want to review all its callsites and figure out their expectation on the APIs... but it seems the only other callsite of GetMIMEInfoFromOS is nsExternalHelperAppService::GetFromTypeAndExtension so probably having a new function makes sense for now?

Comment 18

[:Paolo Amadini]

I've very recently worked with the API layer just above this one, which is... similarly awful :-)

The main architectural issue I'm finding is that we have to query the information from the operating system when creating nsIMIMEInfo and nsIHandlerInfo objects, that are also used when reading custom configuration in Firefox. This makes it difficult to unit test the latter, and makes some operations that don't require all the information slower.

It would be better if we could create blank objects with just a specified MIME type, and then populate them as needed. This isn't something we need to fix here though. As noted, there might be challenges with the Windows implementation, where the file extension rather than the MIME type is used as the main key for file associations.

Gijs, if you come up with something sensible to fix this bug, I can review the change. Setting a needinfo on myself to take a closer look at the code in the meantime.

Comment 19

[:Paolo Amadini]

As a side note, this code is very likely a source of sync IPC messages.

Comment 20

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #19)
> As a side note, this code is very likely a source of sync IPC messages.

Why? I see it being used by sync IPC messages, but not creating any itself...

Comment 21

[:Paolo Amadini]

(In reply to :Gijs from comment #20)
> I see it being used by sync IPC messages, but not creating any itself...

I was in fact referring to this area of the code in general (uriloader/exthandler). The content process is blocked while waiting for the MIME types to be resolved in the parent process.

There is a lot of boilerplate code for passing around nsIHandlerInfo instances across processes synchronously. I wonder if the content process only needs a few asynchronous calls instead, given how downloads and handler selection windows all run in the parent process.

Comment 22

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 53, we don't have a lot of time left in beta though.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Looking over comment 17 and comment 18 we should probably wontfix this for 53. 
Gijs and Paolo, let me know what you think.

Comment 25

[:Gijs (he/him)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24)
> Looking over comment 17 and comment 18 we should probably wontfix this for
> 53. 
> Gijs and Paolo, let me know what you think.

I concur. Wontfix for 53 - this is not easy to fix and it's too late in the cycle at this point.

FWIW, if people are seriously impacted by this one potential local workaround would be associating the CERFile type with an alternative handler (ie an absolute path to some executable file). I *think* (haven't tested) that that might work around this issue for now.

Comment 27

[:Gijs (he/him)]

Attachment: Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Review commit: https://reviewboard.mozilla.org/r/130008/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130008/

Comment 28

[:Gijs (he/him)]

Attachment: Bug 1352348 - don't do registry lookups for empty strings passed when we want to know a mimetype,

Review commit: https://reviewboard.mozilla.org/r/130010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130010/

Comment 29

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review132548

Added Xidorn because we should obviously ensure we don't regress the PDF thing. (I don't think this does, but let's be more sure than that!)

As a nice side-effect, these changes will reduce the amount of registry IO we do for pure simple "I want a mimetype string" lookups on Windows. Probably not a huuuuge perf impact on its own, but it all adds up.

::: uriloader/exthandler/nsExternalHelperAppService.cpp:2940
(Diff revision 1)
> +  nsCOMPtr<nsIMIMEInfo> mimeInfo = GetMIMEInfoFromOS(EmptyCString(), aExtension, &found);
> +  if (found && mimeInfo) {
> +    mimeInfo->GetMIMEType(aMIMEType);
> +    return true;
> +  }
> +  return false;

Can't just return `found` because if mimeInfo is null we should still return false.

::: uriloader/exthandler/win/nsOSHelperAppService.h:43
(Diff revision 1)
> +  virtual bool GetMIMETypeFromOSForExtension(const nsACString& aExtension,
> +                                             nsACString& aMIMEType) override;

Please especially check if my use of inheritance here (both conceptually and in terms of syntax) makes sense.

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Had a brief look, and it looks reasonable, but I'm really not an expert for this module, so redirect to bz.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Just to respond to some of the questions raised in comment 15...

> nsOSHelperAppService::GetMIMEInfoFromOS supports being called with both/either a mimetype
> or a file extension. I'm... not sure why it's been written that way.

Because neither the type nor the extension on its own is enough to uniquely pin down MIME info in general.  There are various cases in which different XML-based formats all get mapped to application/xml but need to be handled by different helper apps.  There are also cases in which the same extension is used for multiple different types of files (both in terms of MIME type and in terms of the right helper app to use).

So the API is designed to take all the information the caller has and in theory use it to figure out the best course of action.  Whether that theory actually works in practice, I'm not sure.

> I think we should just check for that string being empty and don't bother with any of that stuff if so.

That would be reasonable.  It's possible the code used to more or less do that back when it took const char* and null to represent "no arg", before conversion to XPCOM strings.

> This is my second issue with this code: why return nullptr if finding this fails?

That's ... a good question.  This is a behavior change introduced in bug 295202, sort of.  Before that change, we used to only condition the GetMIMEInfoFromRegistry call on this "found" boolean.  We also had a useless

      NS_IF_RELEASE(mimeInfo); // we failed to really find an entry in the registry

line that was only executed when mimeInfo was null.  I _think_ the intent there was to bail out if looking up the _type_ in the registry failed.  That would certainly match the code from before that line was introduced in bug 147679 better.

Anyway, this seems bogus and we should fix this, even though it's not the proximate issue here.

> which, to be clear, I don't think the original consumer here cares about at all

Correct.

> Which then means the NS_ENSURE_SUCCESS fails

The use of NS_ENSURE_SUCCESS here is from bug 397678.  Before that we used to just ignore GetDefaultAppInfo failure and press on.  Jim, do you know why that NS_ENSURE_SUCCESS was added?  This stuff is all supposed to be best-effort; no default app info is an OK thing to happen.

> I wouldn't be at all surprised if returning nsIMIMEInfo objects without app information as soon
> as we find a mimetype will break other consumers of ::GetMIMEInfoFromOS

It really shouldn't, I would think; see above about best-effort.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review133034

::: uriloader/exthandler/nsExternalHelperAppService.cpp:2937
(Diff revision 1)
> +nsExternalHelperAppService::GetMIMETypeFromOSForExtension(const nsACString& aExtension, nsACString& aMIMEType)
> +{
> +  bool found = false;
> +  nsCOMPtr<nsIMIMEInfo> mimeInfo = GetMIMEInfoFromOS(EmptyCString(), aExtension, &found);
> +  if (found && mimeInfo) {
> +    mimeInfo->GetMIMEType(aMIMEType);

Hmm.  So the old code would fail out here if aMimeType ended up empty after this call.  Probably best to keep that in the new code.  So something like:

    return found && mimeInfo && NS_SUCCEEDED(mimeInfo->GetMIMEType(aMIMEType));

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:444
(Diff revision 1)
>    {
> -    found = NS_SUCCEEDED(regKey->ReadStringValue(EmptyString(), 
> +    nsCOMPtr<nsIWindowsRegKey> regKey =
> +      do_CreateInstance("@mozilla.org/windows-registry-key;1");
> +    if (!regKey)
> +      return nullptr;
> +    found = NS_SUCCEEDED(regKey->ReadStringValue(EmptyString(),

This doesn't look right.  You need to Open() the regKey with the right arguments before reading stuff from it, no?

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:595
(Diff revision 1)
> +  // so make sure it's there...
> +  nsAutoString fileExtToUse;
> +  if (aExtension.First() != '.')
> +    fileExtToUse = char16_t('.');
> +
> +  fileExtToUse.Append(NS_ConvertUTF8toUTF16(aExtension));

ApppendUTF8toUTF16(), please.

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:616
(Diff revision 1)
> +  LossyAppendUTF16toASCII(temp, mimeType);
> +  aMIMEType.Assign(mimeType);

How about:

    aMimeType.Truncate();
    LossyAppendUTF16toASCII(temp, aMIMEType);
    
and possibly rename "temp" to "mimeType".

Comment 33

[Boris Zbarsky [:bzbarsky]]

Gijs, let me know if you want me to report bugs on some of the other issues comment 15 raised or whether you plan to.

Comment 34

[:Paolo Amadini]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review133154

The concept makes sense to me.

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:444
(Diff revision 1)
>    {
> -    found = NS_SUCCEEDED(regKey->ReadStringValue(EmptyString(), 
> +    nsCOMPtr<nsIWindowsRegKey> regKey =
> +      do_CreateInstance("@mozilla.org/windows-registry-key;1");
> +    if (!regKey)
> +      return nullptr;
> +    found = NS_SUCCEEDED(regKey->ReadStringValue(EmptyString(),

This looks like a code path specific to Windows XP.

We could just say it's unsupported and remove it completely, on the other hand, as this will likely make external applications completely unusable on Windows XP, you can just fix it by adding the code to open the registry key.

Comment 35

[:Paolo Amadini]

Comment on attachment 8857965 [details]
Bug 1352348 - don't do registry lookups for empty strings passed when we want to know a mimetype,

https://reviewboard.mozilla.org/r/130010/#review133158

Comment 36

[Jim Mathies [:jimm]]

(In reply to :Gijs (away until Tuesday 18) from comment #15)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> > But I probably don't have time to work on this recently...
> > 
> > The start point should be the code in
> > nsExternalHelperAppService::GetTypeFromExtension [1]. To fix this, we
> > probably need to investigate what's going wrong with the GetMIMEInfoFromOS
> > function on different systems. Probably different system needs different
> > fix, and maybe some are not affected at all...
> > 
> > Gijs, could you have a look at this issue, or help finding someone to work
> > on this?
> 
> I had a quick look. I think the issue is that all the internal APIs here are
> awful.
> 
> More seriously, AFAICT:
> 
> 1)
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#491
> nsOSHelperAppService::GetMIMEInfoFromOS supports being called with
> both/either a mimetype or a file extension. I'm... not sure why it's been
> written that way.

My guess: trying to support a single interface on numerous platforms back the late 90's.. note the "4X registry" searches, that's looking for information stored in the Windows Registry by Netscape 4.0! Hence the "dump what you have, we'll try to find some info on it" concept.

> 2) we call it with an empty string for a mimetype (of course, we want to
> find out the mime type!) and an extension. So we enter all this code:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#507-521
> 
> and try to do a registry lookup to find an extension for the mimetype "".
> Needless to say, that doesn't work very well. I think we should just check
> for that string being empty and don't bother with any of that stuff if so.
> 
> 3) later on we apparently realize we've only got an extension to go on and
> we make the 'right' call, here:
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#540-543
> 
> to GetByExtension.
> 
> This last bit is sensible, more or less.
> 
> 4) Even better, GetByExtension initially does the right thing, and this
> code:
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#412-432 correctly determines the right mimetype (!).
> 
> 5) Then we try to find the Windows descriptive name (which I'm sure is not
> the right terminology, but whatever) for the mimetype and file extension,
> using this blob of code:
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#441-471 .

Looks like this is retrieving the ProgID [1] for the default handler, not a description. The ProgID would point you to the application's registration root in HKEY_CLASSES_ROOT where you can find registration and path info for the app.

> This is my second issue with this code: why return nullptr if finding this
> fails? We still got a mimetype, and we don't actually care about any of the
> rest here...

Yeah I think you're right, we should probably return what we have (the mime string). Check the callers though because somewhere something is expecting all that extended information and may use this routine to retrieve it. Areas that use these strings: Preferences, application associations and the application picker.

> In any case, we just about manage to do this, apparently this is a
> "CERFile". Fine.
> 
> 6) Now we try to find the default app info for this file... which, to be
> clear, I don't think the original consumer here cares about at all...
>
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#473-480
> 
> 7) Unfortunately this fails. Specifically, we successfully read some more
> registry stuff:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 00a166a8640dffa2e0f48650f966d75ca3c1836e/uriloader/exthandler/win/
> nsOSHelperAppService.cpp#308-342
> 
> ... but we end up with an expanded rundll executable with some relative dll
> paths. We then strip off the rundll bits, split off some more parameters,
> and end up with "cryptext.dll", try and fail to init a file with that as its
> absolute path, and then return an error. Which then means the
> NS_ENSURE_SUCCESS fails, which means we end up bailing out of the block
> cited in (6) and return nullptr for the mimeinfo.
> 
> This means nsExternalHelperAppService gets a null nsIMIMEInfo object which
> means it won't have a mime type to use which means it falls back to
> octet-stream.

Appears GetDefaultAppInfo is expected to get that description string or fail. I wouldn't change that. GetApplicationDescription probably?

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb776336(v=vs.85).aspx

Feel free to change this code. If you make "big" changes, make sure SV does a QA pass on all supported Windows platforms looking for file association and handling issues.

Comment 37

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #34)
> This looks like a code path specific to Windows XP.
> 
> We could just say it's unsupported and remove it completely, on the other
> hand, as this will likely make external applications completely unusable on
> Windows XP, you can just fix it by adding the code to open the registry key.

My understanding is that recent (>=53) versions of Firefox won't even start on XP anymore, and we've outright removed many other compat things, so if this is really XP-specific I would sort of intend to just remove it.

The issue in terms of backwards compat, though, is that *other* apps may still store information in these locations in the registry, even on later versions of Windows. I don't know if Win7+ still honour it, but I'd bet actual money that we'll manage to break someone's workflow if we remove our code that reads it. On the other hand, if *windows* doesn't honour it (which I don't know) and we do, arguably what we're doing is a bug and we should just fix it. :-\

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #33)
> Gijs, let me know if you want me to report bugs on some of the other issues
> comment 15 raised or whether you plan to.

I can file more issues now that you've validated that I'm not talking absolute horse manure (for at least some of them), which is always a possibility when I stare at code I don't know. Leaving ni for this. Thank you also for the thorough review, it's much appreciated. :-)

Comment 38

[:Paolo Amadini]

(In reply to :Gijs from comment #37)
> My understanding is that recent (>=53) versions of Firefox won't even start
> on XP anymore, and we've outright removed many other compat things, so if
> this is really XP-specific I would sort of intend to just remove it.

In this case we can assume that the initialization of mAppAssoc always succeeds:

https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/uriloader/exthandler/win/nsOSHelperAppService.cpp#42-44

We can just remove the code paths we currently take when this is null. We should still check if the initialization succeeded, but we can fail fatally if it didn't.

> The issue in terms of backwards compat, though, is that *other* apps may
> still store information in these locations in the registry, even on later
> versions of Windows. I don't know if Win7+ still honour it, but I'd bet
> actual money that we'll manage to break someone's workflow if we remove our
> code that reads it.

Are you talking about removing some of the fallback paths we currently take if we don't find an association, that are not dead code even if mAppAssoc is not null?

The documentation doesn't mention how the association is found, so yes, there's a slight chance this may break things:

https://msdn.microsoft.com/en-us/library/windows/desktop/bb776336(v=vs.85).aspx

> On the other hand, if *windows* doesn't honour it (which
> I don't know) and we do, arguably what we're doing is a bug and we should
> just fix it. :-\

But I agree with this, for the application associations.

If you have the "4.x registry" searches in mind, I was also intrigued by that code, and checked on my local machine. There are some Office MIME types there that are *not* in the newer MIME database (but I don't have Office installed). So, strangely, removing these legacy registry reads might break the recognition of a few file extensions on some machines.

Comment 39

[:Gijs (he/him)]

Comment on attachment 8857965 [details]
Bug 1352348 - don't do registry lookups for empty strings passed when we want to know a mimetype,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130010/diff/1-2/

Comment 40

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130008/diff/1-2/

Comment 41

[:Gijs (he/him)]

I kept the XP-style checks for now, because I imagine we'll want to uplift this as far as we can, and keeping risk down seems like the right thing to do. I'll file followups tomorrow to:

1) remove the Netscape 4.x checks. I mean, seriously.
2) remove the XP era checks.
3) make things not return null when we can't find a default app / description

I already filed bug 1356253 on making our rundll code deal with the format of the executable in this particular case (x509 certs) in the appropriate manner (which would also fix this bug, but feels a lot flakier).

Keeping ni for the above.

Comment 42

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review134062

r=me

Comment 43

[:Paolo Amadini]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review134266

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ebecf0ff75be
don't do registry lookups for empty strings passed when we want to know a mimetype, r=Paolo
https://hg.mozilla.org/integration/autoland/rev/ea1bb4b8e325
add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension, r=bz,Paolo

Comment 45

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

https://reviewboard.mozilla.org/r/130008/#review134280

::: uriloader/exthandler/win/nsOSHelperAppService.cpp:395
(Diff revision 1)
> -  if (aFileExt.IsEmpty())
> +  // Determine the mime type.
> +  nsAutoCString typeToUse;
> +  if (aTypeHint && *aTypeHint) {
> +    typeToUse.Assign(aTypeHint);
> +  } else if (!GetMIMETypeFromOSForExtension(NS_ConvertUTF16toUTF8(aFileExt), typeToUse)) {

So, it's apparently possible for this code to be called with a non-empty `aTypeHint`, but an empty `aFileExt`, which is breaking tests because I removed the IsEmpty() check in this code (in favour of one in `GetMIMETypeFromOSForExtension`). See:

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ea1bb4b8e325ddca050f760e6daac572c17d3688&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=92634134

https://public-artifacts.taskcluster.net/BronMdHhSbmMqYnpQoIryg/0/public/logs/live_backing.log

which crashes when trying to use `aFileExt.First()` in here.

I'm being backed out for this, and I'll repush with a fix for this and then reland.

Comment 46

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fe00af04b969
Backed out changeset ea1bb4b8e325 
https://hg.mozilla.org/integration/autoland/rev/68e81a25651a
Backed out changeset ebecf0ff75be for crashing in e.g. browser/components/translation/test/browser_translation_yandex.js on Windows. r=backout on a CLOSED TREE

Comment 47

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130008/diff/2-3/

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d56f0474acb3
don't do registry lookups for empty strings passed when we want to know a mimetype, r=Paolo
https://hg.mozilla.org/integration/autoland/rev/1e5f2ea404fe
add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension, r=bz,Paolo

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for Windows build bustage (undeclare identifier 'aExtension' at nsOSHelperAppService.cpp(395):

https://hg.mozilla.org/integration/autoland/rev/04d4cc17d491b2feef6e71de6ceace05f28895ff
https://hg.mozilla.org/integration/autoland/rev/bd97f87743b50d6c6c8f1ec507041dd8ecb2286b

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1e5f2ea404fe3fc4ed624fe794f4a5f1f1523bdb&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
> z:/build/build/src/uriloader/exthandler/win/nsOSHelperAppService.cpp(395): error C2065: 'aExtension': undeclared identifier

Comment 50

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130008/diff/3-4/

Comment 51

[:Gijs (he/him)]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #49)
> Backed out for Windows build bustage (undeclare identifier 'aExtension' at
> nsOSHelperAppService.cpp(395):
> 
> https://hg.mozilla.org/integration/autoland/rev/
> 04d4cc17d491b2feef6e71de6ceace05f28895ff
> https://hg.mozilla.org/integration/autoland/rev/
> bd97f87743b50d6c6c8f1ec507041dd8ecb2286b

Sigh, that's embarrassing. :-(

This time, I've actually rebuilt and run some of the crashy tests...

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/22acc718c4e7
don't do registry lookups for empty strings passed when we want to know a mimetype, r=Paolo
https://hg.mozilla.org/integration/autoland/rev/8359913330fc
add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension, r=bz,Paolo

Comment 53

[:Gijs (he/him)]

OK, I think I filed everything I noticed...

Comment 55

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

https://hg.mozilla.org/mozilla-central/rev/22acc718c4e7
https://hg.mozilla.org/mozilla-central/rev/8359913330fc

Comment 56

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

I'm basically asking for approval for 54. I don't understand whether that should be aurora or beta at this point, but there we are...

Approval Request Comment
[Feature/Bug causing the regression]: bug 306471
[User impact if declined]: Firefox does not use the right mime type for files when up/downloading files
[Is this code covered by automated tests?]: not sufficiently so.
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes, see *case 2* from comment 6. The values in mimeTypes.rdf continue to not affect uploads (so this fix has no effect on the other STR), but I changed code such that Firefox will now do the right thing based on information from the Windows registry. This is a Windows-only bug. case 2 from comment 6 should now have the same results as case 1 in that comment.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: not very
[Why is the change risky/not risky?]: it's a relatively small bit of refactoring that ensures that, where we look for a mimetype string based on an extension, we do a much smaller set of work and don't give up for unrelated reasons (like whether we understand the default handler for that mimetype as listed in the registry).
[String changes made/needed]: nope

To fix this bug, only this commit needs uplift (not the second one). While the second one might not hurt, I don't see any urgent reason to uplift that one, so let's just let that ride 55 to reduce risk.

Comment 57

[Gerry Chang [:gchang]]

Hi Florin, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 58

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Moving to Brindusa so EngQA can verify it on Nightly.

Comment 59

[:Paolo Amadini]

Moved Ehsan's needinfo to bug 1358369.

Comment 60

[Brindusa Tot, DTE]

I reproduced Case2 from comment 6 on a 55.0a1 Nightly build from 04.19.2017.

Verified as fixed on Windows 10 x64 and Window 7 x64, with latest Nightly 55.0a1, Build ID 20170420030346.

Comment 62

[Gerry Chang [:gchang]]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Fix a regression that Firefox does not use the right mime type for files when up/downloading files and was verified. Beta54+. Should be in 54 beta 2.

Comment 63

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d5fe7a284cf1c77bc7a258f85f80ba80a051ae0d

Comment 64

[Ciprian Georgiu, Desktop QA]

I've managed to reproduce this issue using STR from comment 11 on an old Nightly build.

This bug is not reproducible anymore with 54 beta 2 under Windows 10 x64.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

Is this wontfix for ESR52?

Comment 66

[:Gijs (he/him)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

(In reply to Ryan VanderMeulen [:RyanVM] from comment #65)
> Is this wontfix for ESR52?

I'm honestly not sure. The effect of the regression can be pretty serious if your day-to-day browser usage involves dealing with files that now no longer get the right mimetype, and websites you upload to are strict about requiring this workaround. It's also tricky to workaround. Although we found out about this pretty late, several dupes were filed. There might be more - bugzilla is a big place. This patch essentially fixes a longstanding issue with the code and how it looks up mimetypes, that became much more pressing when we stopped using mimeTypes.rdf for uploads. I expect there's likely an older dupe of the issue the patch addresses. As a bonus, the new code performs significantly better for this usecase (see bug 1358369).

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Some files will upload with the wrong mimetype (which the server may then use to refuse to accept the upload). This can seriously interfere with people's workflow.
User impact if declined: see above
Fix Landed on Version: 55 uplifted to 54
Risk to taking this patch (and alternatives if risky): reasonably low. The fix has already been verified by QE for both 55 and 54, and the change is relatively small.

One potential area of risk is if there are code/add-on consumers that rely on this codepath not returning a mimetype if there is no (valid) default handler application for the mimetype. This seems very unlikely, and as Boris noted at the very bottom of comment #31, would be relying on an implementation detail in a way that they really shouldn't, but I can't preclude it from being the case.

String or UUID changes made by this patch: none

Comment 67

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8857964 [details]
Bug 1352348 - add a method to nsExternalHelperAppService that only fetches a string mimetype for an extension,

Planning to take this one on ESR52.2, seems like a regression that ESR users will be glad we fixed

Gijs, is this fix worth relnoting in ESR52? Normally ESR release notes just point to the sec advisories.

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1d86e96610a1

Comment 69

[:Gijs (he/him)]

(In reply to Ritu Kothari (:ritu) from comment #67)
> Gijs, is this fix worth relnoting in ESR52? Normally ESR release notes just
> point to the sec advisories.

Probably yes, as it's a more general fix to mimetypes and people might see other side effects. Good ones, but still.

Maybe something generic like "Improved file type recognition on Windows"? Not sure if using "MIME" in the release notes would be too technical.

Bouncing ni to make sure this doesn't get lost. :-)

Comment 70

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Sounds good. Thanks Gijs. Leaving the NI in place so I don't forget. Will clear it once I draft the esr relnotes in ~3 weeks.

Comment 71

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to ESR52.2.0 release notes.

Comment 72

[nassifrroma]

Sorry to revive this topic, but we're having exactly this issue on Firefox 92.0.1 (64-bits) with MacOSX 11.4 and "p7s" type files.

The mimetype expected is "application/pkcs7-signature" or "application/pkcs7-signed-data", but we get "application/octet-stream".

On Windows and Linux it's ok, only on MacOSX it's a problem.

Comment 73

[nassifrroma]

Attachment: 77fba055-2192-4194-8a72-471c2ce98e78.zip

Example of "p7s" type file

Comment 74

[:Gijs (he/him)]

(In reply to nassifrroma from comment #72)

Sorry to revive this topic, but we're having exactly this issue on Firefox 92.0.1 (64-bits) with MacOSX 11.4 and "p7s" type files.

The mimetype expected is "application/pkcs7-signature" or "application/pkcs7-signed-data", but we get "application/octet-stream".

On Windows and Linux it's ok, only on MacOSX it's a problem.

Please file a new bug with more details (are you uploading / downloading? If the latter, from what website? Does the same issue reproduce on a new clean Firefox profile ( https://support.mozilla.org/kb/profile-manager-create-remove-switch-firefox-profiles ) ? etc.

This bug is closed and addressed the original problem, so we don't normally reopen it, and anyway it fixed a Windows issue so the macOS issue will likely require a different fix.