Closed Bug 1277627 Opened 4 years ago Closed 3 years ago

Implement a module for working with attribution codes

Categories

(Firefox :: General, enhancement)

Unspecified
Windows
enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: mhowell, Assigned: mhowell)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1261140 added installer support for attribution codes (see bug 1222258 and bug 1259607 for background). The next step for Firefox is to add support for working with the file that the installer patch creates, reading and validating the attribution code from it and deleting the file after it's no longer needed. 

This doesn't yet include actually sending the attribution code anywhere; another bug will be filed for that.
Hey Mike! Mind giving this an r?
Flags: needinfo?(mconley)
https://reviewboard.mozilla.org/r/62918/#review66684

::: browser/modules/AttributionCode.jsm:34
(Diff revision 1)
> +  file.append(AppConstants.MOZ_APP_NAME);
> +  file.append("postSigningData");
> +  return file;
> +}
> +
> +function readAttributionFile() {

Is there a reason to use sync file I/O here?
Can you not use async I/O using OS.File?

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread

::: browser/modules/AttributionCode.jsm:34
(Diff revision 1)
> +  file.append(AppConstants.MOZ_APP_NAME);
> +  file.append("postSigningData");
> +  return file;
> +}
> +
> +function readAttributionFile() {

Is there a reason to use sync file I/O here?
Can you not use async I/O using OS.File?

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread
https://reviewboard.mozilla.org/r/62918/#review66684

> Is there a reason to use sync file I/O here?
> Can you not use async I/O using OS.File?
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/OSFile.jsm/OS.File_for_the_main_thread

Not a strong reason. I thought it made more sense for the getter call to always be synchronous. But I'm not against changing it.
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review67780

This looks really good! Super happy to see the tests too. Just a few minor things below.

::: browser/modules/AttributionCode.jsm:10
(Diff revision 2)
> +Cu.import("resource://gre/modules/AppConstants.jsm");
> +Cu.import("resource://gre/modules/osfile.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/Task.jsm");

If these aren't being used right away, it's usually a good idea to create lazy module getters with the XPCOMUtils module. Here's an example: http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/accessible/jsat/ContentControl.jsm#9

::: browser/modules/AttributionCode.jsm:23
(Diff revision 2)
> +  "^source%3D(" + ATTR_CODE_VALUE_CHARS + "*)%26" +
> +   "medium%3D(" + ATTR_CODE_VALUE_CHARS + "*)%26" +
> +   "campaign%3D(" + ATTR_CODE_VALUE_CHARS + "*)%26" +
> +   "content%3D(" + ATTR_CODE_VALUE_CHARS + "*)$", "i");
> +
> +let cachedCode = null;

Elsewhere in browser code, we tend to prefix globals with "g" to make them easier to detect. Example: gCachedCode.

I don't think it's a hard and fast rule, but it might be a good idea here. I won't insist on it - use your best judgement.

::: browser/modules/AttributionCode.jsm:26
(Diff revision 2)
> +  let directoryService = Cc["@mozilla.org/file/directory_service;1"].
> +                         getService(Ci.nsIProperties);

You should be able to just use Services.dirsvc.

