Closed
Bug 1437281
Opened 6 years ago
Closed 6 years ago
OSX dragging image to desktop changes OSX File associations
Categories
(Core :: Security: Process Sandboxing, defect, P3)
Tracking
()
VERIFIED
FIXED
mozilla61
People
(Reporter: mikeyt, Assigned: haik)
References
(Regressed 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files)
117.01 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
nika
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:58.0) Gecko/20100101 Firefox/58.0 Build ID: 20180128191252 Steps to reproduce: Select a JPG file on the desktop (OSX 10.10.5). Select GET INFO. File association shows "Open with: Photoshop.app" Go to firefox, drag image (jpg) from a website to tge desktop. Select GET INFO on the new file. File association now shows "Open with: Preview.app". Actual results: By dragging the file to the desktop from Firefox the file associations (for ALL jpg files) are changed to open in Apple Preview, overriding any saved settings in OSX. Incidentally, it also causes the file association for Photoshop PSD files to change so Adobe Illustrator instead of Photoshop. Using fsevents I can see that when dragging the file to the desktop, a change is made to the com.apple.launchservices-1070501.csstore (stored in Private/Var/Folder/51/xxxxxx/C folder) so I assume that this is responsible for the OSX file associations being changed. Expected results: Firefox should not be overriding and permanently changing OSX file associations. This does not happen when using OSX 10.12.5
Further comments. This issue also happens in OSX 10.9, but does not occur in OSX 10.12 or 10.13. Unknown as to 10.11.
Component: Untriaged → Widget: Cocoa
OS: Unspecified → Mac OS X
Product: Firefox → Core
Hardware: Unspecified → x86
Comment 2•6 years ago
|
||
This sounds like a macOS bug that was fixed in 10.12, possibly 10.11. Could you run mozregression[1] to see if this is a regression in Firefox? If you have never run mozregression before, simply run these three commands in a Terminal window: sudo easy_install pip sudo pip2 install -U mozregression mozregression A number of Firefox versions will open in succession to narrow down when this started occurring. Simply type "good" or "bad" in Terminal based on whether or not a build reproduces the bug. [1] https://mozilla.github.io/mozregression/
Flags: needinfo?(mikeyt)
Priority: -- → P3
I can confirm this issue happening also with macOS 10.13.3 (17D47) and FF 58.0.2
There is a discussion regarding this issue on the apple discussion boards - seems that many people are having this problem with Firefox 58 on all versions of OSX. The only solution so far seems to be removing and not using FF any more. https://discussions.apple.com/message/33052869?
Flags: needinfo?(mikeyt)
Comment 5•6 years ago
|
||
I don't have Photoshop installed. Does this reproduce if files are set to open in any other app? Or does this only affect Adobe products? I have been unable to reproduce this with apps such as Paintbrush, for example. If files are set to open in Paintbrush, they continue to be opened by Paintbrush after images are dragged from Firefox to the desktop.
Flags: needinfo?(yodolphe)
Flags: needinfo?(mikeyt)
I can confirm the following changes to file type associations happening: .jpg Association BEFORE drag to desktop ---> AFTER ----------------------------------- Adobe Photoshop CS5.app ---> Apple Preview.app ----------------------------------- Adobe Photoshop CS5.app ---> Adobe Illustrator CS5.app (has happend also, i don't know if under special circumstances): and also ----------------------------------- Adobe Photoshop CS5.app ---> Adobe Fireworks CS5.app has occured. ----------------------------------- Notably, also at least .png files had their association changed to Apple Preview.app (formerly also Photoshop CS5.app), though only a .jpg has been dragged from FF to the desktop. Hope these observations are helpful, though not systematic.
Flags: needinfo?(yodolphe)
Comment 7•6 years ago
|
||
Thanks! Could you try setting the .jpg association to Paintbrush.app and see if it changes after dragging an image to the desktop?
Flags: needinfo?(yodolphe)
Yes, .jpg association also changes from Paintbrush.app to Apple Preview.app after dragging an image to the desktop. No Adobe involved here!?
Oh, and probably also worth noting: These weird association changes all revert back to their inital settings after rebooting or logging out and back in macOS. So these changes are only "temporary". macOS seems to remember the actual settings.
Reporter | ||
Comment 10•6 years ago
|
||
The fact that the change is only temporary is supported by the fact that when dragging a file to the desktop a change is made to a temporary file, namely: com.apple.launchservices-1070501.csstore stored in Private/Var/Folder/51/xxxxxx/C folder (the 1070501 is different for each user) rather than the com.apple.launchservices.plist in user/Library/Preferences which is permanent.
Flags: needinfo?(mikeyt)
Comment 11•6 years ago
|
||
Are you able to reproduce on a system without Adobe products installed, if you have one available to test this on? Are you able to reproduce in a new Firefox profile? Are you able to reproduce with a different macOS user account? Can you run older versions of Firefox to see if this worked before? You may choose to use mozregression for this (see comment 2).
Flags: needinfo?(mikeyt)
Comment 12•6 years ago
|
||
I am *NOT* able to reproduce with FF 58 on macOS 10.12 system with no Adobe products at all. No issues here. jpgs files dragged to desktop stay associated with Paintbrush.app respectively Preview.app, whatever the setting was. I am *able* to reproduce with FF 58 on macOS 10.11 (with Adobe CS 5 installed) with a clean fresh Firefox profile. Same issue, file association losst after dragging to desktop (Adobe Photoshop.app —> Adobe Fireworks.app) I am *able* to reproduce with FF 58 on macOS 10.11 (with Adobe CS 5 installed) with a different user accout („Guest“). Same issue, file association losst after dragging to desktop (Preview.app —> Adobe Fireworks.app) Regarding older Firefox version, i can just report that this behaviour did not occur before Quantum.
Flags: needinfo?(yodolphe)
Comment 13•6 years ago
|
||
Thank you for the extra info! (In reply to yodolphe from comment #12) > Regarding older Firefox version, i can just report that this behaviour did > not occur before Quantum. Would you be able and willing to use mozregression (see comment 2) to check when this started happening? It would be great to figure out if there was a point in time when code changed in Firefox that made this issue surface, or if there was an update to Adobe CS 5 which triggered this issue. The fix might still come as a fix to Firefox, but using mozregression would help us figure this out. Thanks in advance!
Flags: needinfo?(yodolphe)
Reporter | ||
Comment 14•6 years ago
|
||
I have now discovered that as well as this issue being caused when dragging a file to the desktop, the same problem with file associations being changed happens when I upload an image to a website that I use. The image upload is done via an upload button on the website homepage at https://www.vivyxprinting.com (big blue button that says UPLOAD IMAGES. Every time I upload a jpeg file to that website all file associations are changed for jpeg and psd documents. It's a html5 button which calls a .js to upload the file. I've only noticed this issue with this upload button on that specific website. Doesn't happen on any other websites I use, but I assume they use different methods for uploading files. This behaviour did not happen with any versions of Firefox previous to Quantum. I use Adobe CS6 products. There have been no updates to CS5 or CS6 products for a number of years so any updates to those products can be ruled out as a cause of the issues. I'll hopefully be able to do a mozregression test in the next couple of days, but anecdotally this only started with Firefox Quantum, did not happen with any Firefox 56 and below.
Flags: needinfo?(mikeyt)
Reporter | ||
Comment 15•6 years ago
|
||
Further investigation shows that when I use the upload button on the website previously mentioned, Firefox makes a change to the com.apple.launcheservices-107501.csstore file - this is the same temp system file that gets changed when dragging an image to the desktop. File location is in: /private/var/folders/51/xxxxxxxxxxxxx/0/ The file does not get changed, for example, when uploading an image via google image search image upload. The vivyx website uses a javascript uploader called plupload.js so Firefox interaction with this .js appears to be causing the same behaviour as dragging a file to the desktop in changing the above temp file for file association.
Comment 16•6 years ago
|
||
I've noticed this problem under OSX 10.13.3/FF 58 using right click > Save Image As... Under OSX 10.12.6 the problem is not there. Both Macbook Pros are Mid 2012 models. It's a very random thing and I will notice Preview.app instead of Photoshop CC 2018 as the default. Only by not launching FireFox have I kept Photoshop as the default "Open With" for images. I've had Firefox for as long as I can remember and really don't use Chrome or Safari except as backup. this problem is killing a very valuable tool in my workflow.
Reporter | ||
Comment 17•6 years ago
|
||
I have found a workaround by installing an OSX preference pane called MAGIC LAUNCH. This app allows you to set rules to what opens JPG and PSD files. Once done, the JPG and PSD files in the GET INFO show MAGIC LAUNCH as the application to open the files. For whatever reason, it then stops FF from changing the file associations of those files when they are dragged/save as/uploaded. When MAGIC LAUNCH is actiavted, no change is made by FF to the temporary file: com.apple.launcheservices-107501.csstore
Comment 18•6 years ago
|
||
Thanks, I will look into that.
Comment 19•6 years ago
|
||
Ok, I can *not* reproduce the problem when just using right click > Save Image As..., here on FF 58 on macOS 10.12. It only happens when dragging out from FF. Thanks for workaround tip!
Reporter | ||
Comment 20•6 years ago
|
||
MOZREGRESSION RESULTS: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ba8db0fbc00605d6d097dde0b7e034297f55c1ec&tochange=6b101438c684bff925471edbfe593e500bcb3a03 17:50.20 INFO: Narrowed inbound regression window from [ba8db0fb, db5209b5] (3 builds) to [ba8db0fb, 6b101438] (2 builds) (~1 steps left) 17:50.20 INFO: No more inbound revisions, bisection finished. 17:50.20 INFO: Last good revision: ba8db0fbc00605d6d097dde0b7e034297f55c1ec 17:50.20 INFO: First bad revision: 6b101438c684bff925471edbfe593e500bcb3a03 17:50.20 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ba8db0fbc00605d6d097dde0b7e034297f55c1ec&tochange=6b101438c684bff925471edbfe593e500bcb3a03
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to Stephen A Pohl [:spohl] from comment #21) > Haik, any thoughts? This sounds a lot like bug 1391186 "Thunderbird loses setting as default email client when "mailto" triggered by Firefox 56/57" where a call to LSGetApplicationForURL() from a sandboxed process calls the OS to reset the URL handler back to the default. That was fixed in 57 and the fix was not specific to mail. See https://bugzilla.mozilla.org/show_bug.cgi?id=1391186#c16 for some more details. This is probably a variation of that same problem where just looking up the target application from content causes the handler to revert to the default. I'll work on getting this addressed ASAP.
Assignee: nobody → haftandilian
Component: Widget: Cocoa → Security: Process Sandboxing
Flags: needinfo?(haftandilian)
Assignee | ||
Comment 23•6 years ago
|
||
Michael, For a workaround, browsing to about:config and setting "security.sandbox.content.level" to 2 (instead of the current default value of 3) and then restarting your browser may work. This makes the security sandbox more permissive, but doesn't completely disable it. I haven't reproduced the problem so I can't confirm if this works around the problem or not. I've tried on 10.13 and will test on some other versions.
Flags: needinfo?(mikeyt)
Reporter | ||
Comment 24•6 years ago
|
||
I can confirm that changing that setting to 2 works on 10.10.5 and the file associations are no longer changed when dragging.
Flags: needinfo?(mikeyt)
Comment 25•6 years ago
|
||
Thank you guys! Can confirm too, setting "security.sandbox.content.level" to 2 prevents that problem from occuring in macOS 10.11.6, FF 58.0.2 Will this change from default setting persist in future updates to FF?
Assignee | ||
Comment 26•6 years ago
|
||
Great. Thank you both for testing the workaround. The setting should persist across updates. Keep in mind that the setting is a temporary workaround and once we have the fix in the browser it should be reverted.
See Also: → 1439680
Assignee | ||
Comment 27•6 years ago
|
||
tl;dr, drag-and-drop triggers a call to LSCopyApplicationForMIMEType()[1] to lookup the handler application for a given MIME type. That fails when called from our sandboxed content processes due to filesystem restrictions which has a side effect of reverting the assigned handler to the default. We shouldn't call LSCopyApplicationForMIMEType() from content processes because it depends on file system access. We need to e10s-ify this. I was able to reproduce this on 10.13.3 and found that the call we make to LSCopyApplicationForMIMEType()[1] fails with kLSApplicationNotFoundErr (-10814) and causes the association for the MIME type to be wiped out. I tested with a JPG file displayed in a content process and saw that private LaunchServices code was trying to read from the application bundle (I had configured JPG files to be opened by a third party app installed in /Applications). The reads from /Applications fail because the content process does not have read access to the /Applications directory. The failing call to LSCopyApplicationForMIMEType() has a side effect of wiping out the default association which I confirmed by stepping through our code in the debugger. I had to manually reset the association after attempting the drag and drop. I did not test rebooting. As soon as a drag is started for an image displayed in a content process, we call LSCopyApplicationForMIMEType() with the stack below. I edited out most of the stack for brevity. We need to refactor how we get the default application for a given MIME type because we prevent content processes from accessing the filesystem for security/isolation and apparently that breaks this call. I tested with /Applications added to our sandbox profile and could not reproduce the problem. In this case, it seems not entirely unreasonable to allow access to /Applications (it would just allow a compromised content process to learn which applications are installed). However, there is no requirement that applications be installed in /Applications and a user could have applications in their home directory which we wouldn't want to allow access to. (lldb) bt * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 * frame #0 LaunchServices`LSCopyApplicationForMIMEType() frame #1 XUL`nsOSHelperAppService::GetMIMEInfoFromOS() at nsOSHelperAppService.mm:338 frame #2 XUL`nsExternalHelperAppService::GetFromTypeAndExtension() at nsExternalHelperAppService.cpp:2562 frame #3 XUL`nsOSHelperAppService::GetFromTypeAndExtension() at nsOSHelperAppService.mm:221 frame #4 XUL`non-virtual thunk to nsOSHelperAppService::GetFromTypeAndExtension(...)() at nsOSHelperAppService.mm:0 frame #5 XUL`DragDataProducer::Produce() at nsContentAreaDragDrop.cpp:588 frame #6 XUL`nsContentAreaDragDrop::GetDragData() at nsContentAreaDragDrop.cpp:132 ... frame #19 XUL`mozilla::dom::TabChild::HandleRealMouseButtonEvent() at TabChild.cpp:1734 frame #20 XUL`mozilla::dom::TabChild::ProcessPendingCoalescedMouseDataAndDispatchEvents() at TabChild.cpp:1576 frame #21 XUL`mozilla::dom::CoalescedMouseMoveFlusher::WillRefresh() at CoalescedMouseData.cpp:77 frame #22 XUL`nsRefreshDriver::Tick() at nsRefreshDriver.cpp:1886 ... frame #60 plugin-container`content_process_main() at plugin-container.cpp:50 frame #61 plugin-container`main() at MozillaRuntimeMain.cpp:25 frame #62 plugin-container`start() 1. https://developer.apple.com/documentation/coreservices/1448586-lscopyapplicationformimetype?language=objc Note: LSCopyApplicationForMIMEType is marked as deprecated, but its recommended replacement LSCopyDefaultApplicationURLForContentType is not available on 10.9 which we still support.
Comment 28•6 years ago
|
||
(In reply to yodolphe from comment #19) > Ok, I can *not* reproduce the problem when just using right click > Save > Image As..., here on FF 58 on macOS 10.12. It only happens when dragging out > from FF. > Thanks for workaround tip! That is correct. The context menu "save image as" problem does not show up in my 10.12.6 mid 2012 MBP using Photoshop CC 2018. I do not drag images outside of FF, because I need the Save dialog to direct saved images to folders and cloud services. The problem shows up in my mid-2012 MBP running 10.13.3 I've changed "security.sandbox.content.level" to 2 and I will see if my image file associations will hold to CC 2018 using "Ctrl + click > Save Image as method. I hope to find out if that works, but it will take an average of two daays to know if the problem repeats even with that setting. Thanks for everyone's hard work on this. I simply know something is wrong, but not exactly why. Gene
Assignee | ||
Comment 29•6 years ago
|
||
The replacement for the deprecated LSCopyApplicationForMIMEType() is LSCopyDefaultApplicationURLForContentType() (10.10+). I tested using LSCopyDefaultApplicationURLForContentType() but it also caused the side effect of wiping out the registered application handler. I'm testing a different fix that changes drag-and-drop code in DragDataProducer::Produce() in nsContentAreaDragDrop.cpp. The fix introduces a sync message to be used on Mac to ask the parent to a) check if the filename extension corresponds to the MIME type and b) return the MIME primary extension.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (obsolete) |
Assignee | ||
Updated•6 years ago
|
Attachment #8955324 -
Flags: review?(bugs)
Assignee | ||
Comment 33•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3fe914faed57dacc66da2a53fd67c326df449ee6 I manually tested this on Linux to make sure the changed code gets executed. I ran into an existing problem where dragging an image out of Firefox into a Nautilus window or the desktop (on Ubuntu 17.10) causes the browser to hang (bug 1431663), but I was able to verify DragDataProducer::Produce() was executed.
Assignee | ||
Comment 34•6 years ago
|
||
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations Cancelling the review request while I explore moving the MIME extension check to the parent.
Attachment #8955324 -
Flags: review?(bugs)
Comment 35•6 years ago
|
||
(In reply to g3gene from comment #28) > (In reply to yodolphe from comment #19) > > Ok, I can *not* reproduce the problem when just using right click > Save > > Image As..., here on FF 58 on macOS 10.12. It only happens when dragging out > > from FF. > > Thanks for workaround tip! > > That is correct. The context menu "save image as" problem does not show up > in my 10.12.6 mid 2012 MBP using Photoshop CC 2018. > I do not drag images outside of FF, because I need the Save dialog to direct > saved images to folders and cloud services. > > The problem shows up in my mid-2012 MBP running 10.13.3 > > I've changed "security.sandbox.content.level" to 2 and I will see if my > image file associations will hold to CC 2018 using "Ctrl + click > Save > Image as method. > > I hope to find out if that works, but it will take an average of two daays > to know if the problem repeats even with that setting. > > Thanks for everyone's hard work on this. I simply know something is wrong, > but not exactly why. > > Gene Update. The sandbox setting of 2 has clearly ended this problem, although it's "right-click and save" and not "drag" as others have experienced. 10.13.3 was affected, but not 10.12.6 for some reason. Gene
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 38•6 years ago
|
||
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations smaug, given that you worked on the e10s support for drag-and-drop, would you be able to give me some feedback on these changes? Or recommend someone? Thanks.
Attachment #8955324 -
Flags: feedback?(bugs)
Comment 39•6 years ago
|
||
I could, but possibly next week. Nica and Enn might recall this stuff too.
Comment 40•6 years ago
|
||
mozreview-review |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review232430 ::: dom/base/nsContentAreaDragDrop.cpp:280 (Diff revision 4) > + // extension is compatible with the OS's handler for this type. > + // If it isn't, or is missing, replace the extension with the > + // primary extension. Do this in the parent process because > + // sandboxing may block access to MIME-handler info from content > + // processes. > + if (XRE_IsParentProcess()) { Perhaps we should emit a warning or similar if you try to read the filename of a dragged image from the content process, because it won't be accurate? ::: dom/base/nsContentAreaDragDrop.cpp (Diff revision 4) > - > - // pass out the image source string nit: Why delete this comment? ::: dom/base/nsContentAreaDragDrop.cpp (Diff revision 4) > - NS_ENSURE_SUCCESS(rv, rv); > - } > - > - nsAutoCString fileName; > + nsAutoCString fileName; > - imgUrl->GetFileName(fileName); > + imgUrl->GetFileName(fileName); > - nit: why delete this line and the other spacing lines below? ::: dom/base/nsContentAreaDragDrop.cpp:887 (Diff revision 4) > + AddString(aDataTransfer, NS_LITERAL_STRING(kImageRequestMime), > + mImageRequestMime, principal); This exposes this information to the web. AddString() calls SetDataWithPrincipal, which will add the data in a web-visible way. For unfortunate legacy reasons, `_moz_htmlcontext` and `_moz_htmlinfo` are web-exposed right now, but I'd rather not add to that list. If we need to attach it to the DataTransfer, there's a "aHidden" argument to the inner SetDataWithPrincipal call (https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/dom/events/DataTransfer.cpp#1345) which could be exposed here, and used to hide this information from the web. If we have somewhere else we can put this data, that would be preferable. (see also bug 1345591) ::: widget/nsITransferable.idl:43 (Diff revision 4) > // that some parent structure and style can be preserved. kHTMLContext > // contains the serialized ancestor elements, whereas kHTMLInfo are numbers > // identifying where in the context the fragment was from. > -#define kHTMLContext "text/_moz_htmlcontext" > -#define kHTMLInfo "text/_moz_htmlinfo" > +#define kHTMLContext "text/_moz_htmlcontext" > +#define kHTMLInfo "text/_moz_htmlinfo" > +#define kImageRequestMime "text/_moz_requestmime" If you decide to add this format, please also add it to knownFormats: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/dom/events/DataTransfer.cpp#1026-1032 ::: widget/nsITransferable.idl:47 (Diff revision 4) > -#define kHTMLInfo "text/_moz_htmlinfo" > +#define kHTMLInfo "text/_moz_htmlinfo" > +#define kImageRequestMime "text/_moz_requestmime" > > // the source URL for a file promise > #define kFilePromiseURLMime "application/x-moz-file-promise-url" > +// the source URL for a file promise Why did you add this comment?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 42•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review232430 Thanks for taking a look, Nika. The comment and whitespace removals are fixed. They were bits I neglected to clean up after refactoring. Sorry about that. I'll look into passing the MIME type to the parent using some other mechanism and failing that will use the hiding approach you mentioned.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8955324 -
Flags: review?(nika)
Assignee | ||
Comment 44•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review232430 > Perhaps we should emit a warning or similar if you try to read the filename of a dragged image from the content process, because it won't be accurate? The filename from the image URI would only differ from the actual file after it was dragged out of the browser. Which seems equivalent to the user performing a File->Save As... If content is displaying an image that was dragged in, the extension overwriting doesn't occur. So I'm not sure a warning applies, but I may have misunderstood. > This exposes this information to the web. AddString() calls SetDataWithPrincipal, which will add the data in a web-visible way. > > For unfortunate legacy reasons, `_moz_htmlcontext` and `_moz_htmlinfo` are web-exposed right now, but I'd rather not add to that list. If we need to attach it to the DataTransfer, there's a "aHidden" argument to the inner SetDataWithPrincipal call (https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/dom/events/DataTransfer.cpp#1345) which could be exposed here, and used to hide this information from the web. > > If we have somewhere else we can put this data, that would be preferable. > > (see also bug 1345591) I looked into trying to pass the MIME type outside of the DataTransfer and came to the conclusion that storing the MIME as a chrome-only field in the DataTransfer was the most natural approach since the MIME type is needed at the same time as the filename in order to overwrite or add the extension. So I've gone with that approach. > If you decide to add this format, please also add it to knownFormats: https://searchfox.org/mozilla-central/rev/588d8120aa11738657da93e09a03378bcd1ba8ec/dom/events/DataTransfer.cpp#1026-1032 Done.
Assignee | ||
Updated•6 years ago
|
Attachment #8955324 -
Flags: review?(nika)
Assignee | ||
Updated•6 years ago
|
Comment hidden (mozreview-request) |
Comment 46•6 years ago
|
||
mozreview-review |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review235188 ::: dom/base/nsContentAreaDragDrop.cpp:201 (Diff revision 6) > +{ > + nsresult rv; > + > + nsCOMPtr<nsIMIMEService> mimeService = > + do_GetService("@mozilla.org/mime;1", &rv); > + NS_ENSURE_SUCCESS(rv, rv); nit: Please just use: ``` nsCOMPtr<nsIMIMEService> mimeService = do_GetService("@mozilla.org/mime;1"); if (NS_WARN_IF(!mimeService)) { return NS_ERROR_FAILURE; } ``` ::: dom/base/nsContentAreaDragDrop.cpp:281 (Diff revision 6) > + // extension is compatible with the OS's handler for this type. > + // If it isn't, or is missing, replace the extension with the > + // primary extension. Do this in the parent process because > + // sandboxing may block access to MIME-handler info from content > + // processes. > + if (XRE_IsParentProcess()) { Is this method ever called in the content process? It seems like SaveFileToURI wouldn't work great if called from the content process... ::: dom/base/nsContentAreaDragDrop.cpp:282 (Diff revision 6) > + aTransferable->GetTransferData(kImageRequestMime, > + getter_AddRefs(tmp), &dataSize); > + supportsString = do_QueryInterface(tmp); > + if (!supportsString) > + return NS_ERROR_FAILURE; > + > + nsAutoString imageRequestMime; > + supportsString->GetData(imageRequestMime); nsITransferable is really miserable to work with, isn't it >.< ::: dom/base/nsContentAreaDragDrop.cpp:293 (Diff revision 6) > + // Build a URL to get the filename extension > + nsCOMPtr<nsIURI> imageURI; > + rv = NS_NewURI(getter_AddRefs(imageURI), sourceURLString); I feel like we should be checking the targetFilename for its extension here rather than pulling it out of the source URL. It feels weird to compute that information in the content process, and then discard it when we get to the parent & recompute it. Perhaps we should stop sending down the targetFile alltogether and exclusively compute it here? --- We also re-parse this URL in SaveURLToFile, so perhaps we can parse it outside of the XRE_IsParentProcess block, and pass it down to avoid parsing the URI more times than we need to. ::: dom/base/nsContentAreaDragDrop.cpp:317 (Diff revision 6) > + rv = NS_MutateURI(imageURL) > + .Apply(NS_MutatorMethod(&nsIURLMutator::SetFileExtension, > + primaryExtension, nullptr)) > + .Finalize(imageURL); Let's avoid needing to mutate the URI here, by just pulling out GetFileBaseName(...) to get the filename - the extension, and then append '.' + primaryExtension. ::: dom/base/nsContentAreaDragDrop.cpp:354 (Diff revision 6) > file->Append(targetFilename); > > bool isPrivate; > aTransferable->GetIsPrivateData(&isPrivate); > > rv = SaveURIToFile(sourceURLString, file, isPrivate); This method constructs a nsIURI from the string internally. Please change this method to take an nsIURI and pass in the already parsed one which you're making now so we can avoid parsing twice. ::: dom/base/nsContentAreaDragDrop.cpp:678 (Diff revision 6) > - // Fix the file extension in the URL if necessary > - if (imgRequest && mimeService) { > nsCOMPtr<nsIURI> imgUri; > imgRequest->GetURI(getter_AddRefs(imgUri)); > > nsCOMPtr<nsIURL> imgUrl(do_QueryInterface(imgUri)); nit: While you're here can you remove this unnecessary queryinterface call? ::: dom/base/nsContentAreaDragDrop.cpp:694 (Diff revision 6) > - nsAutoCString fileName; > + nsAutoCString fileName; > - imgUrl->GetFileName(fileName); > + imgUrl->GetFileName(fileName); > > - NS_UnescapeURL(fileName); > + NS_UnescapeURL(fileName); > > - // make the filename safe for the filesystem > + // make the filename safe for the filesystem > - fileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, > + fileName.ReplaceChar(FILE_PATH_SEPARATOR FILE_ILLEGAL_CHARACTERS, > - '-'); > + '-'); > > - CopyUTF8toUTF16(fileName, mImageDestFileName); > + CopyUTF8toUTF16(fileName, mImageDestFileName); Let's not bother doing this here if we're just gonna re-do it in the parent process. ::: widget/nsITransferable.idl:41 (Diff revision 6) > > // These are used to indicate the context for a fragment of HTML source, such > // that some parent structure and style can be preserved. kHTMLContext > // contains the serialized ancestor elements, whereas kHTMLInfo are numbers > // identifying where in the context the fragment was from. > -#define kHTMLContext "text/_moz_htmlcontext" > +#define kHTMLContext "text/_moz_htmlcontext" Can we avoid changing the indentation here?
Attachment #8955324 -
Flags: review?(nika) → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 48•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review235188 > Is this method ever called in the content process? It seems like SaveFileToURI wouldn't work great if called from the content process... It is called from the content process when we serialize the transferable to send over IPC. We don't hit a SaveURIToFile() failure because kFilePromiseDirectoryMime is never added to the transfer in the child process and therefore we don't call SaveURIToFile(). Sandboxing would block the content process from trying to write the file. (There are a few directories content processes can write to on some platforms. On Mac, as of build 60, no filesystem writes are allowed.) To clean that up a bit, I've moved the SaveURIToFile() call to within the XRE_IsParentProcess() block which would mean it would still be called if e10s is disabled. > nsITransferable is really miserable to work with, isn't it >.< It's not my favorite thing. :) > I feel like we should be checking the targetFilename for its extension here rather than pulling it out of the source URL. > > It feels weird to compute that information in the content process, and then discard it when we get to the parent & recompute it. Perhaps we should stop sending down the targetFile alltogether and exclusively compute it here? > > --- > > We also re-parse this URL in SaveURLToFile, so perhaps we can parse it outside of the XRE_IsParentProcess block, and pass it down to avoid parsing the URI more times than we need to. Agreed that is a bit unnatural. I think it would be an improvement to not put the filename in the transferrable in content, but kFilePromiseDestFilename is referenced in a few other files and I'd prefer to keep this minimal and lower risk. (I'm going to request uplift to Beta after the fix has had some test time in Nightly.) And these changes are breaking the symmetry in a sense by adding per-process-type logic and maybe that could be done better. Would you accept addressing that in a follow-on bug? I've updated SaveURIToFile() to take the URI and avoid the double parsing issue. > nit: While you're here can you remove this unnecessary queryinterface call? Is there a better way to do the cast from nsIURI to nsIURL?
Comment 49•6 years ago
|
||
mozreview-review |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review236208 ::: dom/base/nsContentAreaDragDrop.cpp:188 (Diff revision 8) > + */ > +nsresult > +CheckAndGetExtensionForMime(const nsCString& aExtension, > + const nsCString& aMimeType, > + bool* aIsValidExtension, > + nsCString* aPrimaryExtension) Please change this outparameter to a nsACString& aPrimaryExtension
Attachment #8955324 -
Flags: review?(nika) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 51•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations https://reviewboard.mozilla.org/r/224166/#review236208 > Please change this outparameter to a nsACString& aPrimaryExtension Done.
Comment 52•6 years ago
|
||
Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/608119812c07 OSX dragging image to desktop changes OSX File associations r=mystor
Comment 53•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/608119812c07
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 54•6 years ago
|
||
Backed out changeset 608119812c07 (bug 1437281) on request from haik a=backout
Status: RESOLVED → REOPENED
status-firefox61:
fixed → ---
Ever confirmed: true
Flags: needinfo?(haftandilian)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Comment 55•6 years ago
|
||
Backout by ccoroiu@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/3189f213343f Backed out changeset 608119812c07 on request from haik a=backout
Assignee | ||
Comment 56•6 years ago
|
||
I asked for this to be backed out because it regressed the file-renaming-on-drag-out functionality on Windows. I didn't realize Windows used different code paths in the parent. My manual tests found this after landing the fix. On Windows, we use code from widget/windows/nsDataObj.cpp, specifically GetDownloadDetails(), to pull out the image information. Changing this on Windows is more involved. Since the bug being fixed here only applies to Mac, I'm going to change the fix to only apply to Mac. We have bug 1446549 filed to address how we use the MIME service from content processes. And I've filed bug 1449310 - "Need automated tests for dragging image out of browser and renaming file".
Flags: needinfo?(haftandilian)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 60•6 years ago
|
||
Pushed by haftandilian@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/69964a79c66a OSX dragging image to desktop changes OSX File associations r=mystor
Comment 61•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/69964a79c66a
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 62•6 years ago
|
||
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations Approval Request Comment [Feature/Bug causing the regression]: Bug 1332190, level 3 Filesystem Sandboxing on Mac. [User impact if declined]: Dragging an image in the browser can cause OS X file associations (i.e., the configuration of which apps open which file types) to be reset to the system default which is very frustrating for users. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Only manually. [Needs manual test from QE? If yes, steps to reproduce]: Yes. See description. The description uses Photoshop.app, but any non-default App that can open JPG's should be sufficient (such as a browser.) [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: No. [Why is the change risky/not risky?]: The changes are limited to Mac OS via #ifdefs and pretty self contained. [String changes made/needed]: None
Attachment #8955324 -
Flags: approval-mozilla-beta?
Comment 63•6 years ago
|
||
Comment on attachment 8955324 [details] Bug 1437281 - OSX dragging image to desktop changes OSX File associations fix a regression from mac sandboxing, approved for 60.0b12
Attachment #8955324 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 64•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5c9ecfe21944
status-firefox60:
--- → fixed
Updated•6 years ago
|
Flags: qe-verify+
Comment 65•6 years ago
|
||
I managed to reproduce the bug using macOS 10.12 and Mac OS 10.10.5 on an older version of Nightly (2018-02-10). I retested everything using latest Nightly 61.0a1 and beta 60.0b12 on Mac OS 10.10, Mac OS 10.11 and macOS 10.12, but the bug is not reproducing anymore.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Updated•6 years ago
|
Keywords: regression
Updated•6 years ago
|
Flags: in-qa-testsuite?
Updated•6 years ago
|
Flags: in-qa-testsuite? → in-qa-testsuite+
Comment 67•6 years ago
|
||
Firefox 61.0.2 Mac OS 10.11.6 El Capitan 15 August 2018 Uploading (not dragging) a photo to Twitter changed the computer's default program for opening a .jpg file from Preview to Firefox. Uploading another photo to Twitter changed the computer's default program for opening a .txt file from TextEdit to Console.
You need to log in
before you can comment on or make changes to this bug.
Description
•