Skip to content

Commit

Permalink
Bug 1865845: stop caching mac attribution data in a separate file r=n…
Browse files Browse the repository at this point in the history 
…alexander

This is mostly removing code and tests related to reading/writing the cache file on macOS and updating of tests. Aside from that, the most notable part is the change to `setAttributionString` to automatically prepend `__MOZCUSTOM__` when writing an attribution code. This is mostly done to make things simpler and cleaner in the majority of the tests, but seeing as `getAttributionString` is also aware of it, it seems like a generally nicer way to do this.

Differential Revision: https://phabricator.services.mozilla.com/D197204
  • Loading branch information
@bhearsum
bhearsum committed Jan 9, 2024
1 parent 8d7a597 commit 352258d
Show file tree
Hide file tree
Showing 12 changed files with 88 additions and 199 deletions.
111 changes: 22 additions & 89 deletions browser/components/attribution/AttributionCode.sys.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { AppConstants } from "resource://gre/modules/AppConstants.sys.mjs";
const lazy = {};
ChromeUtils.defineESModuleGetters(lazy, {
MacAttribution: "resource:///modules/MacAttribution.sys.mjs",
UpdateUtils: "resource://gre/modules/UpdateUtils.sys.mjs",
});
ChromeUtils.defineLazyGetter(lazy, "log", () => {
let { ConsoleAPI } = ChromeUtils.importESModule(
Expand Down Expand Up @@ -82,49 +81,6 @@ export var AttributionCode = {
let file = Services.dirsvc.get("GreD", Ci.nsIFile);
file.append("postSigningData");
return file;
} else if (AppConstants.platform == "macosx") {
// There's no `UpdRootD` in xpcshell tests. Some existing tests override
// it, which is onerous and difficult to share across tests. When testing,
// if it's not defined, fallback to a nested subdirectory of the xpcshell
// temp directory. Nesting more closely replicates the situation where the
// update directory does not (yet) exist, testing a scenario witnessed in
// development.
let file;
try {
file = Services.dirsvc.get("UpdRootD", Ci.nsIFile);
} catch (ex) {
// It's most common to test for the profile dir, even though we actually
// are using the temp dir.
if (
ex instanceof Ci.nsIException &&
ex.result == Cr.NS_ERROR_FAILURE &&
Services.env.exists("XPCSHELL_TEST_PROFILE_DIR")
) {
let path = Services.env.get("XPCSHELL_TEST_TEMP_DIR");
file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
file.initWithPath(path);
file.append("nested_UpdRootD_1");
file.append("nested_UpdRootD_2");
} else {
throw ex;
}
}
// Note: this file is in a location that includes the absolute path
// to the running install, and the filename includes the update channel.
// To ensure consistency regardless of when `attributionFile` is accessed we
// explicitly do not include partner IDs that may be part of the full update channel.
// These are not necessarily applied when this is first accessed, and we want to
// ensure consistency between early and late accesses.
// Partner builds never contain attribution information, so this has no known
// consequences.
// For example:
// ~/Library/Caches/Mozilla/updates/Applications/Firefox/macAttributionDataCache-release
// This is done to ensure that attribution data is preserved through a
// pave over install of an install on the same channel.
file.append(
"macAttributionDataCache-" + lazy.UpdateUtils.getUpdateChannel(false)
);
return file;
}

return null;
Expand All @@ -135,8 +91,8 @@ export var AttributionCode = {
* @param {String} code to write.
*/
async writeAttributionFile(code) {
// Writing attribution files is only used as part of test code, and Mac
// attribution, so bailing here for MSIX builds is no big deal.
// Writing attribution files is only used as part of test code
// so bailing here for MSIX builds is no big deal.
if (
AppConstants.platform === "win" &&
Services.sysinfo.getProperty("hasWinPackageId")
Expand Down Expand Up @@ -228,14 +184,12 @@ export var AttributionCode = {
},

async _getMacAttrDataAsync() {
let attributionFile = this.attributionFile;

// On macOS, we fish the attribution data from an extended attribute on
// the .app bundle directory.
try {
let attrStr = await lazy.MacAttribution.getAttributionString();
lazy.log.debug(
`getAttrDataAsync: macOS attribution getAttributionString: "${attrStr}"`
`_getMacAttrDataAsync: getAttributionString: "${attrStr}"`
);

gCachedAttrData = this.parseAttributionCode(attrStr);
Expand All @@ -261,22 +215,6 @@ export var AttributionCode = {
`macOS attribution data is ${JSON.stringify(gCachedAttrData)}`
);

// We only want to try to fetch the attribution string once on macOS
try {
let code = this.serializeAttributionData(gCachedAttrData);
lazy.log.debug(`macOS attribution data serializes as "${code}"`);
await this.writeAttributionFile(code);
} catch (ex) {
lazy.log.debug(`Caught exception writing "${attributionFile.path}"`, ex);
Services.telemetry
.getHistogramById("BROWSER_ATTRIBUTION_ERRORS")
.add("write_error");
return gCachedAttrData;
}

lazy.log.debug(
`Returning after successfully writing "${attributionFile.path}"`
);
return gCachedAttrData;
},

Expand Down Expand Up @@ -308,6 +246,10 @@ export var AttributionCode = {
* strip "utm_" while retrieving the params.
*/
async getAttrDataAsync() {
if (AppConstants.platform != "win" && AppConstants.platform != "macosx") {
// This platform doesn't support attribution.
return gCachedAttrData;
}
if (gCachedAttrData != null) {
lazy.log.debug(
`getAttrDataAsync: attribution is cached: ${JSON.stringify(
Expand All @@ -329,37 +271,25 @@ export var AttributionCode = {
}

gCachedAttrData = {};
let attributionFile = this.attributionFile;
if (!attributionFile) {
// This platform doesn't support attribution.
lazy.log.debug(
`getAttrDataAsync: no attribution (attributionFile is null)`
);
return gCachedAttrData;
}

if (
AppConstants.platform == "macosx" &&
!(await AttributionIOUtils.exists(attributionFile.path))
) {
lazy.log.debug(
`getAttrDataAsync: macOS && !exists("${attributionFile.path}")`
);
if (AppConstants.platform == "macosx") {
lazy.log.debug(`getAttrDataAsync: macOS`);
return this._getMacAttrDataAsync();
}

lazy.log.debug(
`getAttrDataAsync: !macOS || !exists("${attributionFile.path}")`
);
lazy.log.debug("getAttrDataAsync: !macOS");

let attributionFile = this.attributionFile;
let bytes;
try {
if (
AppConstants.platform === "win" &&
Services.sysinfo.getProperty("hasWinPackageId")
) {
lazy.log.debug("getAttrDataAsync: MSIX");
bytes = await this._getWindowsMSIXAttrDataAsync();
} else {
lazy.log.debug("getAttrDataAsync: NSIS");
bytes = await this._getWindowsNSISAttrDataAsync();
}
} catch (ex) {
Expand Down Expand Up @@ -429,12 +359,15 @@ export var AttributionCode = {
* or if the file couldn't be deleted (the promise is never rejected).
*/
async deleteFileAsync() {
try {
await IOUtils.remove(this.attributionFile.path);
} catch (ex) {
// The attribution file may already have been deleted,
// or it may have never been installed at all;
// failure to delete it isn't an error.
// There is no cache file on macOS
if (AppConstants.platform == "win") {
try {
await IOUtils.remove(this.attributionFile.path);
} catch (ex) {
// The attribution file may already have been deleted,
// or it may have never been installed at all;
// failure to delete it isn't an error.
}
}
},

Expand Down
2 changes: 1 addition & 1 deletion browser/components/attribution/MacAttribution.sys.mjs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export var MacAttribution = {
return IOUtils.setMacXAttr(
path,
"com.apple.application-instance",
new TextEncoder().encode(aAttrStr)
new TextEncoder().encode(`__MOZCUSTOM__${aAttrStr}`)
);
},

Expand Down
4 changes: 3 additions & 1 deletion browser/components/attribution/test/browser/browser.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,6 @@ prefs = ["browser.attribution.macos.enabled=true"]
skip-if = ["toolkit != 'cocoa'"] # macOS only telemetry.

["browser_AttributionCode_telemetry.js"]
skip-if = ["(os != 'win' && toolkit != 'cocoa')"] # Windows and macOS only telemetry.
# These tests only cover the attribution cache file - which only exists on
# Windows.
skip-if = ["os != 'win'"]
88 changes: 12 additions & 76 deletions browser/components/attribution/test/browser/browser_AttributionCode_Mac_telemetry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,77 +11,20 @@ const { sinon } = ChromeUtils.importESModule(
"resource://testing-common/Sinon.sys.mjs"
);

async function assertCacheExistsAndIsEmpty() {
// We should have written to the cache, and be able to read back
// with no errors.
const histogram = Services.telemetry.getHistogramById(
"BROWSER_ATTRIBUTION_ERRORS"
);
histogram.clear();

ok(await AttributionIOUtils.exists(AttributionCode.attributionFile.path));
Assert.deepEqual(
"",
new TextDecoder().decode(
await AttributionIOUtils.read(AttributionCode.attributionFile.path)
)
);

AttributionCode._clearCache();
let result = await AttributionCode.getAttrDataAsync();
Assert.deepEqual(result, {}, "Should be able to get cached result");

Assert.deepEqual({}, histogram.snapshot().values || {});
}

add_task(async function test_write_error() {
const sandbox = sinon.createSandbox();
await MacAttribution.setAttributionString("__MOZCUSTOM__content%3Dcontent");

await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();

const histogram = Services.telemetry.getHistogramById(
"BROWSER_ATTRIBUTION_ERRORS"
);

let oldExists = AttributionIOUtils.exists;
let oldWrite = AttributionIOUtils.write;
try {
// Clear any existing telemetry
histogram.clear();

// Force the file to not exist and then cause a write error. This is delicate
// because various background tasks may invoke `IOUtils.writeAtomic` while
// this test is running. Be careful to only stub the one call.
AttributionIOUtils.exists = () => false;
AttributionIOUtils.write = () => {
throw new Error("write_error");
};

// Try to read the attribution code.
let result = await AttributionCode.getAttrDataAsync();
Assert.deepEqual(
result,
{ content: "content" },
"Should be able to get a result even if the file doesn't write"
);

TelemetryTestUtils.assertHistogram(histogram, INDEX_WRITE_ERROR, 1);
} finally {
AttributionIOUtils.exists = oldExists;
AttributionIOUtils.write = oldWrite;
await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();
histogram.clear();
sandbox.restore();
}
});

add_task(async function test_blank_attribution() {
// Ensure no attribution information is present
await MacAttribution.delAttributionString();
await AttributionCode.deleteFileAsync();
try {
await MacAttribution.delAttributionString();
} catch (ex) {
// NS_ERROR_DOM_NOT_FOUND_ERR means there was not an attribution
// string to delete - which we can safely ignore.
if (
!(ex instanceof Ci.nsIException) ||
ex.result != Cr.NS_ERROR_DOM_NOT_FOUND_ERR
) {
throw ex;
}
}
AttributionCode._clearCache();

const histogram = Services.telemetry.getHistogramById(
Expand All @@ -97,10 +40,7 @@ add_task(async function test_blank_attribution() {
Assert.deepEqual(result, {}, "Should be able to get empty result");

Assert.deepEqual({}, histogram.snapshot().values || {});

await assertCacheExistsAndIsEmpty();
} finally {
await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();
histogram.clear();
}
Expand All @@ -111,7 +51,6 @@ add_task(async function test_no_attribution() {
let newApplicationPath = MacAttribution.applicationPath + ".test";
sandbox.stub(MacAttribution, "applicationPath").get(() => newApplicationPath);

await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();

const histogram = Services.telemetry.getHistogramById(
Expand All @@ -128,10 +67,7 @@ add_task(async function test_no_attribution() {
Assert.deepEqual(result, {}, "Should be able to get empty result");

Assert.deepEqual({}, histogram.snapshot().values || {});

await assertCacheExistsAndIsEmpty();
} finally {
await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();
histogram.clear();
sandbox.restore();
Expand Down
12 changes: 1 addition & 11 deletions browser/components/attribution/test/browser/browser_AttributionCode_telemetry.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,6 @@ const { AttributionIOUtils } = ChromeUtils.importESModule(
);

add_task(async function test_parse_error() {
if (AppConstants.platform == "macosx") {
const { MacAttribution } = ChromeUtils.importESModule(
"resource:///modules/MacAttribution.sys.mjs"
);
MacAttribution.setAttributionString("");
}

registerCleanupFunction(async () => {
await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();
Expand Down Expand Up @@ -41,10 +34,7 @@ add_task(async function test_parse_error() {
) {
await AttributionCode.deleteFileAsync();
AttributionCode._clearCache();
// Empty string is valid on macOS.
await AttributionCode.writeAttributionFile(
AppConstants.platform == "macosx" ? "invalid" : ""
);
await AttributionCode.writeAttributionFile("");
result = await AttributionCode.getAttrDataAsync();
Assert.deepEqual(result, {}, "Should have failed to parse");

Expand Down
21 changes: 18 additions & 3 deletions browser/components/attribution/test/xpcshell/test_AttributionCode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,14 @@ add_task(async function testValidAttrCodes() {
// are not URI encoded, and the AttributionCode code that deals with
// them expects that - so we have to simulate that as well.
msixCampaignIdStub.callsFake(async () => decodeURIComponent(currentCode));
} else if (AppConstants.platform === "macosx") {
const { MacAttribution } = ChromeUtils.importESModule(
"resource:///modules/MacAttribution.sys.mjs"
);

await MacAttribution.setAttributionString(currentCode);
} else {
// non-msix windows
await AttributionCode.writeAttributionFile(currentCode);
}
AttributionCode._clearCache();
Expand Down Expand Up @@ -80,7 +87,14 @@ add_task(async function testInvalidAttrCodes() {
}

msixCampaignIdStub.callsFake(async () => decodeURIComponent(currentCode));
} else if (AppConstants.platform === "macosx") {
const { MacAttribution } = ChromeUtils.importESModule(
"resource:///modules/MacAttribution.sys.mjs"
);

await MacAttribution.setAttributionString(currentCode);
} else {
// non-msix windows
await AttributionCode.writeAttributionFile(currentCode);
}
AttributionCode._clearCache();
Expand All @@ -102,11 +116,12 @@ add_task(async function testInvalidAttrCodes() {
* and making sure we still get the expected code.
*/
let condition = {
// MSIX attribution codes are not cached by us, thus this test is
// macOS and MSIX attribution codes are not cached by us, thus this test is
// unnecessary for those builds.
skip_if: () =>
AppConstants.platform === "win" &&
Services.sysinfo.getProperty("hasWinPackageId"),
(AppConstants.platform === "win" &&
Services.sysinfo.getProperty("hasWinPackageId")) ||
AppConstants.platform === "macosx",
};
add_task(condition, async function testDeletedFile() {
// Set up the test by clearing the cache and writing a valid file.
Expand Down
Loading

0 comments on commit 352258d

Please sign in to comment.