::: browser/modules/AttributionCode.jsm:39
(Diff revision 2)
> +}
> +
> +function validateAttributionCode(code) {
> +  let matches = code.match(ATTR_CODE_REGEX);
> +  return code.length <= ATTR_CODE_MAX_LENGTH &&
> +         matches != null && matches.length == 5;

Can you make this magic number a constant instead, and toss it up by the RegExp's that you're defining?

::: browser/modules/AttributionCode.jsm:43
(Diff revision 2)
> +  return code.length <= ATTR_CODE_MAX_LENGTH &&
> +         matches != null && matches.length == 5;
> +}
> +
> +var AttributionCode = {
> +  getCodeAsync() {

I know these functions are more or less self-evident, but documentation sets the tone of a new file. Can you document each of these methods - both the publicly exposed ones, and the helper ones?

::: browser/modules/AttributionCode.jsm:50
(Diff revision 2)
> +      if (cachedCode != null) {
> +        return cachedCode;
> +      }
> +
> +      try {
> +        let bytes = yield OS.File.read(getAttributionFile().path);

Off-main thread IO! Good stuff! :D

::: browser/modules/AttributionCode.jsm:69
(Diff revision 2)
> +    });
> +  },
> +
> +  deleteFile() {
> +    try {
> +      getAttributionFile().remove(false);

This, however, is main-thread sync IO. We should use OS.File.remove instead. Or is this only going to be used by tests? If so, maybe this shouldn't be in here, and should only be within a helper in the tests.

::: browser/modules/AttributionCode.jsm:79
(Diff revision 2)
> +    }
> +  },
> +
> +  // This function should probably only be used by tests.
> +  _clearCache() {
> +    cachedCode = null;

Is this only ever going to be used by xpcshell tests? If so, you could use Services.env.get("XPCSHELL_TEST_PROFILE_DIR") to see if an xpcshell test is running, and throw if it's not set.

Not obligatory, but an added layer of foot-gun protection.

::: browser/modules/test/xpcshell/test_AttributionCode.js:49
(Diff revision 2)
> +   valid: false},
> +  {code: "source%reallyreallyreallyreallyreallyreallyreallyreallyreallylongdomain.com%26medium%3Dorganic%26campaign%3D(not%20set)%26content%3Dalmostexactlyenoughcontenttomakethisstringlongerthanthe200characterlimit",
> +   valid: false},
> +];
> +
> +add_task(function* testAttrCodes() {

Each of these add_tasks should have a brief description of what's being tested.

::: browser/modules/test/xpcshell/test_AttributionCode.js:59
(Diff revision 2)
> +    if (entry.valid) {
> +      Assert.equal(result, entry.code, "Code should be valid");
> +    } else {
> +      Assert.equal(result, "", "Code should not be valid");
> +    }
> +  }

We should probably clearCache here to not taint any future tests.

::: browser/modules/test/xpcshell/test_AttributionCode.js:66
(Diff revision 2)
> +
> +add_task(function* testDeletedFile() {
> +  // Set up the test by clearing the cache and writing a valid file.
> +  const code =
> +    "source%3Dgoogle.com%26medium%3Dorganic%26campaign%3D(not%20set)%26content%3D(not%20set)";
> +  AttributionCode._clearCache();

Probably not necessary if we clear after the above test.
Attachment #8768901 - Flags: review-
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review67780

> Elsewhere in browser code, we tend to prefix globals with "g" to make them easier to detect. Example: gCachedCode.
> 
> I don't think it's a hard and fast rule, but it might be a good idea here. I won't insist on it - use your best judgement.

Maybe not strictly necessary here, but it's not worth violating the convention over.

> This, however, is main-thread sync IO. We should use OS.File.remove instead. Or is this only going to be used by tests? If so, maybe this shouldn't be in here, and should only be within a helper in the tests.

This is intended to be used by browser code; the idea is that the file sort of "expires" after it's no longer needed, so we don't keep the data sitting around forever.

> Is this only ever going to be used by xpcshell tests? If so, you could use Services.env.get("XPCSHELL_TEST_PROFILE_DIR") to see if an xpcshell test is running, and throw if it's not set.
> 
> Not obligatory, but an added layer of foot-gun protection.

The Services.env alias seems to no longer exist, but the service itself still appears to work.
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review68306

This looks good to me, once the comments are in the proper block format. Thanks Matt!

::: browser/modules/AttributionCode.jsm:31
(Diff revision 3)
> +   "content%3D(" + ATTR_CODE_VALUE_CHARS + "*)$", "i");
> +const ATTR_CODE_REGEX_NUM_GROUPS = 4;
> +
> +let gCachedCode = null;
> +
> +// Returns an nsIFile for the file containing the attribution data.

Nit: I think most function documentation in our code uses the /* */, for example:

```
// Returns true if the passed-in attribution code string fits the expected format,
// or false otherwise
```

becomes

```
/**
 * Returns true if the passed-in attribution code string fits the expected format,
 * or false otherwise
 */
```

::: browser/modules/test/xpcshell/xpcshell.ini:8
(Diff revision 3)
>  tail =
>  firefox-appdir = browser
>  skip-if = toolkit == 'android' || toolkit == 'gonk'
>  
> +[test_AttributionCode.js]
> +skip-if = os != 'win'

I assume we're not going to be doing anything similar for the other platforms?
Attachment #8768901 - Flags: review?(mconley) → review+
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review68306

Okay, I can fix that, then. Thanks for the review!

> I assume we're not going to be doing anything similar for the other platforms?

No, we don't have any plans to. Currently, we lean pretty heavily on the Windows installer to create the data that this module reads; I'm not sure how to do it on any other platform.
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e1a9d35f9a8
Added module for working with attribution codes, including tests; r=mconley
Backed out for ESlint warnings:

https://hg.mozilla.org/integration/autoland/rev/4bfeac7af13b

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8e1a9d35f9a842a4ee85799ccc6c03407760d4c4
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=1763535&repo=autoland

[task 2016-08-11T15:55:56.576703Z] TEST-UNEXPECTED-ERROR | browser/modules/AttributionCode.jsm:15:52 | Expected linebreaks to be 'LF' but found 'CRLF'. (linebreak-style)
[task 2016-08-11T15:55:56.577411Z] TEST-UNEXPECTED-ERROR | browser/modules/AttributionCode.jsm:70:9 | Expected space(s) after "catch". (keyword-spacing)
[task 2016-08-11T15:55:56.577580Z] TEST-UNEXPECTED-ERROR | browser/modules/AttributionCode.jsm:93:9 | Expected space(s) after "catch". (keyword-spacing)
Flags: needinfo?(mhowell)
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/051f3d4d56d8
Added module for working with attribution codes, including tests; r=mconley
Relanded with fixes.
Flags: needinfo?(mhowell)
And from early results, it looks like this only affects Windows 7 and 8, not XP.
Interesting. Well, I need to modify this patch anyway to accommodate a requirement I didn't think about (namely support for not all of the attribution parameters being present); I'll get this fixed for that version.
Flags: needinfo?(mhowell)
The updated patch has the changes mentioned in comment 19; I've also pushed it to try so I can check my guess about what was causing the test failures.
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review70332

I had to make pretty substantial changes to implement a requirement for accepting codes that contain only some of the four parameters, and to be able to implement bug 1292360. I think the changes are big enough to require re-review.
Attachment #8768901 - Flags: review?(mhowell)
Attachment #8768901 - Flags: review?(mhowell)
Attachment #8768901 - Flags: review?(mconley)
Attachment #8768901 - Flags: review+
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

https://reviewboard.mozilla.org/r/62918/#review70338

I've looked at the interdiff, and it makes sense to me. r=me.
Attachment #8768901 - Flags: review?(mconley) → review+
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #23)
> Comment on attachment 8768901 [details]
> Bug 1277627 - Added module for working with attribution codes, including
> tests;
> 
> https://reviewboard.mozilla.org/r/62918/#review70338
> 
> I've looked at the interdiff, and it makes sense to me. r=me.

That was quick! Thanks, Mike.
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0e016284bbd
Added module for working with attribution codes, including tests; r=mconley
https://hg.mozilla.org/mozilla-central/rev/c0e016284bbd
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

Approval Request Comment
[Feature/regressing bug #]:
Attribution is a new feature; bug 1259607 is the tracking bug.

[User impact if declined]:
Impact is to schedule targets for the attribution project; the sooner this data gets into telemetry, the sooner it can be used to better understand how users are getting Firefox.

[Describe test coverage new/current, TreeHerder]:
This patch adds tests for the new code; see above comments for links to pushes with test runs.

[Risks and why]: 
Risk is low to nonexistent; this patch is all new code which isn't even called from anywhere until bug 1292360 lands, and appropriate tests are included.

[String/UUID change made/needed]:
None

Note: I plan to also make this request for bug 1292360 as soon as it's ready.
Attachment #8768901 - Flags: approval-mozilla-aurora?
Hi Matt, is the stub attribution work planned for Fx50? I think it's a bit late in the Aurora50 cycle to be uplifting a new concept (don't want to call it a feature). I understand the risk for uplifting this is low as it's dead code until another patch that calls into it is uplifted. However, if we find ourselves running into crash/perf issues, is there a way to turn this off? Or will we need to backout this patch? 

Other than this bug and bug 1292360, are there other bug fixes that will need to be uplifted for Fx50? Just trying to get an idea of timeline and beta readiness on this one.
Flags: needinfo?(mhowell)
(In reply to Ritu Kothari (:ritu) from comment #29)
> Hi Matt, is the stub attribution work planned for Fx50? I think it's a bit
> late in the Aurora50 cycle to be uplifting a new concept (don't want to call
> it a feature). I understand the risk for uplifting this is low as it's dead
> code until another patch that calls into it is uplifted. However, if we find
> ourselves running into crash/perf issues, is there a way to turn this off?
> Or will we need to backout this patch? 
I'm going to redirect the schedule question to ckprice; he's the PM for this work.

With bug 1292360, attribution is baked in to telemetry, so a backout of that patch would be the only way to get it turned off.

> Other than this bug and bug 1292360, are there other bug fixes that will
> need to be uplifted for Fx50? Just trying to get an idea of timeline and
> beta readiness on this one.
No, these two are the only bugs involved.
Flags: needinfo?(mhowell) → needinfo?(cprice)
(In reply to Ritu Kothari (:ritu) from comment #29)
> Hi Matt, is the stub attribution work planned for Fx50?

This work is planned for Fx 50. You can find a further breakdown for the business case in bug1222258comment0.
Flags: needinfo?(cprice)
(In reply to Cory Price [:ckprice] from comment #31)
> (In reply to Ritu Kothari (:ritu) from comment #29)
> > Hi Matt, is the stub attribution work planned for Fx50?
> 
> This work is planned for Fx 50. You can find a further breakdown for the
> business case in bug1222258comment0.

Thanks Matt and Cory. I will approve the uplift to Aurora. Hoping for a smooth ride on this one.
Comment on attachment 8768901 [details]
Bug 1277627 - Added module for working with attribution codes, including tests;

This is planned feature work for 50, Aurora+
Attachment #8768901 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.