Backed out changeset d63aab4e0688 (bug 1733535) for causing mochitest failures on browser_AttributionCode_telemetry.js CLOSED TREE
authorNorisz Fay <nfay@mozilla.com>
Fri, 22 Oct 2021 16:32:43 +0300
changeset 596738 ad7f2081d3672aef29c228c7510bbeac4d02b4e5
parent 596737 1145d08e7cd5574dc13de04acb201fad276bb408
child 596739 c20bf0436442406286ab88ed0d8fc7f76da2206f
push id152015
push usernfay@mozilla.com
push dateFri, 22 Oct 2021 13:33:45 +0000
treeherderautoland@ad7f2081d367 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1733535
milestone95.0a1
backs outd63aab4e0688bca6b0c2bf6ab63a036f6b0e0bcf
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Backed out changeset d63aab4e0688 (bug 1733535) for causing mochitest failures on browser_AttributionCode_telemetry.js CLOSED TREE
browser/components/attribution/AttributionCode.jsm
browser/components/attribution/MacAttribution.jsm
browser/components/attribution/test/browser/browser_AttributionCode_Mac_telemetry.js
browser/components/attribution/test/browser/browser_AttributionCode_telemetry.js
browser/components/attribution/test/xpcshell/head.js
--- a/browser/components/attribution/AttributionCode.jsm
+++ b/browser/components/attribution/AttributionCode.jsm
@@ -1,32 +1,24 @@
 /* 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/. */
 "use strict";
 
-var EXPORTED_SYMBOLS = ["AttributionCode", "AttributionIOUtils"];
-
-/**
- * This is a policy object used to override behavior for testing.
- */
-const AttributionIOUtils = {
-  writeUTF8: async (path, bytes) => IOUtils.writeUTF8(path, bytes),
-  readUTF8: async path => IOUtils.readUTF8(path),
-  exists: async path => IOUtils.exists(path),
-};
+var EXPORTED_SYMBOLS = ["AttributionCode"];
 
 const { XPCOMUtils } = ChromeUtils.import(
   "resource://gre/modules/XPCOMUtils.jsm"
 );
 ChromeUtils.defineModuleGetter(
   this,
   "AppConstants",
   "resource://gre/modules/AppConstants.jsm"
 );
+ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
 ChromeUtils.defineModuleGetter(
   this,
   "Services",
   "resource://gre/modules/Services.jsm"
 );
 ChromeUtils.defineModuleGetter(
   this,
   "MacAttribution",
@@ -126,17 +118,18 @@ var AttributionCode = {
       // `OS.File.makeDir` has an awkward API for our situation.
       dir.create(Ci.nsIFile.DIRECTORY_TYPE, 0o755);
     } catch (ex) {
       if (ex.result != Cr.NS_ERROR_FILE_ALREADY_EXISTS) {
         throw ex;
       }
       // Ignore the exception due to a directory that already exists.
     }
-    await AttributionIOUtils.writeUTF8(file.path, code);
+    let bytes = new TextEncoder().encode(code);
+    await OS.File.writeAtomic(file.path, bytes);
   },
 
   /**
    * Returns an array of allowed attribution code keys.
    */
   get allowedCodeKeys() {
     return [...ATTR_CODE_KEYS];
   },
