Make sanitize_non_media_extensions pref work
Categories
(Firefox :: File Handling, defect)
Tracking
()
People
(Reporter: evilpie, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
This pref didn't actually work. Fix it and a test.
Assignee | ||
Comment 1•3 years ago
|
||
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
Comment 3•3 years ago
|
||
Backed out for causing failures on browser_bug676619.js.
Backout link: https://hg.mozilla.org/integration/autoland/rev/097098bb8ec2e2db1ca12ac38b9cbe1fdb50bb41
Failure log: https://treeherder.mozilla.org/logviewer?job_id=328487751&repo=autoland&lineNumber=2805
Assignee | ||
Comment 4•3 years ago
|
||
Not sure what the problem is. I have this theory that on Windows we might not get the right Content-Type for pdf files.
Assignee | ||
Comment 5•3 years ago
|
||
I pushed an alternative test with an explicit Content-Type header to try: https://treeherder.mozilla.org/jobs?repo=try&revision=790fbd168d6aa7d3547084511356ec11c987336d.
Assignee | ||
Comment 6•3 years ago
|
||
Can you take a look? I don't want to debug windows.
https://treeherder.mozilla.org/jobs?repo=try&revision=790fbd168d6aa7d3547084511356ec11c987336d&selectedTaskRun=G7grsKCUSACZkkDQku36sQ.0
Comment 7•3 years ago
|
||
(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. :-\
Comment 8•3 years ago
|
||
So the issue is we hit:
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...
Assignee | ||
Comment 9•3 years ago
|
||
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.
Comment 10•3 years ago
|
||
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
Comment 11•3 years ago
|
||
(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...
Assignee | ||
Comment 12•3 years ago
|
||
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.
Comment 13•3 years ago
|
||
(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.
Assignee | ||
Updated•3 years ago
|
Comment 14•3 years ago
|
||
bugherder |
Assignee | ||
Comment 16•3 years ago
|
||
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:
Comment 17•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 18•3 years ago
|
||
bugherder uplift |
Description
•