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)

58 Branch
x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox60 --- verified
firefox61 --- verified

People

(Reporter: mikeyt, Assigned: haik)

References

(Regressed 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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
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)
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)
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.
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)
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)
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)
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)
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)
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.
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.
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
Thanks, I will look into that.
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!
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
Haik, any thoughts?
Blocks: 1332190
Flags: needinfo?(haftandilian)
Flags: needinfo?(yodolphe)
(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)
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)
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)
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?
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
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.
(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
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.
Attachment #8955324 - Flags: review?(bugs)
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.
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)
(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 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)
I could, but possibly next week.
Nica and Enn might recall this stuff too.
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 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.
Attachment #8955324 - Flags: review?(nika)
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.
Attachment #8955324 - Flags: review?(nika)
See Also: 14396801446549
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 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 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 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.
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/608119812c07
OSX dragging image to desktop changes OSX File associations r=mystor
https://hg.mozilla.org/mozilla-central/rev/608119812c07
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1448764
Backed out changeset 608119812c07 (bug 1437281) on request from haik a=backout
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(haftandilian)
Resolution: FIXED → ---
Target Milestone: mozilla61 → ---
Backout by ccoroiu@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/3189f213343f
Backed out changeset 608119812c07 on request from haik a=backout
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)
See Also: → 1448374
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69964a79c66a
OSX dragging image to desktop changes OSX File associations r=mystor
https://hg.mozilla.org/mozilla-central/rev/69964a79c66a
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
See Also: → 1452278
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 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+
Flags: qe-verify+
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+
Flags: in-qa-testsuite?
Flags: in-qa-testsuite? → in-qa-testsuite+
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.
Regressions: 1689471
You need to log in before you can comment on or make changes to this bug.