Bugzilla

Comment 7

•

8 years ago

(Moving over discussion from bug 373621 as that's now closed and has a gazillion people in the CC list, and this is the first not-pdf-related dupe I could find to reopen, per requests in bug 373621 to reopen non-pdf-related dupes.)

(In reply to Boris Zbarsky [:bz] (Vacation until Aug 21) from bug 373621 comment #77)
> Comment on attachment 8775534 [details]
> Bug 373621 - Make mime-type of PDF not overridable.
> 
> https://reviewboard.mozilla.org/r/67714/#review64830
> 
> I guess....  I really wish someone would actually pick up ownership of this
> junk and fix bug 332690 instead of having to play whack-a-mole.  :(
>  But the front end team seems to think the front end writing garbage to mimeTypes.rdf
> is not their problem somehow.  :(

Yes, but that requires time. This set of bugs has been in my list of "if I had time, I would look at fixing this" for well over a year now. But the list is long and I rarely have (enough - this is not a trivial issue) time, nor do most of the other folks on the team I'm on. There are more problems to address than engineers to do so. That applies to everybody.

I would love to get this thing sorted, but it's not clear to me that it's as simple as "the front end writing garbage to mimeTypes.rdf".

(In reply to Boris Zbarsky [:bz] (Vacation until Aug 21) from bug 373621 comment #85)
> [W]e have hooks into the OS filetype support and fall back to those
> if there is nothing overriding them.  The mimetypes file allows thing like a
> Firefox user selecting a different app than the OS-default for the given
> filetype and having that information be saved.  It also used to allow (not
> sure whether it still does) explicitly adding extension to type and type to
> helper app mappings via the Firefox preferences.
> 
> Arguably, we should skip looking in mimeTypes.rdf when doing the
> extension-to-type mapping, maybe.  Especially if there is no longer UI for
> adding such mappings directly.

The only direct UI Firefox has is mapping from mime/protocol -> handling in about:preferences ("Applications" section). Protocols and mime types (using their labels, e.g. "AVI file" or "Document (text/x-python)") are shown in a list with a corresponding dropdown to indicate what action Firefox should take. It does not let you edit the file<->mime mapping directly. Note that folks in Taipei are looking at simplifying the UI for this still further, and the option to pick something other than "open with your OS default" or "save to file" or "open with this Firefox add-on / Firefox itself" is likely to go away, which may or may not render some/all of this discussion moot...

Ignoring the mapping from extensions to mimetypes in mimetypes.rdf would fix the issue with form upload mime types, but AIUI just removing that lookup would break the "remember what to do with this type of file" logic if the server provides a broken mimetype (or none at all), but does provide an extension, if your OS does not provide a useful mimetype for the extension. Maybe that's an edgecase we shouldn't worry about? Maybe we could prioritize the OS lookup and only use the mimeTypes.rdf data for extensions that the OS doesn't know? Xidorn, do you think one of these options would work and/or do you have cycles to write such a patch?

Paolo, can you remind me what the tracking bugs are for the work to the applications pane in the prefs and the new download flow?

Status: RESOLVED → REOPENED

Ever confirmed: true

Flags: needinfo?(xidorn+moz)

Flags: needinfo?(paolo.mozmail)

Resolution: DUPLICATE → ---

Summary: Upload setting wrong Content-Type for mp3 files → Upload setting wrong Content-Type for files if you downloaded files with such an extension from a server that provided the wrong Content-Type for them

Updated

•

8 years ago

Comment 13

•

8 years ago

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #7)
> Ignoring the mapping from extensions to mimetypes in mimetypes.rdf would fix
> the issue with form upload mime types, but AIUI just removing that lookup
> would break the "remember what to do with this type of file" logic if the
> server provides a broken mimetype (or none at all), but does provide an
> extension, if your OS does not provide a useful mimetype for the extension.
> Maybe that's an edgecase we shouldn't worry about?

With the current logic, if the server provides a broken mimetype (or none at all) but does provide an extension, and your OS does not provide a useful mimetype for the extension, the association would work *only* if you *previously* downloaded from a (probably different) site that provided the correct type for that extension. I don't see how this can be particularly useful. This is best explained in bug 332690 comment 13.

Apparently we can also shortcut the "type --(through HTTP response)--> extension --(through OS)--> description" associations to an incorrect "type --(through mimeTypes.rdf)--> description" association as illustrated in bug 649321 comment 0.

> Paolo, can you remind me what the tracking bugs are for the work to the
> applications pane in the prefs and the new download flow?

The meta-bug for the content flow enhancements is bug 1270405, but more relevant to this discussion is the mimeTypes.rdf conversion meta-bug which is bug 474043.

I've asked the team in bug 1287658 to complete the analysis of the nsIHandlerService API before starting the implementation work, because I suspect that the API structure may be playing a role in the bugs we are discussing here, and if we fix these bugs then most of the conversion and migration work the team was originally planning may become irrelevant.

Flags: needinfo?(paolo.mozmail)

Assignee

Comment 14

•

8 years ago

I've had a simple patchset which moves handler service to the last one to check in nsExternalHelperAppService::GetTypeFromExtension. I think that way makes sense because users aren't really able to set anything there, and sources like OS, plugins, and extensions are considered more reliable than random content-type from the Internet.

Also, user should in general be able to set file type in the OS, and disagreement between the OS and the browser about filetype doesn't sound like something sensible.

But given Paolo just said that the team has started working on a more fundamental solution, I'm not going to take this bug for now. I'll upload the patches without requesting review.

Flags: needinfo?(xidorn+moz)

Comment hidden (mozreview-request)

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

Comment hidden (mozreview-request)

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

Comment 17

•

8 years ago

Maybe we should ignore mimeTypes.rdf in GetTypeFromExtension entirely.

Something we could easily gather is keyed telemetry to see how often we hit each case for each type, but some more thought is needed to understand if this telemetry would actually help with this decision.

Comment 18

•

8 years ago

Ah, I also think that in the early days add-ons used to write to mimeTypes.rdf to create associations, but we have the "ext-to-type-mapping" category for that.

Assignee

Comment 19

•

8 years ago

I thought using mimeTypes.rdf as a last resort might make sense, but yeah, probably just ignoring it is even better than relying on random thing from the Internet...

Comment 20

•

8 years ago

> but it's not clear to me that it's as simple as "the front end writing garbage to mimeTypes.rdf".

Oh, it's certainly not.

> Maybe we should ignore mimeTypes.rdf in GetTypeFromExtension entirely.

That is the semi-conclusion I came to in bug 373621 comment 85 as well.  See the last paragraph of that comment.

Comment 21

•

8 years ago

(In reply to :Paolo Amadini from comment #13)
> I've asked the team in bug 1287658 to complete the analysis of the
> nsIHandlerService API before starting the implementation work, because I
> suspect that the API structure may be playing a role in the bugs we are
> discussing here, and if we fix these bugs then most of the conversion and
> migration work the team was originally planning may become irrelevant.

OK, but based on the last few comments, bz, xidorn, you and me all think that removing or reordering the mimeTypes.rdf lookup from the GetTypeFromExtension method is the right thing. I don't know what the timeline is for the Taipei team moving forward with that bug and replacing mimeTypes.rdf with a JSON thing (but that sounds like a lot of work!), and we effectively have a patch in hand here.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #14)
> But given Paolo just said that the team has started working on a more
> fundamental solution, I'm not going to take this bug for now. I'll upload
> the patches without requesting review.

Personally, if the patch works, I would prefer we get it reviewed and landed now, rather than waiting for the "refactor the world and fix all the bugs" solution, which IME tend to take longer. :-)

(Though, from a brief look at the patch, I think it would be super useful to have a regression test. If you think that's a lot of work, please split it off to a separate bug and I or someone from the Taipei team can look at it - a mochitest-browser test with some sjs magic to serve bogus mimetypes should be relatively doable, I think/hope.)

Xidorn/Paolo, would you agree with taking this patch rather than waiting, or am I missing something?

Flags: needinfo?(xidorn+moz)

Flags: needinfo?(paolo.mozmail)

Comment 22

•

8 years ago

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #21)
> Xidorn/Paolo, would you agree with taking this patch rather than waiting, or
> am I missing something?

I'm fine with just removing the mimeTypes.rdf lookup in GetTypeFromExtension.

We can keep on the lookout for regressions as this rides the trains, although I suspect it may fix more cases than it breaks.

Flags: needinfo?(paolo.mozmail)

Comment hidden (mozreview-request)

Comment on attachment 8780480 [details]
Bug 306471 part 1 - Some code style cleanup for nsExternalHelperAppService::GetTypeFromExtension.

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

Comment hidden (mozreview-request)

Comment on attachment 8780481 [details]
Bug 306471 part 2 - Do not query handler service for content-type of file extension.

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

Assignee

Comment 25

•

8 years ago

Patch updated, but it seems bz isn't accepting review request at this moment.

Flags: needinfo?(xidorn+moz)

Comment 26

•

8 years ago

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> Patch updated, but it seems bz isn't accepting review request at this moment.

He's on PTO - based on hg log, you could ask paolo, jchen, or jdm for review as well.

(In reply to :Paolo Amadini from comment #22)
> We can keep on the lookout for regressions as this rides the trains,
> although I suspect it may fix more cases than it breaks.

Yes, that's my suspicion as well.

Assignee

Comment 27

•

8 years ago

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #26)
> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #25)
> > Patch updated, but it seems bz isn't accepting review request at this moment.
> 
> He's on PTO - based on hg log, you could ask paolo, jchen, or jdm for review
> as well.

Hmmm, but Modules/Core shows only bz, smaug, and jst can review files under uriloader... Probably that page should be updated, I don't know :/

Comment 28

•

8 years ago

Xidorn, are these patches waiting for review from me?  mozreview seems to think they have review requested from no-one or something...

Flags: needinfo?(xidorn+moz)

Assignee

Updated

•

8 years ago

Attachment #8780480 - Flags: review?(bzbarsky)

Attachment #8780481 - Flags: review?(bzbarsky)

Assignee

Comment 29

•

8 years ago

Yes, they are waiting for review from you :)

Assignee: nobody → xidorn+moz

Flags: needinfo?(xidorn+moz)

Comment 30

•

8 years ago

mozreview-review

Comment on attachment 8780480 [details]
Bug 306471 part 1 - Some code style cleanup for nsExternalHelperAppService::GetTypeFromExtension.

https://reviewboard.mozilla.org/r/71186/#review71456

r=me

Attachment #8780480 - Flags: review?(bzbarsky) → review+

Comment 31

•

8 years ago

mozreview-review

Comment on attachment 8780481 [details]
Bug 306471 part 2 - Do not query handler service for content-type of file extension.

https://reviewboard.mozilla.org/r/71188/#review71458

::: uriloader/exthandler/nsExternalHelperAppService.cpp
(Diff revision 2)
>        aContentType = entry.mMimeType;
>        return NS_OK;
>      }
>    }
>  
> -  // Check user-set prefs

Probably a good idea to clearly document that we are purposefully not looking at nsIHandlerService.

Comment 32

•

8 years ago

mozreview-review

Comment on attachment 8780481 [details]
Bug 306471 part 2 - Do not query handler service for content-type of file extension.

https://reviewboard.mozilla.org/r/71188/#review71460

The commit message should say "Do not query" instead of "Not query".

r=me

Attachment #8780481 - Flags: review?(bzbarsky) → review+

Comment hidden (mozreview-request)

Comment on attachment 8780480 [details]
Bug 306471 part 1 - Some code style cleanup for nsExternalHelperAppService::GetTypeFromExtension.

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

Comment hidden (mozreview-request)

Comment on attachment 8780481 [details]
Bug 306471 part 2 - Do not query handler service for content-type of file extension.

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

Pulsebot

Comment 35

•

8 years ago

Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/202cf4e4a2f4
part 1 - Some code style cleanup for nsExternalHelperAppService::GetTypeFromExtension. r=bz
https://hg.mozilla.org/integration/autoland/rev/cc15f60b0838
part 2 - Do not query handler service for content-type of file extension. r=bz

Ryan VanderMeulen [:RyanVM]

Comment 36

•

8 years ago

Marking addon-compat and dev-doc-needed because if an add-on uses mimeTypes.rdf to provide a file extension to MIME type mapping it should now register an entry in the "ext-to-type-mapping" category.

Keywords: addon-compat, dev-doc-needed

Comment 37

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/202cf4e4a2f4
https://hg.mozilla.org/mozilla-central/rev/cc15f60b0838

Status: REOPENED → RESOLVED

Closed: 16 years ago → 8 years ago

status-firefox51: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → Firefox 51