1344771 - Desktop attribution on Mac OSX

Status: VERIFIED FIXED

(Firefox :: General, enhancement, P1)

Description

[Chris More [:cmore]]

Similar to what was developed and launched for Windows in bug 1222258 and 1259607, I think we can do something very similar with Mac, but with much less effort.

It appears that when you download any software from any browser on the Mac, the OS records what browser downloaded it, from what domain and when. 

If this is available on the OS and Firefox is installed into a privledged place on the file system, maybe it is possible to read this meta data and send it into telemetery similar to what Windows stub attribution is doing. 

I will attach a screenshot of the dialog box and I will probably need a technical review to see if this is even possible and if other programs on the OS are able to read that specific meta data. Ideally, we would be able to determine what domain was used to download Firefox, set that the "source" and the "medium" to "referral" and pass that intro telemetry's existing attribution fields. It wouldn't be a detailed as the Windows side is, but it is better than nothing.

If we did this, we would also have to maintain the whitelist of domains (bug 1306457) in Firefox and the stop service, which is one downside to this idea.

Let's dig into this and see if anything is possible here.

Comment 1

[Chris More [:cmore]]

Attachment: file_quarantine.png

Example dialog box

Comment 2

[Francesco Polizzi [:fpolizzi]]

This data is gathered when a file is added to the [File Quarantine](https://support.apple.com/en-us/HT201940) after it is downloaded from the Web. Metadata can be accessed by running `xattr -lp com.apple.quarantine "filename.dmg"` or `xattr -lp com.apple.metadata:kMDItemWhereFroms "filename.dmg"`

Not sure whether it is accessible to us yet.

Comment 3

[Chris More [:cmore]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #2)
> This data is gathered when a file is added to the [File
> Quarantine](https://support.apple.com/en-us/HT201940) after it is downloaded
> from the Web. Metadata can be accessed by running `xattr -lp
> com.apple.quarantine "filename.dmg"` or `xattr -lp
> com.apple.metadata:kMDItemWhereFroms "filename.dmg"`
> 
> Not sure whether it is accessible to us yet.

Ah nice. Yeah, I just downloaded Firefox and this is what the second command produced:

xattr -lp com.apple.metadata:kMDItemWhereFroms firefox.dmg 
com.apple.metadata:kMDItemWhereFroms:
00000000  62 70 6C 69 73 74 30 30 A2 01 02 5F 10 65 68 74  |bplist00..._.eht|
00000010  74 70 73 3A 2F 2F 64 6F 77 6E 6C 6F 61 64 2D 69  |tps://download-i|
00000020  6E 73 74 61 6C 6C 65 72 2E 63 64 6E 2E 6D 6F 7A  |nstaller.cdn.moz|
00000030  69 6C 6C 61 2E 6E 65 74 2F 70 75 62 2F 66 69 72  |illa.net/pub/fir|
00000040  65 66 6F 78 2F 72 65 6C 65 61 73 65 73 2F 35 31  |efox/releases/51|
00000050  2E 30 2E 31 2F 6D 61 63 2F 65 6E 2D 55 53 2F 46  |.0.1/mac/en-US/F|
00000060  69 72 65 66 6F 78 25 32 30 35 31 2E 30 2E 31 2E  |irefox%2051.0.1.|
00000070  64 6D 67 5F 10 32 68 74 74 70 73 3A 2F 2F 77 77  |dmg_.2https://ww|
00000080  77 2E 6D 6F 7A 69 6C 6C 61 2E 6F 72 67 2F 65 6E  |w.mozilla.org/en|
00000090  2D 55 53 2F 66 69 72 65 66 6F 78 2F 6E 65 77 2F  |-US/firefox/new/|
000000A0  3F 73 63 65 6E 65 3D 32 08 0B 73 00 00 00 00 00  |?scene=2..s.....|
000000B0  00 01 01 00 00 00 00 00 00 00 03 00 00 00 00 00  |................|
000000C0  00 00 00 00 00 00 00 00 00 00 A8                 |...........|
000000cb

I can see that it was downloaded from the CDR and the referring website was http://www.mozilla.org/en-US/firefox/new/?scene=2 and if there was UTM parameters on scene 2, it could be parsed if needed.

Comment 4

[Chris More [:cmore]]

edit: /CDR/CDN/

Comment 5

[Francesco Polizzi [:fpolizzi]]

More on extended attributes: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/xattr.1.html

Comment 6

[Chris More [:cmore]]

The one issue with using this method is that it would look like all referring domains to Firefox is www.mozilla.org if they download from www.mozilla.org. If they came from any other source, it would still look like mozilla.org. 

UNLESS: we urlencode http.referer as &referrer= in the URL and decode it later. This isn't needed for the Windows solution because the webpage is talking to a service directly and does this without having to pass it via a URL parameter.

Comment 7

[Chris More [:cmore]]

I didn't need sudo to run xattr -lp on the file system to get the info, so I wonder if an installed app, like Firefox would also have the permissions to do the same.

Comment 8

[Francesco Polizzi [:fpolizzi]]

(In reply to Chris More [:cmore] from comment #6)
> The one issue with using this method is that it would look like all
> referring domains to Firefox is www.mozilla.org if they download from
> www.mozilla.org. If they came from any other source, it would still look
> like mozilla.org. 
> 
> UNLESS: we urlencode http.referer as &referrer= in the URL and decode it
> later. This isn't needed for the Windows solution because the webpage is
> talking to a service directly and does this without having to pass it via a
> URL parameter.

Why is that? There seem to be two URLs in the metadata: 1: https://download-installer.cdn.mozilla.net/pub/firefox/releases/51.0.1/mac/en-US/Firefox%2051.0.1.dmg_.2 and https://www.mozilla.org/en-US/firefox/new/?scene=2

Comment 9

[Francesco Polizzi [:fpolizzi]]

Here's the documentation for the BSD System Call: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man2/getxattr.2.html#//apple_ref/doc/man/2/getxattr

Comment 10

[Chris More [:cmore]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #8)
> (In reply to Chris More [:cmore] from comment #6)
> > The one issue with using this method is that it would look like all
> > referring domains to Firefox is www.mozilla.org if they download from
> > www.mozilla.org. If they came from any other source, it would still look
> > like mozilla.org. 
> > 
> > UNLESS: we urlencode http.referer as &referrer= in the URL and decode it
> > later. This isn't needed for the Windows solution because the webpage is
> > talking to a service directly and does this without having to pass it via a
> > URL parameter.
> 
> Why is that? There seem to be two URLs in the metadata: 1:
> https://download-installer.cdn.mozilla.net/pub/firefox/releases/51.0.1/mac/
> en-US/Firefox%2051.0.1.dmg_.2 and
> https://www.mozilla.org/en-US/firefox/new/?scene=2

Because if for example, you came from a Google search, landed on www.mozilla.org/firefox/new/, the Windows-based attribution would have source=google.com because that is the source that eventually drove the person to the download page. With the metadata on the mac, it's not the download page's source domain, but the download page itself. So, that would mean it would look like www.mozilla.org is the source of everything, when attribution is supposed to be about where the person came before downloading.

Comment 11

[Francesco Polizzi [:fpolizzi]]

So, talked with :nhnt11 and we might have a potential solutions:

1. Hdiutil on OSx will give information about the currently mounted .DMG's on the system [1][2]. If we operate under the hypothesis that few people will unmount the Firefox DMG before firstrun (either via reboot or manual unmount), we could use hdiutil to grab the location of the DMG in the File System, and then grab that file's xattr's to get the download URL.

2. We could include an installer on OSx which would be able to more easily grab the DMG file location and its xattr's. Pro: more reliable
Con: bigger dropoff between download and firstrun (due to potential installer issues)

Additionally, we may be able to do option 1 within an add-on SDK funnelcake, because the sdk provides a way to spawn child processes [3], from which we should be able to access hdutil and xattr.

[1] https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man1/hdiutil.1.html 
[2] http://stackoverflow.com/questions/1949879/get-the-path-of-the-dmg-from-the-mount-point
[3] https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/system_child_process

Comment 12

[Francesco Polizzi [:fpolizzi]]

Attachment: Console log for the add-on containing URL sourc

I have created a proof of concept sdk add-on which successfully grabs the URLs of any DMG named "Firefox 51.0.1.dmg" which is currently mounted. Assuming we can figure out what the file-name should be upon download, this should work.

Code: https://github.com/FrancescoSTL/OSxAttribution
Output: attached

Comment 13

[Francesco Polizzi [:fpolizzi]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #12)
> Created attachment 8845061 [details]
> Console log for the add-on containing URL sourc
> 
> I have created a proof of concept sdk add-on which successfully grabs the
> URLs of any DMG named "Firefox 51.0.1.dmg" which is currently mounted.
> Assuming we can figure out what the file-name should be upon download, this
> should work.
> 
> Code: https://github.com/FrancescoSTL/OSxAttribution
> Output: attached

Scratch that, it actually just grabs the source URL of the latest DMG you've mounted, for the sake of example.

Comment 14

[Francesco Polizzi [:fpolizzi]]

In short, if we can:
1. Reliably determine the DMG file name for Firefox on the user's system
2. Be confident that at least *most* users won't unmount the DMG before they first run Firefox
3. Urlencode http.referer as &referrer= in the URL

then this proof-of-concept should work fine in Firefox to give users attribution.

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #14)
> In short, if we can:
> 1. Reliably determine the DMG file name for Firefox on the user's system
> 2. Be confident that at least *most* users won't unmount the DMG before they
> first run Firefox

Both of these points seem like big assumptions to make without data to back it up. I, for one, always unmount any DMGs that were mounted to install an application before running it. Also, I frequently rename DMGs before downloading them. macOS may also rename files when there are multiple DMGs with the same name. You would at least need to use a placeholder for "(1)" and similar additions to the file name.

(In reply to Francesco Polizzi [:fpolizzi] from comment #12)
> I have created a proof of concept sdk add-on

We're slowly but surely moving to WebExtensions for addons. You may want to verify that you have the ability to execute the necessary commands using WebExtensions.

(In reply to Francesco Polizzi [:fpolizzi] from comment #11)
> 2. We could include an installer on OSx which would be able to more easily
> grab the DMG file location and its xattr's. Pro: more reliable
> Con: bigger dropoff between download and firstrun (due to potential
> installer issues)

There are numerous additional cons to adding an installer on macOS. Unless there are absolutely no other ways of achieving what we need here without installers, we should not consider this as an option.

Could you point me to the code that was added on Windows to achieve this functionality? I'm getting lost in the various meta bugs. Thanks!

Comment 16

[Francesco Polizzi [:fpolizzi]]

(In reply to Stephen A Pohl [:spohl] from comment #15)
> (In reply to Francesco Polizzi [:fpolizzi] from comment #14)
> > In short, if we can:
> > 1. Reliably determine the DMG file name for Firefox on the user's system
> > 2. Be confident that at least *most* users won't unmount the DMG before they
> > first run Firefox
> 
> Both of these points seem like big assumptions to make without data to back
> it up. I, for one, always unmount any DMGs that were mounted to install an
> application before running it. Also, I frequently rename DMGs before
> downloading them. macOS may also rename files when there are multiple DMGs
> with the same name. You would at least need to use a placeholder for "(1)"
> and similar additions to the file name.

Need info-ing :cmore

> 
> (In reply to Francesco Polizzi [:fpolizzi] from comment #12)
> > I have created a proof of concept sdk add-on
> 
> We're slowly but surely moving to WebExtensions for addons. You may want to
> verify that you have the ability to execute the necessary commands using
> WebExtensions.

If the sdk functionality still available to platform engineers within Firefox, this should be fine. The proof of concept is just to determine whether or not it is a viable solution for integration into Firefox. That said, we will definitely need to revisit this if we try any funnelcake testing.

> 
> (In reply to Francesco Polizzi [:fpolizzi] from comment #11)
> > 2. We could include an installer on OSx which would be able to more easily
> > grab the DMG file location and its xattr's. Pro: more reliable
> > Con: bigger dropoff between download and firstrun (due to potential
> > installer issues)
> 
> There are numerous additional cons to adding an installer on macOS. Unless
> there are absolutely no other ways of achieving what we need here without
> installers, we should not consider this as an option.

Agreed!

> 
> Could you point me to the code that was added on Windows to achieve this
> functionality? I'm getting lost in the various meta bugs. Thanks!

Attribution in telemetry: https://hg.mozilla.org/releases/mozilla-beta/rev/7703463c9421
Whitelist for URLs we send to telemetry as "source" or "medium": https://github.com/mozilla-services/stubattribution/commit/50094bb1b30e425be1ca1c09171a8026ab1d66ba

Comment 17

[Chris More [:cmore]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #16)
> (In reply to Stephen A Pohl [:spohl] from comment #15)
> > (In reply to Francesco Polizzi [:fpolizzi] from comment #14)
> > > In short, if we can:
> > > 1. Reliably determine the DMG file name for Firefox on the user's system
> > > 2. Be confident that at least *most* users won't unmount the DMG before they
> > > first run Firefox
> > 
> > Both of these points seem like big assumptions to make without data to back
> > it up. I, for one, always unmount any DMGs that were mounted to install an
> > application before running it. Also, I frequently rename DMGs before
> > downloading them. macOS may also rename files when there are multiple DMGs
> > with the same name. You would at least need to use a placeholder for "(1)"
> > and similar additions to the file name.
> 
> Need info-ing :cmore

Agree. it is an assumption. Though, if you think about the typical work flow, you seek out a new product, download, install and open it. You usually go back and unmount the image later when cleaning up, though, some people will unmount before opening. 

:fpolizzi: is the meta data only in the mounted image? So, if I download a piece of software on my Mac, unmount the image, and open the software for the first time, it won't mention the URL where it was downloaded?

> 
> > 
> > (In reply to Francesco Polizzi [:fpolizzi] from comment #12)
> > > I have created a proof of concept sdk add-on
> > 
> > We're slowly but surely moving to WebExtensions for addons. You may want to
> > verify that you have the ability to execute the necessary commands using
> > WebExtensions.
> 
> If the sdk functionality still available to platform engineers within
> Firefox, this should be fine. The proof of concept is just to determine
> whether or not it is a viable solution for integration into Firefox. That
> said, we will definitely need to revisit this if we try any funnelcake
> testing.
> 
> > 
> > (In reply to Francesco Polizzi [:fpolizzi] from comment #11)
> > > 2. We could include an installer on OSx which would be able to more easily
> > > grab the DMG file location and its xattr's. Pro: more reliable
> > > Con: bigger dropoff between download and firstrun (due to potential
> > > installer issues)
> > 
> > There are numerous additional cons to adding an installer on macOS. Unless
> > there are absolutely no other ways of achieving what we need here without
> > installers, we should not consider this as an option.
> 
> Agreed!
> 
> > 
> > Could you point me to the code that was added on Windows to achieve this
> > functionality? I'm getting lost in the various meta bugs. Thanks!
> 
> Attribution in telemetry:
> https://hg.mozilla.org/releases/mozilla-beta/rev/7703463c9421
> Whitelist for URLs we send to telemetry as "source" or "medium":
> https://github.com/mozilla-services/stubattribution/commit/
> 50094bb1b30e425be1ca1c09171a8026ab1d66ba

Comment 18

[Francesco Polizzi [:fpolizzi]]

(In reply to Chris More [:cmore] from comment #17)
> :fpolizzi: is the meta data only in the mounted image? So, if I download a
> piece of software on my Mac, unmount the image, and open the software for
> the first time, it won't mention the URL where it was downloaded?

Nope, the metadata is there regardless. The only issue is finding the file location. We can get a filepath by looking at all currently mounted DMGs and locating Firefox's DMG within that list. If there is a better way to locate the filepath of the DMG that would be ideal, however, I'm not sure if there is. Maybe Firefox has a way of determining its origin DMG filepath? Not sure.

Comment 19

[Chris More [:cmore]]

(In reply to Francesco Polizzi [:fpolizzi] from comment #18)
> (In reply to Chris More [:cmore] from comment #17)
> > :fpolizzi: is the meta data only in the mounted image? So, if I download a
> > piece of software on my Mac, unmount the image, and open the software for
> > the first time, it won't mention the URL where it was downloaded?
> 
> Nope, the metadata is there regardless. The only issue is finding the file
> location. We can get a filepath by looking at all currently mounted DMGs and
> locating Firefox's DMG within that list. If there is a better way to locate
> the filepath of the DMG that would be ideal, however, I'm not sure if there
> is. Maybe Firefox has a way of determining its origin DMG filepath? Not sure.

Do you need to know the file path of the downloaded dmg or the file path of the installed Firefox? What if they delete they unmount and delete the original file? What file is the meta data associated to?

Comment 20

[Francesco Polizzi [:fpolizzi]]

(In reply to Chris More [:cmore] from comment #19)
>
> Do you need to know the file path of the downloaded dmg or the file path of
> the installed Firefox? What if they delete they unmount and delete the
> original file? What file is the meta data associated to?

To my knowledge, the metadata in question is only associated with the originally downloaded DMG file. It is data that OSx appends as "extended attributes" to any file downloaded from the web[1].

[1] https://en.wikipedia.org/wiki/Extended_file_attributes#macOS

Comment 21

[Francesco Polizzi [:fpolizzi]]

Hey :spohl, any chance you had the opportunity to review the above? As :cmore mentioned, we're just looking to grab a majority (and hopefully representative of the whole) sample of users. If this seems like a viable method for that end, we're hoping reach out to Platform to discuss further.

Comment 22

[Stephen A Pohl [:spohl]]

My initial points about the assumptions still stand, but you can of course decide to go ahead anyway if this is good enough for the type of analysis that you wish to perform.

Comment 23

[Shane Caraveo (:mixedpuppy)]

(In reply to Francesco Polizzi [:fpolizzi] from comment #20)
> (In reply to Chris More [:cmore] from comment #19)
> >
> > Do you need to know the file path of the downloaded dmg or the file path of
> > the installed Firefox? What if they delete they unmount and delete the
> > original file? What file is the meta data associated to?
> 
> To my knowledge, the metadata in question is only associated with the
> originally downloaded DMG file.

In my testing that is not the case.  The attributes are on Firefox.app, not the dmg (well, perhaps in addition to the dmg).

Also, we can pass query strings through.  I used "https://www.mozilla.org/en-US/firefox/download/thanks/?source=amo&campaign=foobar&medium=test&content=addonid" to download firefox.  So we could pass the source ourselves (but then so can anyone).

I also verified that the xattrs survive at least first run (even the second run).  That means we should be able run code inside firefox to fetch the xattr, open the sqlite db and query the url.

xattr -l ~/Downloads/Firefox.app

com.apple.quarantine: 0181;5b294bee;Firefox\x20Nightly;A86ECDF2-0826-42B9-920A-9C999CEB135E

sqlite3 ~/Library/Preferences/com.apple.LaunchServices.QuarantineEventsV2 "SELECT * FROM LSQuarantineEvent WHERE LSQuarantineEventIdentifier == 'A86ECDF2-0826-42B9-920A-9C999CEB135E'"

A86ECDF2-0826-42B9-920A-9C999CEB135E|551125836.0|org.mozilla.nightly|Firefox Nightly|https://download-installer.cdn.mozilla.net/pub/firefox/releases/60.0.2/mac/en-US/Firefox%2060.0.2.dmg|||0||https://www.mozilla.org/en-US/firefox/download/thanks/?source=amo&campaign=foobar&medium=test&content=addonid|

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 25

[Shane Caraveo (:mixedpuppy)]

Some details on the attached patch.

- Telemetry reads attribution on startup.
  - Whoever consumes that data should be aware that osx would be added to the dataset.
  - The osx apis should be fast, I dont expect any startup perf issue.
- Not sure how to handle automated testing (see below)
- The content param in the case of AMO would need to be the extension id.
- This only works if the download url contains the params

That last item means a touch more work.  For example, on AMO a download firefox button will point to:

https://www.mozilla.org/en-US/firefox/new/?utm_campaign=non-fx-button&utm_content=[extension id]&utm_medium=referral&utm_source=addons.mozilla.org

This presents a page with a download button (not sure why it doesn't just initiate the download).

That download button does not contain the params, so the downloaded dmg does not have attributions.  The button on the page would need to be updated to have modified params (note the removal of utm_):

https://www.mozilla.org/en-US/firefox/download/thanks/?campaign=non-fx-button&content=[extension id]&medium=referral&source=addons.mozilla.org

== testing ==

- Using the above "thanks" url, download a dmg.
- extract firefox to some directory
- start firefox
- in a scratchpad using browser environment run the following code and see the output in console.


ChromeUtils.import("resource:///modules/AttributionCode.jsm");
ChromeUtils.import("resource://gre/modules/addons/AddonRepository.jsm");

async function test_attribution() {
  // This is what we would do in Firefox to get the extension data
  let attribution = await AttributionCode.getAttrDataAsync();
  console.log(`*** attribution is`, attribution);

  // If content were the extension id, this will work
  let result = await AddonRepository.getAddonsByIDs([attribution.content]);
  console.log(`*** addon is`, result);
}

test_attribution();

Comment 26

[Shane Caraveo (:mixedpuppy)]

Another note on testing during dev:

To test a local build out of m-c, download per above notes then to apply the attributions to the .app bundle in your build:

cd m-c/
xattr -p com.apple.quarantine ~/Downloads/Firefox.app > ./com.apple.quarantine
xattr -w com.apple.quarantine "`cat com.apple.quarantine`" obj-x86_64-apple-darwin17.6.0/dist/Nightly.app

Comment 27

[Stephen A Pohl [:spohl]]

What's the type of feedback you're requesting at the moment? Are the macOS parts ready for review? Or is there a specific problem that you're trying to resolve?

Comment 28

[Shane Caraveo (:mixedpuppy)]

(In reply to Stephen A Pohl [:spohl] from comment #27)
> What's the type of feedback you're requesting at the moment? Are the macOS
> parts ready for review? Or is there a specific problem that you're trying to
> resolve?

It's probably ready for review, but am unsure about

a) is adding this to MacShell ok (couldn't think of a better place)?
b) resolving how to deal with testing
c) my cocoa-foo is weak

Comment 29

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 30

[Stephen A Pohl [:spohl]]

Sorry about the delay here. Working on the review now.

Comment 31

[Stephen A Pohl [:spohl]]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/252786/#review261128

nit: over 80 characters in several places throughout this patch

::: commit-message-6b6f3:1
(Diff revision 2)
> +Bug 1344771 - Implement attribution on OSX using quarentine data, r?spohl

nit: s/quarentine/quarantine/

::: browser/components/shell/nsIMacShellService.idl:15
(Diff revision 2)
>  {
>    const long APPLICATION_KEYCHAIN_ACCESS  = 2;
>    const long APPLICATION_NETWORK          = 3;
>    const long APPLICATION_DESKTOP          = 4;
> +
> +  void getQuarantineMetadata(in ACString filePath, out ACString source, out ACString referrer);

As you suspected, I don't believe this goes here. I believe the best choice would be to introduce a new interface under toolkit/mozapps/installer specifically for attribution. As such, you will also want to get a review from either :rstrong or :mhowell.

::: browser/components/shell/nsMacShellService.cpp:429
(Diff revision 2)
> +
> +NS_IMETHODIMP
> +nsMacShellService::GetQuarantineMetadata(const nsACString& aFilePath, nsACString& source, nsACString& referrer)
> +{
> +  CFStringRef filePath = ::CFStringCreateWithCString(kCFAllocatorDefault,
> +                                                       PromiseFlatCString(aFilePath).get(),

nit: vertical alignment and over 80 characters

::: browser/components/shell/nsMacShellService.cpp:436
(Diff revision 2)
> +  CFURLRef fileURL = ::CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
> +                                                     filePath,
> +                                                     kCFURLPOSIXPathStyle,
> +                                                     false);
> +
> +  // The properties key changed in 10.10:

Rather than copy all this code from CocoaFileUtils.mm it would be great to refactor this into a separate function that can be used in both places.

::: browser/components/shell/nsMacShellService.cpp:470
(Diff revision 2)
> +    ::CFDictionaryCreateMutableCopy(kCFAllocatorDefault, 0, (CFDictionaryRef)quarantineProps);
> +  ::CFRelease(quarantineProps);
> +
> +  CFTypeRef referrerRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineOriginURLKey);
> +  if (referrerRef && CFGetTypeID(referrerRef) == CFURLGetTypeID()) {
> +    const char *referrerUrl = ::CFStringGetCStringPtr(::CFURLGetString(static_cast<CFURLRef>(referrerRef)), kCFStringEncodingUTF8);

nit: s/const char */const char*/

::: browser/components/shell/nsMacShellService.cpp:473
(Diff revision 2)
> +  CFTypeRef referrerRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineOriginURLKey);
> +  if (referrerRef && CFGetTypeID(referrerRef) == CFURLGetTypeID()) {
> +    const char *referrerUrl = ::CFStringGetCStringPtr(::CFURLGetString(static_cast<CFURLRef>(referrerRef)), kCFStringEncodingUTF8);
> +    referrer.Assign(referrerUrl);
> +  }
> +  CFTypeRef sourceRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineDataURLKey);

Since `source` is not currently used, it seems to make sense to simply remove it from the function signature and not extract it here. Am I missing something?

::: browser/components/shell/nsMacShellService.cpp:475
(Diff revision 2)
> +    const char *referrerUrl = ::CFStringGetCStringPtr(::CFURLGetString(static_cast<CFURLRef>(referrerRef)), kCFStringEncodingUTF8);
> +    referrer.Assign(referrerUrl);
> +  }
> +  CFTypeRef sourceRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineDataURLKey);
> +  if (sourceRef && CFGetTypeID(sourceRef) == CFURLGetTypeID()) {
> +    const char *sourceUrl = ::CFStringGetCStringPtr(::CFURLGetString(static_cast<CFURLRef>(sourceRef)), kCFStringEncodingUTF8);

nit: s/const char */const char*/

::: browser/modules/AttributionCode.jsm:1
(Diff revision 2)
>  /* This Source Code Form is subject to the terms of the Mozilla Public

This file will need a review from a browser module peer.

::: browser/modules/AttributionCode.jsm:72
(Diff revision 2)
>     * Returns a promise that fulfills with an object containing the parsed
>     * attribution data if the code could be read and is valid,
>     * or an empty object otherwise.
> +   *
> +   * On windows the attribution service converts utm_* keys, removing "utm_".
> +   * On OSX the attriutions are set directly on download and retain "utm_".  We

s/attriutions/attributions/

::: browser/modules/AttributionCode.jsm:73
(Diff revision 2)
>     * attribution data if the code could be read and is valid,
>     * or an empty object otherwise.
> +   *
> +   * On windows the attribution service converts utm_* keys, removing "utm_".
> +   * On OSX the attriutions are set directly on download and retain "utm_".  We
> +   * strip "utm_" while retreiving the params.

s/retreiving/retrieving/

Comment 32

[Stephen A Pohl [:spohl]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> b) resolving how to deal with testing

Can you point me to the Windows tests for this?

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to Stephen A Pohl [:spohl] from comment #32)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> > b) resolving how to deal with testing
> 
> Can you point me to the Windows tests for this?

https://searchfox.org/mozilla-central/source/browser/modules/test/unit/test_AttributionCode.js

However, it doesn't really test the whole attribution process.  It writes the file that the stub installer would otherwise write then reads that.  I thought about exposing a write attributes function, but that feels as meh as the windows test.

Comment 34

[Shane Caraveo (:mixedpuppy)]

> As you suspected, I don't believe this goes here. I believe the best choice
> would be to introduce a new interface under toolkit/mozapps/installer
> specifically for attribution. As such, you will also want to get a review
> from either :rstrong or :mhowell.

There's not an installer for osx, so I opted to just create a new toolkit component.

Comment 35

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 36

[Stephen A Pohl [:spohl]]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/252786/#review262892

Can we name the folder under toolkit/components `macattribution` rather than `macquarantine`? Same for the interface and implementation files, i.e. nsMacAttribution.h/.cpp etc. This makes it clear what these files are doing. The fact that they use the Mac Quarantine to do this is an implementation detail that could change in the future.

::: browser/modules/AttributionCode.jsm:96
(Diff revisions 2 - 3)
>            // failure to open or read it isn't an error.
>          }
>        } else if (AppConstants.platform == "macosx") {
>          try {
>            let appPath = Services.dirsvc.get("GreD", Ci.nsIFile).parent.parent.path;
> -          let shell = Cc["@mozilla.org/browser/shell-service;1"]
> +          let qsvc = Cc["@mozilla.org/mac-quarantine;1"]

nit: Once we change the name to mac-attribution, `let attributionSvc` may be a better variable name.

::: toolkit/components/macquarantine/nsIMacQuarantine.idl:1
(Diff revision 3)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Use `a` prefixes for attributes throughout, for example `aReferrer` instead of `referrer`.

::: toolkit/components/macquarantine/nsIMacQuarantine.idl:12
(Diff revision 3)
> +
> +[scriptable, uuid(6FC66A78-6CBC-4B3F-B7BA-379289B29276)]
> +interface nsIMacQuarantineService : nsISupports
> +{
> +  /**
> +   * Used by the Attributions system to get download referrer.

nit: ... 'the' download referrer

::: toolkit/components/macquarantine/nsIMacQuarantine.idl:24
(Diff revision 3)
> +  /**
> +   * Used by the tests.
> +   *
> +   * @param filePath A path to the file to set the quarantine data on.
> +   * @param referrer A url to set as the referrer for the download.
> +   * @param create Add or replace existing quarantine properties.

Could you improve the description for the create param? If I'm reading this right, if true, the quarantine property dictionary is created if none exists and replaced if one exists. If false, it will only update the dictionary if one already exists. Is this correct?

::: toolkit/components/macquarantine/nsMacQuarantine.cpp:6
(Diff revision 3)
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsMacQuarantine.h"

The order of these includes seems to be off. How about:

#include "nsMacQuarantine.h"

#include <CoreFoundation/CoreFoundation.h>
#include <ApplicationServices/ApplicationServices.h>

#include "../../../xpcom/io/CocoaFileUtils.h"
#include "nsCocoaFeatures.h"
#include "nsString.h"

::: toolkit/components/macquarantine/nsMacQuarantine.cpp:64
(Diff revision 3)
> +                                                 referrer, nullptr);
> +
> +  CocoaFileUtils::AddQuarantineMetadataToFile(filePath,
> +                                              referrerURL,
> +                                              referrerURL,
> +                                              isFromWeb,

nit: you may just pass true here instead of using a local variable.

::: toolkit/components/macquarantine/test/test_quarantine.js:13
(Diff revision 3)
> +
> +add_task(async function test_quarantine() {
> +  let appPath = Services.dirsvc.get("GreD", Ci.nsIFile).parent.parent.path;
> +  ok(`using path at ${appPath}`);
> +  let qsvc = Cc["@mozilla.org/mac-quarantine;1"]
> +                .getService(Ci.nsIMacQuarantineService);

nit: indent appears to have one too many spaces.

::: toolkit/components/macquarantine/test/test_quarantine.js:17
(Diff revision 3)
> +  let qsvc = Cc["@mozilla.org/mac-quarantine;1"]
> +                .getService(Ci.nsIMacQuarantineService);
> +
> +  qsvc.setReferrerUrl(appPath, "", true);
> +  let referrer = qsvc.getReferrerUrl(appPath);
> +  equal(referrer, "", "quarantine attribute read");

It would be great to have specific test comments for each check here (instead of the identical "quarantine attribute read") to make it easier to differentiate failures in test logs.

::: toolkit/components/macquarantine/test/xpcshell.ini:3
(Diff revision 3)
> +[DEFAULT]
> +firefox-appdir = browser
> +skip-if = appname == "thunderbird" || toolkit != "cocoa"

Can this be rewritten as follows:

skip-if = appname != "firefox" || toolkit != "cocoa",

or is this supposed to run for any other apps on mac?

::: toolkit/components/moz.build:116
(Diff revision 3)
>      EXTRA_COMPONENTS += [
>          'nsDefaultCLH.js',
>          'nsDefaultCLH.manifest',
>      ]
>  
> +DIRS += ['macquarantine']

It seems like this could be narrowed to CONFIG['MOZ_BUILD_APP'] == 'browser' and even CONFIG['MOZ_WIDGET_TOOLKIT'] == 'cocoa'.

::: xpcom/io/CocoaFileUtils.mm:201
(Diff revision 3)
> -    quarantinePropKey = kLSItemQuarantineProperties;
>    }
> +  return kLSItemQuarantineProperties;
> +}
> +
> +CFMutableDictionaryRef GetQuarantineDictionary(const CFURLRef fileURL, const bool createProps) {

This function should be called `CreateQuarantineDictionary` as this is creating a new dictionary that will have to be released by the caller. Also, the callers don't currently release the created dictionary. See https://developer.apple.com/library/archive/documentation/CoreFoundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html

::: xpcom/io/CocoaFileUtils.mm:291
(Diff revision 3)
> +  }
> +
> +  // CFURLRef referrerURLRef = NULL;
> +  CFTypeRef referrerRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineOriginURLKey);
> +  if (referrerRef && CFGetTypeID(referrerRef) == CFURLGetTypeID()) {
> +    return ::CFURLGetString(static_cast<CFURLRef>(referrerRef));

This return fails to release fileURL and mutQuarantineProps.

Comment 37

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 38

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

r? Mossop for new component in toolkit.

Comment 39

[Dave Townsend [:mossop]]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/252786/#review263120

I'd like to see all the toolkit bits and AttributionCode.jsm move to browser/components/attribution so we have a central place for all of this. Presumably the package-lock.json file changes are unnecessary.

Comment 40

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252786/diff/4-5/

Comment 41

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/252786/#review263160

I didn't review the new Mac-specific code, just the reorg of the existing code that's also part of this patch, but all of those changes look good to me.

Comment 42

[Stephen A Pohl [:spohl]]

Comment on attachment 8987548 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/252786/#review264092

This looks good with the comments addressed.

1. Please ensure that the 80 character limit is respected throughout the patch. I pointed out a few places where the limit was exceeded, but there may be more.
2. I suggest sending this back to :dtownsend to get his r+ as well.

::: xpcom/io/CocoaFileUtils.h:33
(Diff revisions 3 - 5)
>  void     AddQuarantineMetadataToFile(const CFStringRef filePath,
>                                       const CFURLRef sourceURL,
>                                       const CFURLRef referrerURL,
>                                       const bool isFromWeb,
>                                       const bool createProps=false);
> -CFStringRef GetQuarantineReferrerUrl(const CFStringRef filePath);
> +void CopyQuarantineReferrerUrl(const CFStringRef filePath, nsAString& referrer);

Use `a` prefixes for attributes

::: xpcom/io/CocoaFileUtils.mm:14
(Diff revisions 3 - 5)
>  #include "nsCocoaUtils.h"
>  #include <Cocoa/Cocoa.h>
>  #include "nsObjCExceptions.h"
>  #include "nsDebug.h"
> +#include "nsString.h"
> +#include "../base/MacStringHelpers.h"

Can this be changed to:
#include "mozilla/MacStringHelpers.h"?

::: xpcom/io/CocoaFileUtils.mm:203
(Diff revisions 3 - 5)
>      return kCFURLQuarantinePropertiesKey;
>    }
>    return kLSItemQuarantineProperties;
>  }
>  
> -CFMutableDictionaryRef GetQuarantineDictionary(const CFURLRef fileURL, const bool createProps) {
> +CFMutableDictionaryRef CreateQuarantineDictionary(const CFURLRef fileURL, const bool createProps) {

Use `a` prefixes for attributes

::: xpcom/io/CocoaFileUtils.mm:203
(Diff revisions 3 - 5)
>      return kCFURLQuarantinePropertiesKey;
>    }
>    return kLSItemQuarantineProperties;
>  }
>  
> -CFMutableDictionaryRef GetQuarantineDictionary(const CFURLRef fileURL, const bool createProps) {
> +CFMutableDictionaryRef CreateQuarantineDictionary(const CFURLRef fileURL, const bool createProps) {

nit: over 80 characters

::: xpcom/io/CocoaFileUtils.mm:246
(Diff revisions 3 - 5)
>    CFURLRef fileURL = ::CFURLCreateWithFileSystemPath(kCFAllocatorDefault,
>                                                       filePath,
>                                                       kCFURLPOSIXPathStyle,
>                                                       false);
>  
> -  CFMutableDictionaryRef mutQuarantineProps = GetQuarantineDictionary(fileURL, createProps);
> +  CFMutableDictionaryRef mutQuarantineProps = CreateQuarantineDictionary(fileURL, createProps);

nit: over 80 characters

::: xpcom/io/CocoaFileUtils.mm:293
(Diff revisions 3 - 5)
>  
> -  // CFURLRef referrerURLRef = NULL;
>    CFTypeRef referrerRef = ::CFDictionaryGetValue(mutQuarantineProps, kLSQuarantineOriginURLKey);
> -  if (referrerRef && CFGetTypeID(referrerRef) == CFURLGetTypeID()) {
> -    return ::CFURLGetString(static_cast<CFURLRef>(referrerRef));
> +  if (referrerRef && ::CFGetTypeID(referrerRef) == ::CFURLGetTypeID()) {
> +    // URL string must be copied prior to releasing the dictionary.
> +    mozilla::CopyCocoaStringToXPCOMString((NSString*)::CFURLGetString(static_cast<CFURLRef>(referrerRef)), aReferrer);

nit: over 80 characters

::: browser/components/attribution/nsIMacAttribution.idl:24
(Diff revision 5)
> +  /**
> +   * Used by the tests.
> +   *
> +   * @param filePath A path to the file to set the quarantine data on.
> +   * @param referrer A url to set as the referrer for the download.
> +   * @param create   Create new quarantine properties, overwriting any

nit: add "If true, create..."

Comment 43

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 44

[Stephen A Pohl [:spohl]]

Comment on attachment 8992430 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/257298/#review264136

::: browser/components/attribution/nsIMacAttribution.idl:24
(Diff revision 1)
> +  /**
> +   * Used by the tests.
> +   *
> +   * @param filePath A path to the file to set the quarantine data on.
> +   * @param referrer A url to set as the referrer for the download.
> +   * @param create   If true, create new quarantine properties, overwriting any

nit: s/create/creates/ (my mistake in the previous review)

::: xpcom/io/CocoaFileUtils.h:33
(Diff revision 1)
>  void     AddQuarantineMetadataToFile(const CFStringRef filePath,
>                                       const CFURLRef sourceURL,
>                                       const CFURLRef referrerURL,
> -                                     const bool isFromWeb);
> +                                     const bool isFromWeb,
> +                                     const bool createProps=false);
> +void CopyQuarantineReferrerUrl(const CFStringRef aFilePath, nsAString& aReferrer);

nit: over 80 characters.

Comment 45

[Dave Townsend [:mossop]]

Comment on attachment 8992430 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

https://reviewboard.mozilla.org/r/257298/#review265064

Looks fine. I have a vague wonder about why we guard all this by testing we're using the cocoa widget rather than the OSX platform but if spohl is happy with that then so am I.

::: browser/components/attribution/nsIMacAttribution.idl:14
(Diff revision 1)
> +interface nsIMacAttributionService : nsISupports
> +{
> +  /**
> +   * Used by the Attributions system to get the download referrer.
> +   *
> +   * @param filePath A path to the file to get the quarantine data from.

Name doesn't match the parameter in the signature.

::: browser/components/attribution/nsIMacAttribution.idl:22
(Diff revision 1)
> +   * @param filePath A path to the file to set the quarantine data on.
> +   * @param referrer A url to set as the referrer for the download.
> +   * @param create   If true, create new quarantine properties, overwriting any

These parameters don't match the 'a' prefixed versions in the actual signature.

Comment 46

[Stephen A Pohl [:spohl]]

(In reply to Dave Townsend [:mossop] from comment #45)
> Comment on attachment 8992430 [details]
> Bug 1344771 - Implement attribution on OSX using quarantine data,
> 
> https://reviewboard.mozilla.org/r/257298/#review265064
> 
> Looks fine. I have a vague wonder about why we guard all this by testing
> we're using the cocoa widget rather than the OSX platform but if spohl is
> happy with that then so am I.

From bug 691133 comment 6:

> MOZ_WIDGET_COCOA is for building against the Cocoa frameworks.
> XP_MACOSX is for building on darwin (which can be Cocoa, GTK, ...)

MOZ_WIDGET_COCOA is appropriate if we want to track attribution for Firefox on macOS-only in this patch. If there are any kind of other builds we want to track that are built on macOS, we may have to revisit.

Comment 47

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8992430 [details]
Bug 1344771 - Implement attribution on OSX using quarantine data,

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

Comment 48

[Shane Caraveo (:mixedpuppy)]

got an ok from mreid, going to land...

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7079fbaa9af
Implement attribution on OSX using quarantine data, r=mossop,spohl

Comment 50

[Chris More [:cmore]]

Question: What Firefox train (if any) will OSX attribution be on?

Comment 51

[Shane Caraveo (:mixedpuppy)]

(In reply to Chris More [:cmore] from comment #50)
> Question: What Firefox train (if any) will OSX attribution be on?

63

Comment 52

[Chris More [:cmore]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #51)
> (In reply to Chris More [:cmore] from comment #50)
> > Question: What Firefox train (if any) will OSX attribution be on?
> 
> 63

Great! Should we denote this in any tracking flags on this bug?

Comment 53

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e7079fbaa9af

Comment 54

[Chris More [:cmore]]

Stephen: We finally have attribution data for OSX and it landed on nightly. What can we do to get any QA tests working with OSX to ensure it is functions and we don't have any regressions in the future?

Comment 55

[Chris More [:cmore]]

Shane: I just noticed something above that I wanted to make sure I understood. Attribution on Windows works with the www.mozilla.org website talking to the DMO stub service that ingests the utm_* URL parameters on www.mozilla.org. If those parameters are not present, the JS on www.mozilla.org creates utm_* variables and fills with them http refer values. The parameters are utm_* because those are the parameters used across the entire Mozilla ecosystem when tagging links. Those same parameters are also used by Google Analytics to associated with specific source, medium, campaign and content tags fields. 

Question: Will attribution on OSX work with both ?source=foobar.com *and* and ?utm_source=foobar.com or only one? It really should work with utm_* by default as that is now it works on Windows and that is now it works within Google Analytics. If it only works with, for example, ?source=foobar.com then that will break web analytics. Those parameters without utm_ appended to the URL will look literally like a unique URL instead of a parameter to parse. If that is all the case and we wanted it to work with GA and OSX attribution, we would have to include ?source=foobar.com&utm_source=foodbar.com and double up all of the parameters to effectively measure the entire funnel.

Comment 56

[Shane Caraveo (:mixedpuppy)]

It only works with utm_*.  The attribution module in Firefox translates those, e.g. utm_source becomes source.  That is what telemetry then receives.

My understanding is that the windows stub only recognized utm_*.  It works a little different, but in the end, when telemetry fetches data from the attribution module, "utm_" has been stripped from the params.

There is an automated test for both windows (pre-existing) and osx in the tree, but neither are full round trip tests.

Comment 57

[Chris More [:cmore]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #56)
> It only works with utm_*.  The attribution module in Firefox translates
> those, e.g. utm_source becomes source.  That is what telemetry then receives.
> 
> My understanding is that the windows stub only recognized utm_*.  It works a
> little different, but in the end, when telemetry fetches data from the
> attribution module, "utm_" has been stripped from the params.
> 
> There is an automated test for both windows (pre-existing) and osx in the
> tree, but neither are full round trip tests.

Ok, great! Thanks for the clarification.

Comment 58

[Victor Carciu]

After talking with Shane, I used the script from https://bug1475354.bmoattachments.org/attachment.cgi?id=8995240  in order to test this bug and everything is now working as expected. 

I tested using using latest Nightly on MacOS 10.12.3 and I can confirm that I reproduced the issue on previous builds.

Here you have a postfix video :  https://www.youtube.com/watch?v=FCHMWA88pyU&feature=youtu.be