Closed Bug 1690051 Opened 2 years ago Closed 2 years ago

Make sanitize_non_media_extensions pref work

Categories

(Firefox :: File Handling, defect)

defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox86 --- fixed
firefox87 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This pref didn't actually work. Fix it and a test.

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0aab2ac0976a
Make browser.download.sanitize_non_media_extensions work and test it. r=Gijs

Not sure what the problem is. I have this theory that on Windows we might not get the right Content-Type for pdf files.

I pushed an alternative test with an explicit Content-Type header to try: https://treeherder.mozilla.org/jobs?repo=try&revision=790fbd168d6aa7d3547084511356ec11c987336d.

Flags: needinfo?(evilpies)
Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Tom Schuster [:evilpie] from comment #6)

Can you take a look? I don't want to debug windows.

I should have been just as reluctant, it took me until now to get a working windows build again after updating my m-c copy. :-\

So the issue is we hit:

https://searchfox.org/mozilla-central/rev/b2433a832c250c55255e0ee37d05192d04f20427/uriloader/exthandler/nsExternalHelperAppService.cpp#851

which looks up a mime info object for the text/calendar mimetype and the not-ics extension.

We first ask the OS. The OS doesn't provide an extension for text/calendar, so this code doesn't find us an extension. We end up hitting all of this code which returns a mime info object with the mimetype and the provided extension (.not-ics).

When we then fill the mime type object with the info from "extras", the provided extension (.not-ics) is still there.

I don't know why the PDF code would have failed; I'd have expected the code to be able to find a pdf file extension... leaving ni for that, I guess.

The fix for this ics issue is probably to allow the GetExtensionFromWindowsMimeDatabase to consult the "extra" info array, but I don't know what other side effects pulling that string of this gordian knot would have...

Wait does that mean this code didn't work on Windows at all? That can't be true, we got bug reports. I am going to land this patch with the test on Windows disabled.

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b6ee0010506e
Make browser.download.sanitize_non_media_extensions work and test it. r=Gijs

(In reply to Tom Schuster [:evilpie] from comment #9)

Wait does that mean this code didn't work on Windows at all? That can't be true, we got bug reports.

It would work for mimetypes that were recognized in the Windows registry of those users. This is not the case for text/calendar on the Win10 VMs on which our tests run, and my machine. As I said, I think there must be something else going on for the PDF implementation you had before.

I am going to land this patch with the test on Windows disabled.

This seems unfortunate...

Maybe Windows 7 doesn't have a default association for PDF? I remember having to install a PDF viewer when I was using Windows 7. We should just file a follow up bug here.

(In reply to Tom Schuster [:evilpie] from comment #12)

Maybe Windows 7 doesn't have a default association for PDF?

Ah, if the PDF issues were windows-7 specific, then yes that seems like a plausible explanation.

We should just file a follow up bug here.

OK. Would you mind doing that? I can probably take it.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(evilpies)
Blocks: 1690539
Flags: needinfo?(evilpies)
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

Do we want to beta uplift this?

Flags: needinfo?(evilpies)

Comment on attachment 9200479 [details]
Bug 1690051 - Make browser.download.sanitize_non_media_extensions work and test it. r?Gijs

Beta/Release Uplift Approval Request

  • User impact if declined: No direct impact. However we added this pref to unship this code when problems like bug 1688306 happen. This simple fix would make it possible to provide users a workaround.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:
Flags: needinfo?(evilpies)
Attachment #9200479 - Flags: approval-mozilla-beta?

Comment on attachment 9200479 [details]
Bug 1690051 - Make browser.download.sanitize_non_media_extensions work and test it. r?Gijs

Approved for 86 beta, thanks.

Attachment #9200479 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.