@@ -261,17 +254,17 @@ var AttributionCode = {
     if (!attributionFile) {
       // This platform doesn't support attribution.
       log.debug(`getAttrDataAsync: no attribution (attributionFile is null)`);
       return gCachedAttrData;
     }
 
     if (
       AppConstants.platform == "macosx" &&
-      !(await AttributionIOUtils.exists(attributionFile.path))
+      !(await OS.File.exists(attributionFile.path))
     ) {
       log.debug(
         `getAttrDataAsync: macOS && !exists("${attributionFile.path}")`
       );
 
       // On macOS, we fish the attribution data from the system quarantine DB.
       try {
         let referrer = await MacAttribution.getReferrerUrl();
@@ -317,34 +310,36 @@ var AttributionCode = {
       log.debug(
         `Returning after successfully writing "${attributionFile.path}"`
       );
       return gCachedAttrData;
     }
 
     log.debug(`getAttrDataAsync: !macOS || !exists("${attributionFile.path}")`);
 
-    let code;
+    let bytes;
     try {
-      code = await AttributionIOUtils.readUTF8(attributionFile.path);
+      bytes = await OS.File.read(attributionFile.path);
     } catch (ex) {
-      if (ex instanceof DOMException && ex.name == "NotFoundError") {
+      if (ex instanceof OS.File.Error && ex.becauseNoSuchFile) {
         log.debug(
           `getAttrDataAsync: !exists("${
             attributionFile.path
           }"), returning ${JSON.stringify(gCachedAttrData)}`
         );
         return gCachedAttrData;
       }
       Services.telemetry
         .getHistogramById("BROWSER_ATTRIBUTION_ERRORS")
         .add("read_error");
     }
-    if (code) {
+    if (bytes) {
       try {
+        let decoder = new TextDecoder();
+        let code = decoder.decode(bytes);
         log.debug(
           `getAttrDataAsync: ${attributionFile.path} deserializes to ${code}`
         );
         if (AppConstants.platform == "macosx" && !code) {
           // On macOS, an empty attribution code is fine.  (On Windows, that
           // means the stub/full installer has been incorrectly attributed,
           // which is an error.)
           return gCachedAttrData;
@@ -379,17 +374,17 @@ var AttributionCode = {
 
   /**
    * Deletes the attribution data file.
    * Returns a promise that resolves when the file is deleted,
    * or if the file couldn't be deleted (the promise is never rejected).
    */
   async deleteFileAsync() {
     try {
-      await IOUtils.remove(this.attributionFile.path);
+      await OS.File.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.
     }
   },
 
   /**
--- a/browser/components/attribution/MacAttribution.jsm
+++ b/browser/components/attribution/MacAttribution.jsm
@@ -49,17 +49,16 @@ function getQuarantineDatabasePath() {
  * Query given path for quarantine extended attributes.
  * @param {String} path of the file to query.
  * @return {[String, String]} pair of the quarantine data GUID and remaining
  *                            quarantine data (usually, Gatekeeper flags).
  * @throws NS_ERROR_NOT_AVAILABLE if there is no quarantine GUID for the given path.
  * @throws NS_ERROR_UNEXPECTED if there is a quarantine GUID, but it is malformed.
  */
 async function getQuarantineAttributes(path) {
-  // TODO: Bug 1736331 replace OS.File.macGetXAttr with an alternative.
   let bytes = await OS.File.macGetXAttr(path, "com.apple.quarantine");
   if (!bytes) {
     throw new Components.Exception(
       `No macOS quarantine xattrs found for ${path}`,
       Cr.NS_ERROR_NOT_AVAILABLE
     );
   }
 
--- a/browser/components/attribution/test/browser/browser_AttributionCode_Mac_telemetry.js
+++ b/browser/components/attribution/test/browser/browser_AttributionCode_Mac_telemetry.js
@@ -1,33 +1,32 @@
 ChromeUtils.defineModuleGetter(
   this,
   "TelemetryTestUtils",
   "resource://testing-common/TelemetryTestUtils.jsm"
 );
 const { MacAttribution } = ChromeUtils.import(
   "resource:///modules/MacAttribution.jsm"
 );
-const { AttributionIOUtils } = ChromeUtils.import(
-  "resource:///modules/AttributionCode.jsm"
-);
 const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 
 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));
+  ok(await OS.File.exists(AttributionCode.attributionFile.path));
   Assert.deepEqual(
     "",
-    await AttributionIOUtils.readUTF8(AttributionCode.attributionFile.path)
+    new TextDecoder().decode(
+      await OS.File.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 || {});
 }
@@ -45,42 +44,42 @@ add_task(async function test_write_error
 
   await AttributionCode.deleteFileAsync();
   AttributionCode._clearCache();
 
   const histogram = Services.telemetry.getHistogramById(
     "BROWSER_ATTRIBUTION_ERRORS"
   );
 
-  let oldExists = AttributionIOUtils.exists;
-  let oldWrite = AttributionIOUtils.writeUTF8;
   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
+    // because various background tasks may invoke `OS.File.writeAtomic` while
     // this test is running.  Be careful to only stub the one call.
-    AttributionIOUtils.exists = () => false;
-    AttributionIOUtils.writeUTF8 = () => {
-      throw new Error("write_error");
-    };
+    const writeAtomic = sandbox.stub(OS.File, "writeAtomic");
+    writeAtomic
+      .withArgs(
+        sinon.match(AttributionCode.attributionFile.path),
+        sinon.match.any
+      )
+      .throws(() => new Error("write_error"));
+    OS.File.writeAtomic.callThrough();
 
     // 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.writeUTF8 = oldWrite;
     await AttributionCode.deleteFileAsync();
     AttributionCode._clearCache();
     histogram.clear();
     sandbox.restore();
   }
 });
 
 add_task(async function test_unusual_referrer() {
@@ -210,17 +209,16 @@ add_task(async function test_broken_refe
   // These magic constants are macOS GateKeeper flags.
   let string = [
     "01c1",
     "5991b778",
     "Safari.app",
     generateQuarantineGUID(),
   ].join(";");
   let bytes = new TextEncoder().encode(string);
-  // TODO: Bug 1736331 replace OS.File.macSetXAttr with an alternative.
   await OS.File.macSetXAttr(
     MacAttribution.applicationPath,
     "com.apple.quarantine",
     bytes
   );
 
   await AttributionCode.deleteFileAsync();
   AttributionCode._clearCache();
--- a/browser/components/attribution/test/browser/browser_AttributionCode_telemetry.js
+++ b/browser/components/attribution/test/browser/browser_AttributionCode_telemetry.js
@@ -1,16 +1,14 @@
 ChromeUtils.defineModuleGetter(
   this,
   "TelemetryTestUtils",
   "resource://testing-common/TelemetryTestUtils.jsm"
 );
-const { AttributionIOUtils } = ChromeUtils.import(
-  "resource:///modules/AttributionCode.jsm"
-);
+const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 
 add_task(async function test_parse_error() {
   if (AppConstants.platform == "macosx") {
     // On macOS, the underlying data is the OS-level quarantine
     // database.  We need to start from nothing to isolate the cache.
     const { MacAttribution } = ChromeUtils.import(
       "resource:///modules/MacAttribution.jsm"
     );
@@ -59,38 +57,30 @@ add_task(async function test_parse_error
 add_task(async function test_read_error() {
   registerCleanupFunction(async () => {
     await AttributionCode.deleteFileAsync();
     AttributionCode._clearCache();
   });
   const histogram = Services.telemetry.getHistogramById(
     "BROWSER_ATTRIBUTION_ERRORS"
   );
-
+  const sandbox = sinon.createSandbox();
   // Delete the file to trigger a read error
   await AttributionCode.deleteFileAsync();
   AttributionCode._clearCache();
   // Clear any existing telemetry
   histogram.clear();
 
   // Force the file to exist but then cause a read error
-  let oldExists = AttributionIOUtils.exists;
-  AttributionIOUtils.exists = () => true;
-
-  let oldRead = AttributionIOUtils.readUTF8;
-  AttributionIOUtils.readUTF8 = () => {
-    throw new Error("read_error");
-  };
-
-  registerCleanupFunction(() => {
-    AttributionIOUtils.exists = oldExists;
-    AttributionIOUtils.readUTF8 = oldRead;
-  });
-
+  const exists = sandbox.stub(OS.File, "exists");
+  exists.resolves(true);
+  const read = sandbox.stub(OS.File, "read");
+  read.throws(() => new Error("read_error"));
   // Try to read the file
   await AttributionCode.getAttrDataAsync();
 
   // It should record the read error
   TelemetryTestUtils.assertHistogram(histogram, INDEX_READ_ERROR, 1);
 
   // Clear any existing telemetry
   histogram.clear();
+  sandbox.restore();
 });
--- a/browser/components/attribution/test/xpcshell/head.js
+++ b/browser/components/attribution/test/xpcshell/head.js
@@ -79,16 +79,17 @@ let invalidAttrCodes = [
  * to follow.  In the App Update Service tests, an `nsIDirectoryServiceProvider`
  * is installed, which is global and much harder to extract for re-use.
  */
 async function setupStubs() {
   // Local imports to avoid polluting the global namespace.
   const { AppConstants } = ChromeUtils.import(
     "resource://gre/modules/AppConstants.jsm"
   );
+  const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm");
   const { sinon } = ChromeUtils.import("resource://testing-common/Sinon.jsm");
 
   // This depends on the caller to invoke it by name.  We do try to
   // prevent the most obvious incorrect invocation, namely
   // `add_task(setupStubs)`.
   let caller = Components.stack.caller;
   const testID = caller.filename
     .toString()
@@ -111,12 +112,10 @@ async function setupStubs() {
       .get(() => applicationFile.path);
   }
 
   // The macOS quarantine database applies to existing paths only, so make
   // sure our mock application path exists.  This also creates the parent
   // directory for the attribution file, needed on both macOS and Windows.  We
   // don't ignore existing paths because we're inside a temporary directory:
   // this should never be invoked twice for the same test.
-  await IOUtils.makeDirectory(applicationFile.path, {
-    from: do_get_tempdir().path,
-  });
+  await OS.File.makeDir(applicationFile.path, { from: do_get_tempdir().path });
 }