Merge autoland to mozilla-central a=merge

.eslintrc.js

@@ -56,7 +56,6 @@ module.exports = {
       "browser/modules/test/browser/**",
       "browser/tools/mozscreenshots/browser_boundingbox.js",
       "devtools/client/inspector/extensions/test/head_devtools_inspector_sidebar.js",
-      "services/**",
       "storage/test/unit/**",
       "testing/marionette/test/unit/**",
       "toolkit/components/**",

browser/base/content/browser-siteIdentity.js

@@ -201,6 +201,10 @@ var gIdentityHandler = {
     delete this._popupExpander;
     return this._popupExpander = document.getElementById("identity-popup-security-expander");
   },
+  get _clearSiteDataFooter() {
+    delete this._clearSiteDataFooter;
+    return this._clearSiteDataFooter = document.getElementById("identity-popup-clear-sitedata-footer");
+  },
   get _permissionAnchors() {
     delete this._permissionAnchors;
     let permissionAnchors = {};
@@ -210,6 +214,39 @@ var gIdentityHandler = {
     return this._permissionAnchors = permissionAnchors;
   },
 
+  /**
+   * Handles clicks on the "Clear Cookies and Site Data" button.
+   */
+  async clearSiteData(event) {
+    if (!this._uriHasHost) {
+      return;
+    }
+
+    let host = this._uri.host;
+
+    // Site data could have changed while the identity popup was open,
+    // reload again to be sure.
+    await SiteDataManager.updateSites();
+
+    let baseDomain = SiteDataManager.getBaseDomainFromHost(host);
+    let siteData = await SiteDataManager.getSites(baseDomain);
+
+    // Hide the popup before showing the removal prompt, to
+    // avoid a pretty ugly transition. Also hide it even
+    // if the update resulted in no site data, to keep the
+    // illusion that clicking the button had an effect.
+    PanelMultiView.hidePopup(this._identityPopup);
+
+    if (siteData && siteData.length) {
+      let hosts = siteData.map(site => site.host);
+      if (SiteDataManager.promptSiteDataRemoval(window, hosts)) {
+        SiteDataManager.remove(hosts);
+      }
+    }
+
+    event.stopPropagation();
+  },
+
   /**
    * Handler for mouseclicks on the "More Information" button in the
    * "identity-popup" panel.
@@ -578,6 +615,21 @@ var gIdentityHandler = {
    * applicable
    */
   refreshIdentityPopup() {
+    // Update cookies and site data information and show the
+    // "Clear Site Data" button if the site is storing local data.
+    this._clearSiteDataFooter.hidden = true;
+    if (this._uriHasHost) {
+      let host = this._uri.host;
+      SiteDataManager.updateSites().then(async () => {
+        let baseDomain = SiteDataManager.getBaseDomainFromHost(host);
+        let siteData = await SiteDataManager.getSites(baseDomain);
+
+        if (siteData && siteData.length) {
+          this._clearSiteDataFooter.hidden = false;
+        }
+      });
+    }
+
     // Update "Learn More" for Mixed Content Blocking and Insecure Login Forms.
     let baseURL = Services.urlFormatter.formatURLPref("app.support.baseURL");
     this._identityPopupMixedContentLearnMore

browser/base/content/browser.js

@@ -55,6 +55,7 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   SchedulePressure: "resource:///modules/SchedulePressure.jsm",
   ShortcutUtils: "resource://gre/modules/ShortcutUtils.jsm",
   SimpleServiceDiscovery: "resource://gre/modules/SimpleServiceDiscovery.jsm",
+  SiteDataManager: "resource:///modules/SiteDataManager.jsm",
   SitePermissions: "resource:///modules/SitePermissions.jsm",
   TabCrashHandler: "resource:///modules/ContentCrashHandlers.jsm",
   TelemetryStopwatch: "resource://gre/modules/TelemetryStopwatch.jsm",

browser/base/content/test/siteIdentity/browser.ini

@@ -46,6 +46,7 @@ support-files =
 [browser_identity_UI.js]
 [browser_identityBlock_focus.js]
 support-files = ../permissions/permissions.html
+[browser_identityPopup_clearSiteData.js]
 [browser_identityPopup_focus.js]
 [browser_insecureLoginForms.js]
 support-files =

browser/base/content/test/siteIdentity/browser_identityPopup_clearSiteData.js

@@ -0,0 +1,110 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const TEST_ORIGIN = "https://example.com";
+const TEST_SUB_ORIGIN = "https://test1.example.com";
+const REMOVE_DIALOG_URL = "chrome://browser/content/preferences/siteDataRemoveSelected.xul";
+
+ChromeUtils.defineModuleGetter(this, "SiteDataTestUtils",
+                               "resource://testing-common/SiteDataTestUtils.jsm");
+
+async function testClearing(testQuota, testCookies) {
+  // Add some test quota storage.
+  if (testQuota) {
+    await SiteDataTestUtils.addToIndexedDB(TEST_ORIGIN);
+    await SiteDataTestUtils.addToIndexedDB(TEST_SUB_ORIGIN);
+  }
+
+  // Add some test cookies.
+  if (testCookies) {
+    SiteDataTestUtils.addToCookies(TEST_ORIGIN, "test1", "1");
+    SiteDataTestUtils.addToCookies(TEST_ORIGIN, "test2", "2");
+    SiteDataTestUtils.addToCookies(TEST_SUB_ORIGIN, "test3", "1");
+  }
+
+  await BrowserTestUtils.withNewTab(TEST_ORIGIN, async function(browser) {
+    // Verify we have added quota storage.
+    if (testQuota) {
+      let usage = await SiteDataTestUtils.getQuotaUsage(TEST_ORIGIN);
+      Assert.greater(usage, 0, "Should have data for the base origin.");
+
+      usage = await SiteDataTestUtils.getQuotaUsage(TEST_SUB_ORIGIN);
+      Assert.greater(usage, 0, "Should have data for the sub origin.");
+    }
+
+    // Open the identity popup.
+    let { gIdentityHandler } = gBrowser.ownerGlobal;
+    let promisePanelOpen = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
+    let siteDataUpdated = TestUtils.topicObserved("sitedatamanager:sites-updated");
+    gIdentityHandler._identityBox.click();
+    await promisePanelOpen;
+    await siteDataUpdated;
+
+    let clearFooter = document.getElementById("identity-popup-clear-sitedata-footer");
+    let clearButton = document.getElementById("identity-popup-clear-sitedata-button");
+    ok(!clearFooter.hidden, "The clear data footer is not hidden.");
+
+    let cookiesCleared;
+    if (testCookies) {
+      cookiesCleared = Promise.all([
+        TestUtils.topicObserved("cookie-changed", (subj, data) => data == "deleted" && subj.name == "test1"),
+        TestUtils.topicObserved("cookie-changed", (subj, data) => data == "deleted" && subj.name == "test2"),
+        TestUtils.topicObserved("cookie-changed", (subj, data) => data == "deleted" && subj.name == "test3"),
+      ]);
+    }
+
+    // Click the "Clear data" button.
+    siteDataUpdated = TestUtils.topicObserved("sitedatamanager:sites-updated");
+    let hideEvent = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popuphidden");
+    let removeDialogPromise = BrowserTestUtils.promiseAlertDialogOpen("accept", REMOVE_DIALOG_URL);
+    clearButton.click();
+    await hideEvent;
+    await removeDialogPromise;
+
+    await siteDataUpdated;
+
+    // Check that cookies were deleted.
+    if (testCookies) {
+      await cookiesCleared;
+      let uri = Services.io.newURI(TEST_ORIGIN);
+      is(Services.cookies.countCookiesFromHost(uri.host), 0, "Cookies from the base domain should be cleared");
+      uri = Services.io.newURI(TEST_SUB_ORIGIN);
+      is(Services.cookies.countCookiesFromHost(uri.host), 0, "Cookies from the sub domain should be cleared");
+    }
+
+    // Check that quota storage was deleted.
+    if (testQuota) {
+      await TestUtils.waitForCondition(async () => {
+        let usage = await SiteDataTestUtils.getQuotaUsage(TEST_ORIGIN);
+        return usage == 0;
+      }, "Should have no data for the base origin.");
+
+      let usage = await SiteDataTestUtils.getQuotaUsage(TEST_SUB_ORIGIN);
+      is(usage, 0, "Should have no data for the sub origin.");
+    }
+
+    // Open the site identity panel again to check that the button isn't shown anymore.
+    promisePanelOpen = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
+    siteDataUpdated = TestUtils.topicObserved("sitedatamanager:sites-updated");
+    gIdentityHandler._identityBox.click();
+    await promisePanelOpen;
+    await siteDataUpdated;
+
+    ok(clearFooter.hidden, "The clear data footer is hidden after clearing data.");
+  });
+}
+
+// Test removing quota managed storage.
+add_task(async function test_ClearSiteData() {
+  await testClearing(true, false);
+});
+
+// Test removing cookies.
+add_task(async function test_ClearCookies() {
+  await testClearing(false, true);
+});
+
+// Test removing both.
+add_task(async function test_ClearCookiesAndSiteData() {
+  await testClearing(true, true);
+});

browser/components/controlcenter/content/panel.inc.xul

@@ -95,6 +95,16 @@
           <description id="identity-popup-permission-empty-hint">&identity.permissionsEmpty;</description>
         </vbox>
       </hbox>
+
+      <!-- Clear Site Data Button -->
+      <vbox hidden="true"
+            id="identity-popup-clear-sitedata-footer"
+            class="identity-popup-footer">
+        <button class="subviewkeynav"
+                id="identity-popup-clear-sitedata-button"
+                label="&identity.clearSiteData;"
+                oncommand="gIdentityHandler.clearSiteData(event);"/>
+      </vbox>
     </panelview>
 
     <!-- Security SubView -->
@@ -178,7 +188,7 @@
                 oncommand="gIdentityHandler.enableMixedContentProtection()"/>
       </vbox>
 
-      <vbox id="identity-popup-securityView-footer">
+      <vbox id="identity-popup-more-info-footer" class="identity-popup-footer">
         <!-- More Security Information -->
         <button id="identity-popup-more-info"  class="subviewkeynav"
                 label="&identity.moreInfoLinkText2;"

browser/components/payments/content/paymentDialogWrapper.js

@@ -577,9 +577,20 @@ var paymentDialogWrapper = {
 
       // Select the new record
       if (selectedStateKey) {
+        if (selectedStateKey.length == 1) {
           Object.assign(successStateChange, {
-          [selectedStateKey]: guid,
+            [selectedStateKey[0]]: guid,
           });
+        } else if (selectedStateKey.length == 2) {
+          // Need to keep properties like preserveFieldValues from getting removed.
+          let subObj = Object.assign({}, successStateChange[selectedStateKey[0]]);
+          subObj[selectedStateKey[1]] = guid;
+          Object.assign(successStateChange, {
+            [selectedStateKey[0]]: subObj,
+          });
+        } else {
+          throw new Error(`selectedStateKey not supported: '${selectedStateKey}'`);
+        }
       }
 
       this.sendMessageToContent("updateState", successStateChange);

browser/components/payments/res/containers/address-form.js

@@ -86,6 +86,7 @@ export default class AddressForm extends PaymentStateSubscriberMixin(HTMLElement
     let record = {};
     let {
       page,
+      "address-page": addressPage,
     } = state;
 
     if (this.id && page && page.id !== this.id) {
@@ -101,23 +102,23 @@ export default class AddressForm extends PaymentStateSubscriberMixin(HTMLElement
     this.backButton.hidden = page.onboardingWizard;
     this.cancelButton.hidden = !page.onboardingWizard;
 
-    if (page.addressFields) {
-      this.setAttribute("address-fields", page.addressFields);
+    if (addressPage.addressFields) {
+      this.setAttribute("address-fields", addressPage.addressFields);
     } else {
       this.removeAttribute("address-fields");
     }
 
-    this.pageTitle.textContent = page.title;
+    this.pageTitle.textContent = addressPage.title;
     this.genericErrorText.textContent = page.error;
 
-    let editing = !!page.guid;
+    let editing = !!addressPage.guid;
     let addresses = paymentRequest.getAddresses(state);
 
     // If an address is selected we want to edit it.
     if (editing) {
-      record = addresses[page.guid];
+      record = addresses[addressPage.guid];
       if (!record) {
-        throw new Error("Trying to edit a non-existing address: " + page.guid);
+        throw new Error("Trying to edit a non-existing address: " + addressPage.guid);
       }
       // When editing an existing record, prevent changes to persistence
       this.persistCheckbox.hidden = true;
@@ -146,11 +147,19 @@ export default class AddressForm extends PaymentStateSubscriberMixin(HTMLElement
         break;
       }
       case this.backButton: {
-        this.requestStore.setState({
+        let currentState = this.requestStore.getState();
+        const previousId = currentState.page.previousId;
+        let state = {
           page: {
-            id: "payment-summary",
+            id: previousId || "payment-summary",
           },
+        };
+        if (previousId) {
+          state[previousId] = Object.assign({}, currentState[previousId], {
+            preserveFieldValues: true,
           });
+        }
+        this.requestStore.setState(state);
         break;
       }
       case this.saveButton: {
@@ -165,14 +174,16 @@ export default class AddressForm extends PaymentStateSubscriberMixin(HTMLElement
 
   saveRecord() {
     let record = this.formHandler.buildFormObject();
+    let currentState = this.requestStore.getState();
     let {
       page,
       tempAddresses,
       savedBasicCards,
-    } = this.requestStore.getState();
-    let editing = !!page.guid;
+      "address-page": addressPage,
+    } = currentState;
+    let editing = !!addressPage.guid;
 
-    if (editing ? (page.guid in tempAddresses) : !this.persistCheckbox.checked) {
+    if (editing ? (addressPage.guid in tempAddresses) : !this.persistCheckbox.checked) {
       record.isTemporary = true;
     }
 
@@ -183,28 +194,36 @@ export default class AddressForm extends PaymentStateSubscriberMixin(HTMLElement
           onboardingWizard: page.onboardingWizard,
           error: this.dataset.errorGenericSave,
         },
+        "address-page": addressPage,
       },
       preserveOldProperties: true,
       selectedStateKey: page.selectedStateKey,
     };
 
+    const previousId = page.previousId;
     if (page.onboardingWizard && !Object.keys(savedBasicCards).length) {
       state.successStateChange = {
         page: {
           id: "basic-card-page",
-          onboardingWizard: true,
-          guid: null,
+          previousId: "address-page",
+          onboardingWizard: page.onboardingWizard,
         },
       };
     } else {
       state.successStateChange = {
         page: {
-          id: "payment-summary",
+          id: previousId || "payment-summary",
+          onboardingWizard: page.onboardingWizard,
         },
       };
     }
 
-    paymentRequest.updateAutofillRecord("addresses", record, page.guid, state);
+    if (previousId) {
+      state.successStateChange[previousId] = Object.assign({}, currentState[previousId]);
+      state.successStateChange[previousId].preserveFieldValues = true;
+    }
+
+    paymentRequest.updateAutofillRecord("addresses", record, addressPage.guid, state);
   }
 }
 

browser/components/payments/res/containers/address-picker.js

@@ -168,22 +168,24 @@ export default class AddressPicker extends PaymentStateSubscriberMixin(HTMLEleme
     let nextState = {
       page: {
         id: "address-page",
-        selectedStateKey: this.selectedStateKey,
+      },
+      "address-page": {
         addressFields: this.getAttribute("address-fields"),
+        selectedStateKey: this.selectedStateKey,
       },
     };
 
     switch (target) {
       case this.addLink: {
-        nextState.page.guid = null;
-        nextState.page.title = this.dataset.addAddressTitle;
+        nextState["address-page"].guid = null;
+        nextState["address-page"].title = this.dataset.addAddressTitle;
         break;
       }
       case this.editLink: {
         let state = this.requestStore.getState();
         let selectedAddressGUID = state[this.selectedStateKey];
-        nextState.page.guid = selectedAddressGUID;
-        nextState.page.title = this.dataset.editAddressTitle;
+        nextState["address-page"].guid = selectedAddressGUID;
+        nextState["address-page"].title = this.dataset.editAddressTitle;
         break;
       }
       default: {

browser/components/payments/res/containers/basic-card-form.js

@@ -27,6 +27,15 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
     this.cancelButton.className = "cancel-button";
     this.cancelButton.addEventListener("click", this);
 
+    this.addressAddLink = document.createElement("a");
+    this.addressAddLink.className = "add-link";
+    this.addressAddLink.href = "javascript:void(0)";
+    this.addressAddLink.addEventListener("click", this);
+    this.addressEditLink = document.createElement("a");
+    this.addressEditLink.className = "edit-link";
+    this.addressEditLink.href = "javascript:void(0)";
+    this.addressEditLink.addEventListener("click", this);
+
     this.backButton = document.createElement("button");
     this.backButton.className = "back-button";
     this.backButton.addEventListener("click", this);
@@ -72,6 +81,13 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
         getAddressLabel: PaymentDialogUtils.getAddressLabel,
       });
 
+      let fragment = document.createDocumentFragment();
+      fragment.append(this.addressAddLink);
+      fragment.append(" ");
+      fragment.append(this.addressEditLink);
+      let billingAddressRow = this.form.querySelector(".billingAddressRow");
+      billingAddressRow.appendChild(fragment);
+
       this.appendChild(this.persistCheckbox);
       this.appendChild(this.genericErrorText);
       this.appendChild(this.cancelButton);
@@ -87,6 +103,7 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
     let {
       page,
       selectedShippingAddress,
+      "basic-card-page": basicCardPage,
     } = state;
 
     if (this.id && page && page.id !== this.id) {
@@ -98,6 +115,8 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
     this.backButton.textContent = this.dataset.backButtonLabel;
     this.saveButton.textContent = this.dataset.saveButtonLabel;
     this.persistCheckbox.label = this.dataset.persistCheckboxLabel;
+    this.addressAddLink.textContent = this.dataset.addressAddLinkLabel;
+    this.addressEditLink.textContent = this.dataset.addressEditLinkLabel;
 
     // The back button is temporarily hidden(See Bug 1462461).
     this.backButton.hidden = !!page.onboardingWizard;
@@ -109,22 +128,22 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
 
     this.genericErrorText.textContent = page.error;
 
-    let editing = !!page.guid;
+    let editing = !!basicCardPage.guid;
     this.form.querySelector("#cc-number").disabled = editing;
 
     // If a card is selected we want to edit it.
     if (editing) {
       this.pageTitle.textContent = this.dataset.editBasicCardTitle;
-      record = basicCards[page.guid];
+      record = basicCards[basicCardPage.guid];
       if (!record) {
-        throw new Error("Trying to edit a non-existing card: " + page.guid);
+        throw new Error("Trying to edit a non-existing card: " + basicCardPage.guid);
       }
       // When editing an existing record, prevent changes to persistence
       this.persistCheckbox.hidden = true;
     } else {
       this.pageTitle.textContent = this.dataset.addBasicCardTitle;
       // Use a currently selected shipping address as the default billing address
-      if (selectedShippingAddress) {
+      if (!record.billingAddressGUID && selectedShippingAddress) {
         record.billingAddressGUID = selectedShippingAddress;
       }
       // Adding a new record: default persistence to checked when in a not-private session
@@ -132,7 +151,15 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
       this.persistCheckbox.checked = !state.isPrivate;
     }
 
-    this.formHandler.loadRecord(record, addresses);
+    this.formHandler.loadRecord(record, addresses, basicCardPage.preserveFieldValues);
+
+    this.form.querySelector(".billingAddressRow").hidden = false;
+
+    if (basicCardPage.billingAddressGUID) {
+      let addressGuid = basicCardPage.billingAddressGUID;
+      let billingAddressSelect = this.form.querySelector("#billingAddressGUID");
+      billingAddressSelect.value = addressGuid;
+    }
   }
 
   handleEvent(event) {
@@ -150,6 +177,36 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
         paymentRequest.cancel();
         break;
       }
+      case this.addressAddLink:
+      case this.addressEditLink: {
+        let {
+          "basic-card-page": basicCardPage,
+        } = this.requestStore.getState();
+        let nextState = {
+          page: {
+            id: "address-page",
+            previousId: "basic-card-page",
+            selectedStateKey: ["basic-card-page", "billingAddressGUID"],
+          },
+          "address-page": {
+            guid: null,
+            title: this.dataset.billingAddressTitleAdd,
+          },
+          "basic-card-page": {
+            preserveFieldValues: true,
+            guid: basicCardPage.guid,
+          },
+        };
+        let billingAddressGUID = this.form.querySelector("#billingAddressGUID");
+        let selectedOption = billingAddressGUID.selectedOptions.length &&
+                             billingAddressGUID.selectedOptions[0];
+        if (evt.target == this.addressEditLink && selectedOption && selectedOption.value) {
+          nextState["address-page"].title = this.dataset.billingAddressTitleEdit;
+          nextState["address-page"].guid = selectedOption.value;
+        }
+        this.requestStore.setState(nextState);
+        break;
+      }
       case this.backButton: {
         this.requestStore.setState({
           page: {
@@ -170,13 +227,15 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
 
   saveRecord() {
     let record = this.formHandler.buildFormObject();
+    let currentState = this.requestStore.getState();
     let {
       page,
       tempBasicCards,
-    } = this.requestStore.getState();
-    let editing = !!page.guid;
+      "basic-card-page": basicCardPage,
+    } = currentState;
+    let editing = !!basicCardPage.guid;
 
-    if (editing ? (page.guid in tempBasicCards) : !this.persistCheckbox.checked) {
+    if (editing ? (basicCardPage.guid in tempBasicCards) : !this.persistCheckbox.checked) {
       record.isTemporary = true;
     }
 
@@ -190,7 +249,7 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
       record["cc-number"] = record["cc-number"] || "";
     }
 
-    paymentRequest.updateAutofillRecord("creditCards", record, page.guid, {
+    let state = {
       errorStateChange: {
         page: {
           id: "basic-card-page",
@@ -198,13 +257,20 @@ export default class BasicCardForm extends PaymentStateSubscriberMixin(HTMLEleme
         },
       },
       preserveOldProperties: true,
-      selectedStateKey: "selectedPaymentCard",
+      selectedStateKey: ["selectedPaymentCard"],
       successStateChange: {
         page: {
           id: "payment-summary",
         },
       },
-    });
+    };
+
+    const previousId = page.previousId;
+    if (previousId) {
+      state.successStateChange[previousId] = Object.assign({}, currentState[previousId]);
+    }
+
+    paymentRequest.updateAutofillRecord("creditCards", record, basicCardPage.guid, state);
   }
 }
 

browser/components/payments/res/containers/payment-method-picker.js

@@ -133,17 +133,18 @@ export default class PaymentMethodPicker extends PaymentStateSubscriberMixin(HTM
       page: {
         id: "basic-card-page",
       },
+      "basic-card-page": {},
     };
 
     switch (target) {
       case this.addLink: {
-        nextState.page.guid = null;
+        nextState["basic-card-page"].guid = null;
         break;
       }
       case this.editLink: {
         let state = this.requestStore.getState();
         let selectedPaymentCardGUID = state[this.selectedStateKey];
-        nextState.page.guid = selectedPaymentCardGUID;
+        nextState["basic-card-page"].guid = selectedPaymentCardGUID;
         break;
       }
       default: {

browser/components/payments/res/mixins/PaymentStateSubscriberMixin.js

@@ -15,10 +15,21 @@ export let requestStore = new PaymentsStore({
   changesPrevented: false,
   completionState: "initial",
   orderDetailsShowing: false,
+  "basic-card-page": {
+    guid: null,
+    // preserveFieldValues: true,
+  },
+  "address-page": {
+    guid: null,
+    title: "",
+  },
+  "payment-summary": {
+  },
   page: {
     id: "payment-summary",
+    previousId: null,
     // onboardingWizard: true,
-    // error: "My error",
+    // error: "",
   },
   request: {
     tabId: null,

browser/components/payments/res/paymentRequest.js

@@ -134,7 +134,6 @@ var paymentRequest = {
       state.page = {
         id: "basic-card-page",
         onboardingWizard: true,
-        guid: null,
       };
     }
 

browser/components/payments/res/paymentRequest.xhtml

@@ -25,6 +25,8 @@
   <!ENTITY deliveryAddress.editPage.title "Edit Delivery Address">
   <!ENTITY pickupAddress.addPage.title    "Add Pickup Address">
   <!ENTITY pickupAddress.editPage.title   "Edit Pickup Address">
+  <!ENTITY billingAddress.addPage.title   "Add Billing Address">
+  <!ENTITY billingAddress.editPage.title  "Edit Billing Address">
   <!ENTITY basicCard.addPage.title    "Add Credit Card">
   <!ENTITY basicCard.editPage.title   "Edit Credit Card">
   <!ENTITY payer.addPage.title        "Add Payer Contact">
@@ -39,6 +41,8 @@
   <!ENTITY orderDetailsLabel          "Order Details">
   <!ENTITY orderTotalLabel            "Total">
   <!ENTITY basicCardPage.error.genericSave    "There was an error saving the payment card.">
+  <!ENTITY basicCardPage.addressAddLink.label "Add">
+  <!ENTITY basicCardPage.addressEditLink.label "Edit">
   <!ENTITY basicCardPage.backButton.label     "Back">
   <!ENTITY basicCardPage.saveButton.label     "Save">
   <!ENTITY basicCardPage.persistCheckbox.label     "Save credit card to Firefox (Security code will not be saved)">
@@ -138,6 +142,10 @@
                        data-add-basic-card-title="&basicCard.addPage.title;"
                        data-edit-basic-card-title="&basicCard.editPage.title;"
                        data-error-generic-save="&basicCardPage.error.genericSave;"
+                       data-address-add-link-label="&basicCardPage.addressAddLink.label;"
+                       data-address-edit-link-label="&basicCardPage.addressEditLink.label;"
+                       data-billing-address-title-add="&billingAddress.addPage.title;"
+                       data-billing-address-title-edit="&billingAddress.editPage.title;"
                        data-back-button-label="&basicCardPage.backButton.label;"
                        data-save-button-label="&basicCardPage.saveButton.label;"
                        data-cancel-button-label="&cancelPaymentButton.label;"

browser/components/payments/test/browser/browser_address_edit.js

@@ -54,15 +54,15 @@ add_task(async function test_add_link() {
       addLink.click();
 
       state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-        return state.page.id == "address-page" && !state.page.guid;
+        return state.page.id == "address-page" && !state["address-page"].guid;
       }, "Check add page state");
 
       let title = content.document.querySelector("address-form h1");
       is(title.textContent, "Add Shipping Address", "Page title should be set");
 
-      let persistInput = content.document.querySelector("address-form labelled-checkbox");
-      ok(!persistInput.hidden, "checkbox should be visible when adding a new address");
-      ok(Cu.waiveXrays(persistInput).checked, "persist checkbox should be checked by default");
+      let persistCheckbox = content.document.querySelector("address-form labelled-checkbox");
+      ok(!persistCheckbox.hidden, "checkbox should be visible when adding a new address");
+      ok(Cu.waiveXrays(persistCheckbox).checked, "persist checkbox should be checked by default");
 
       info("filling fields");
       for (let [key, val] of Object.entries(address)) {
@@ -145,14 +145,14 @@ add_task(async function test_edit_link() {
       editLink.click();
 
       state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-        return state.page.id == "address-page" && !!state.page.guid;
+        return state.page.id == "address-page" && !!state["address-page"].guid;
       }, "Check edit page state");
 
       let title = content.document.querySelector("address-form h1");
       is(title.textContent, "Edit Shipping Address", "Page title should be set");
 
-      let persistInput = content.document.querySelector("address-form labelled-checkbox");
-      ok(persistInput.hidden, "checkbox should be hidden when editing an address");
+      let persistCheckbox = content.document.querySelector("address-form labelled-checkbox");
+      ok(persistCheckbox.hidden, "checkbox should be hidden when editing an address");
 
       info("overwriting field values");
       for (let [key, val] of Object.entries(address)) {
@@ -230,15 +230,15 @@ add_task(async function test_add_payer_contact_name_email_link() {
       addLink.click();
 
       state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-        return state.page.id == "address-page" && !state.page.guid;
+        return state.page.id == "address-page" && !state["address-page"].guid;
       }, "Check add page state");
 
       let title = content.document.querySelector("address-form h1");
       is(title.textContent, "Add Payer Contact", "Page title should be set");
 
-      let persistInput = content.document.querySelector("address-form labelled-checkbox");
-      ok(!persistInput.hidden, "checkbox should be visible when adding a new address");
-      ok(Cu.waiveXrays(persistInput).checked, "persist checkbox should be checked by default");
+      let persistCheckbox = content.document.querySelector("address-form labelled-checkbox");
+      ok(!persistCheckbox.hidden, "checkbox should be visible when adding a new address");
+      ok(Cu.waiveXrays(persistCheckbox).checked, "persist checkbox should be checked by default");
 
       info("filling fields");
       for (let [key, val] of Object.entries(address)) {
@@ -317,15 +317,14 @@ add_task(async function test_edit_payer_contact_name_email_phone_link() {
       editLink.click();
 
       state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-        info("state.page.id: " + state.page.id + "; state.page.guid: " + state.page.guid);
-        return state.page.id == "address-page" && !!state.page.guid;
+        return state.page.id == "address-page" && !!state["address-page"].guid;
       }, "Check edit page state");
 
       let title = content.document.querySelector("address-form h1");
       is(title.textContent, "Edit Payer Contact", "Page title should be set");
 
-      let persistInput = content.document.querySelector("address-form labelled-checkbox");
-      ok(persistInput.hidden, "checkbox should be hidden when editing an address");
+      let persistCheckbox = content.document.querySelector("address-form labelled-checkbox");
+      ok(persistCheckbox.hidden, "checkbox should be hidden when editing an address");
 
       info("overwriting field values");
       for (let [key, val] of Object.entries(address)) {
@@ -437,9 +436,10 @@ add_task(async function test_private_persist_addresses() {
         PaymentTestUtils: PTU,
       } = ChromeUtils.import("resource://testing-common/PaymentTestUtils.jsm", {});
 
-      let persistInput = content.document.querySelector("address-form labelled-checkbox");
-      ok(!persistInput.hidden, "checkbox should be visible when adding a new address");
-      ok(!Cu.waiveXrays(persistInput).checked, "persist checkbox should be unchecked by default");
+      let persistCheckbox = content.document.querySelector("address-form labelled-checkbox");
+      ok(!persistCheckbox.hidden, "checkbox should be visible when adding a new address");
+      ok(!Cu.waiveXrays(persistCheckbox).checked,
+         "persist checkbox should be unchecked by default");
 
       info("add the temp address");
       let addressToAdd = PTU.Addresses.Temp;

browser/components/payments/test/browser/browser_card_edit.js

@@ -18,16 +18,21 @@ add_task(async function test_add_link() {
     addLink.click();
 
     let state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-      return state.page.id == "basic-card-page" && !state.page.guid;
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid;
     }, "Check add page state");
 
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return Object.keys(state.savedBasicCards).length == 0 &&
+             Object.keys(state.savedAddresses).length == 0;
+    }, "Check no cards or addresses present at beginning of test");
+
     let title = content.document.querySelector("basic-card-form h1");
     is(title.textContent, "Add Credit Card", "Add title should be set");
 
     ok(!state.isPrivate,
        "isPrivate flag is not set when paymentrequest is shown from a non-private session");
-    let persistInput = content.document.querySelector("basic-card-form labelled-checkbox");
-    ok(Cu.waiveXrays(persistInput).checked, "persist checkbox should be checked by default");
+    let persistCheckbox = content.document.querySelector("basic-card-form labelled-checkbox");
+    ok(Cu.waiveXrays(persistCheckbox).checked, "persist checkbox should be checked by default");
 
     let year = (new Date()).getFullYear();
     let card = {
@@ -44,11 +49,88 @@ add_task(async function test_add_link() {
       ok(!field.disabled, `Field #${key} shouldn't be disabled`);
     }
 
+    let billingAddressSelect = content.document.querySelector("#billingAddressGUID");
+    isnot(billingAddressSelect.getBoundingClientRect().height, 0,
+          "The billing address selector should always be visible");
+    is(billingAddressSelect.childElementCount, 1,
+       "Only one child option should exist by default");
+    is(billingAddressSelect.children[0].value, "",
+       "The only option should be the blank/empty option");
+
+    let addressAddLink = content.document.querySelector(".billingAddressRow .add-link");
+    addressAddLink.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "address-page" && !state["address-page"].guid;
+    }, "Check address page state");
+
+    let addressTitle = content.document.querySelector("address-form h1");
+    is(addressTitle.textContent, "Add Billing Address",
+       "Address on add address page should be correct");
+
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return Object.keys(state.savedBasicCards).length == 0;
+    }, "Check card was not added when clicking the 'add' address button");
+
+    let addressBackButton = content.document.querySelector("address-form .back-button");
+    addressBackButton.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid &&
+             Object.keys(state.savedAddresses).length == 0;
+    }, "Check basic-card page, but card should not be saved and no addresses present");
+
+    is(title.textContent, "Add Credit Card", "Add title should be still be on credit card page");
+
+    for (let [key, val] of Object.entries(card)) {
+      let field = content.document.getElementById(key);
+      is(field.value, val, "Field should still have previous value entered");
+      ok(!field.disabled, "Fields should still be enabled for editing");
+    }
+
+    addressAddLink.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "address-page" && !state["address-page"].guid;
+    }, "Check address page state");
+
+    info("filling address fields");
+    for (let [key, val] of Object.entries(PTU.Addresses.TimBL)) {
+      let field = content.document.getElementById(key);
+      if (!field) {
+        ok(false, `${key} field not found`);
+      }
+      field.value = val;
+      ok(!field.disabled, `Field #${key} shouldn't be disabled`);
+    }
+
+    content.document.querySelector("address-form button:last-of-type").click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid &&
+             Object.keys(state.savedAddresses).length == 1;
+    }, "Check address was added and we're back on basic-card page (add)");
+
+    ok(state["basic-card-page"].preserveFieldValues,
+       "preserveFieldValues should be set when coming back from address-page");
+
+    ok(state["basic-card-page"].billingAddressGUID,
+       "billingAddressGUID should be set when coming back from address-page");
+
+    is(billingAddressSelect.childElementCount, 2,
+       "Two options should exist in the billingAddressSelect");
+    let selectedOption =
+      billingAddressSelect.children[billingAddressSelect.selectedIndex];
+    let selectedAddressGuid = selectedOption.value;
+    is(selectedAddressGuid, Object.values(state.savedAddresses)[0].guid,
+       "The select should have the new address selected");
+
+    for (let [key, val] of Object.entries(card)) {
+      let field = content.document.getElementById(key);
+      is(field.value, val, `Field #${key} should have value`);
+    }
+
     content.document.querySelector("basic-card-form button:last-of-type").click();
 
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
       return Object.keys(state.savedBasicCards).length == 1;
-    }, "Check card was added");
+    }, "Check card was not added again");
 
     let cardGUIDs = Object.keys(state.savedBasicCards);
     is(cardGUIDs.length, 1, "Check there is one card");
@@ -57,6 +139,8 @@ add_task(async function test_add_link() {
     for (let [key, val] of Object.entries(card)) {
       is(savedCard[key], val, "Check " + key);
     }
+    is(savedCard.billingAddressGUID, selectedAddressGuid,
+       "The saved card should be associated with the billing address");
 
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
       return state.page.id == "payment-summary";
@@ -80,9 +164,14 @@ add_task(async function test_edit_link() {
     editLink.click();
 
     let state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-      return state.page.id == "basic-card-page" && !!state.page.guid;
+      return state.page.id == "basic-card-page" && state["basic-card-page"].guid;
     }, "Check edit page state");
 
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return Object.keys(state.savedBasicCards).length == 1 &&
+             Object.keys(state.savedAddresses).length == 1;
+    }, "Check card and address present at beginning of test");
+
     let title = content.document.querySelector("basic-card-form h1");
     is(title.textContent, "Edit Credit Card", "Edit title should be set");
 
@@ -102,6 +191,90 @@ add_task(async function test_edit_link() {
     }
     ok(content.document.getElementById("cc-number").disabled, "cc-number field should be disabled");
 
+    let billingAddressSelect = content.document.querySelector("#billingAddressGUID");
+    is(billingAddressSelect.childElementCount, 2,
+       "Two options should exist in the billingAddressSelect");
+    is(billingAddressSelect.selectedIndex, 1,
+       "The billing address set by the previous test should be selected by default");
+
+    info("Test clicking 'edit' on the empty option first");
+    billingAddressSelect.selectedIndex = 0;
+
+    let addressEditLink = content.document.querySelector(".billingAddressRow .edit-link");
+    addressEditLink.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "address-page" && !state["address-page"].guid;
+    }, "Clicking edit button when the empty option is selected will go to 'add' page (no guid)");
+
+    let addressTitle = content.document.querySelector("address-form h1");
+    is(addressTitle.textContent, "Add Billing Address",
+       "Address on add address page should be correct");
+
+    let addressBackButton = content.document.querySelector("address-form .back-button");
+    addressBackButton.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "basic-card-page" && state["basic-card-page"].guid &&
+             Object.keys(state.savedAddresses).length == 1;
+    }, "Check we're back at basic-card page with no state changed after adding");
+
+    info("Go back to previously selected option before clicking 'edit' now");
+    billingAddressSelect.selectedIndex = 1;
+
+    let selectedOption = billingAddressSelect.selectedOptions.length &&
+                         billingAddressSelect.selectedOptions[0];
+    ok(selectedOption && selectedOption.value, "select should have a selected option value");
+
+    addressEditLink.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "address-page" && state["address-page"].guid;
+    }, "Check address page state (editing)");
+
+    is(addressTitle.textContent, "Edit Billing Address",
+       "Address on edit address page should be correct");
+
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return Object.keys(state.savedBasicCards).length == 1;
+    }, "Check card was not added again when clicking the 'edit' address button");
+
+    addressBackButton.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "basic-card-page" && state["basic-card-page"].guid &&
+             Object.keys(state.savedAddresses).length == 1;
+    }, "Check we're back at basic-card page with no state changed after editing");
+
+    for (let [key, val] of Object.entries(card)) {
+      let field = content.document.getElementById(key);
+      is(field.value, val, "Field should still have previous value entered");
+    }
+
+    selectedOption = billingAddressSelect.selectedOptions.length &&
+                     billingAddressSelect.selectedOptions[0];
+    ok(selectedOption && selectedOption.value, "select should have a selected option value");
+
+    addressEditLink.click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "address-page" && state["address-page"].guid;
+    }, "Check address page state (editing)");
+
+    info("filling address fields");
+    for (let [key, val] of Object.entries(PTU.Addresses.TimBL)) {
+      let field = content.document.getElementById(key);
+      if (!field) {
+        ok(false, `${key} field not found`);
+      }
+      field.value = val + "1";
+      ok(!field.disabled, `Field #${key} shouldn't be disabled`);
+    }
+
+    content.document.querySelector("address-form button:last-of-type").click();
+    state = await PTU.DialogContentUtils.waitForState(content, (state) => {
+      return state.page.id == "basic-card-page" && state["basic-card-page"].guid &&
+             Object.keys(state.savedAddresses).length == 1;
+    }, "Check still only one address and we're back on basic-card page");
+
+    is(Object.values(state.savedAddresses)[0].tel, PTU.Addresses.TimBL.tel + "1",
+       "Check that address was edited and saved");
+
     content.document.querySelector("basic-card-form button:last-of-type").click();
 
     state = await PTU.DialogContentUtils.waitForState(content, (state) => {
@@ -140,14 +313,14 @@ add_task(async function test_private_persist_defaults() {
     addLink.click();
 
     let state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-      return state.page.id == "basic-card-page" && !state.page.guid;
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid;
     },
                                                           "Check add page state");
 
     ok(!state.isPrivate,
        "isPrivate flag is not set when paymentrequest is shown from a non-private session");
-    let persistInput = content.document.querySelector("basic-card-form labelled-checkbox");
-    ok(Cu.waiveXrays(persistInput).checked,
+    let persistCheckbox = content.document.querySelector("basic-card-form labelled-checkbox");
+    ok(Cu.waiveXrays(persistCheckbox).checked,
        "checkbox is checked by default from a non-private session");
   }, args);
 
@@ -163,14 +336,14 @@ add_task(async function test_private_persist_defaults() {
     addLink.click();
 
     let state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-      return state.page.id == "basic-card-page" && !state.page.guid;
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid;
     },
                                                           "Check add page state");
 
     ok(state.isPrivate,
        "isPrivate flag is set when paymentrequest is shown from a private session");
-    let persistInput = content.document.querySelector("labelled-checkbox");
-    ok(!Cu.waiveXrays(persistInput).checked,
+    let persistCheckbox = content.document.querySelector("labelled-checkbox");
+    ok(!Cu.waiveXrays(persistCheckbox).checked,
        "checkbox is not checked by default from a private session");
   }, args, {
     browser: privateWin.gBrowser,
@@ -195,7 +368,7 @@ add_task(async function test_private_card_adding() {
     addLink.click();
 
     let state = await PTU.DialogContentUtils.waitForState(content, (state) => {
-      return state.page.id == "basic-card-page" && !state.page.guid;
+      return state.page.id == "basic-card-page" && !state["basic-card-page"].guid;
     },
                                                           "Check add page state");
 

browser/components/payments/test/mochitest/test_address_form.html

@@ -74,7 +74,9 @@ add_task(async function test_backButton() {
   form.dataset.backButtonLabel = "Back";
   await form.requestStore.setState({
     page: {
-      id: "test-page",
+      id: "address-page",
+    },
+    "address-page": {
       title: "Sample page title",
     },
   });
@@ -138,6 +140,9 @@ add_task(async function test_saveButton() {
         error: "Generic error",
         onboardingWizard: undefined,
       },
+      "address-page": {
+        title: "Sample page title",
+      },
     },
     guid: undefined,
     messageType: "updateAutofillRecord",
@@ -158,6 +163,7 @@ add_task(async function test_saveButton() {
     successStateChange: {
       page: {
         id: "payment-summary",
+        onboardingWizard: undefined,
       },
     },
   }, "Check event details for the message to chrome");
@@ -193,6 +199,8 @@ add_task(async function test_edit() {
   await form.requestStore.setState({
     page: {
       id: "address-page",
+    },
+    "address-page": {
       guid: address1.guid,
     },
     savedAddresses: {
@@ -210,6 +218,8 @@ add_task(async function test_edit() {
   await form.requestStore.setState({
     page: {
       id: "address-page",
+    },
+    "address-page": {
       guid: minimalAddress.guid,
     },
     savedAddresses: {
@@ -224,6 +234,7 @@ add_task(async function test_edit() {
     page: {
       id: "address-page",
     },
+    "address-page": {},
   });
   await asyncElementRendered();
   checkAddressForm(form, {});

browser/components/payments/test/mochitest/test_basic_card_form.html

@@ -69,7 +69,9 @@ add_task(async function test_backButton() {
   form.dataset.addBasicCardTitle = "Sample page title 2";
   await form.requestStore.setState({
     page: {
-      id: "test-page",
+      id: "basic-card-page",
+    },
+    "basic-card-page": {
     },
   });
   await form.promiseReady;
@@ -128,7 +130,7 @@ add_task(async function test_saveButton() {
       "cc-name": "J. Smith",
       "cc-number": "4111111111111111",
     },
-    selectedStateKey: "selectedPaymentCard",
+    selectedStateKey: ["selectedPaymentCard"],
     successStateChange: {
       page: {
         id: "payment-summary",
@@ -244,6 +246,8 @@ add_task(async function test_edit() {
   await form.requestStore.setState({
     page: {
       id: "basic-card-page",
+    },
+    "basic-card-page": {
       guid: card1.guid,
     },
     savedBasicCards: {
@@ -273,6 +277,8 @@ add_task(async function test_edit() {
   await form.requestStore.setState({
     page: {
       id: "basic-card-page",
+    },
+    "basic-card-page": {
       guid: minimalCard.guid,
     },
     savedBasicCards: {
@@ -287,6 +293,9 @@ add_task(async function test_edit() {
     page: {
       id: "basic-card-page",
     },
+    "basic-card-page": {
+      guid: null,
+    },
   });
   await asyncElementRendered();
   checkCCForm(form, {});

browser/extensions/formautofill/content/autofillEditForms.js

@@ -197,14 +197,17 @@ class EditCreditCard extends EditAutofillForm {
     this.attachEventListeners();
   }
 
-  loadRecord(record, addresses) {
+  loadRecord(record, addresses, preserveFieldValues) {
     // _record must be updated before generateYears and generateBillingAddressOptions are called.
     this._record = record;
     this._addresses = addresses;
-    this.generateYears();
     this.generateBillingAddressOptions();
+    if (!preserveFieldValues) {
+      // Re-generating the years will reset the selected option.
+      this.generateYears();
       super.loadRecord(record);
     }
+  }
 
   generateYears() {
     const count = 11;

browser/locales/en-US/chrome/browser/browser.dtd

@@ -792,6 +792,8 @@ you can use these alternative items. Otherwise, their values should be empty.  -
 
 <!ENTITY identity.moreInfoLinkText2 "More Information">
 
+<!ENTITY identity.clearSiteData "Clear Cookies and Site Data">
+
 <!ENTITY identity.permissions "Permissions">
 <!ENTITY identity.permissionsEmpty "You have not granted this site any special permissions.">
 <!ENTITY identity.permissionsReloadHint "You may need to reload the page for changes to apply.">

browser/themes/shared/controlcenter/panel.inc.css

@@ -239,12 +239,15 @@
   padding-inline-end: 1em;
 }
 
-#identity-popup-securityView-footer {
+#identity-popup-more-info-footer {
   margin-top: 1em;
+}
+
+.identity-popup-footer {
   background-color: var(--arrowpanel-dimmed);
 }
 
-#identity-popup-securityView-footer > button {
+.identity-popup-footer > button {
   -moz-appearance: none;
   margin: 0;
   border: none;
@@ -254,12 +257,12 @@
   background-color: transparent;
 }
 
-#identity-popup-securityView-footer > button:hover,
-#identity-popup-securityView-footer > button:focus {
+.identity-popup-footer > button:hover,
+.identity-popup-footer > button:focus {
   background-color: var(--arrowpanel-dimmed);
 }
 
-#identity-popup-securityView-footer > button:hover:active {
+.identity-popup-footer > button:hover:active {
   background-color: var(--arrowpanel-dimmed-further);
 }
 

devtools/client/shared/test/unit/test_parseDeclarations.js

@@ -71,6 +71,27 @@ const TEST_DATA = [
       {name: "p2", value: "v2", priority: "important", offsets: [21, 40]}
     ]
   },
+  // Test simple priority
+  {
+    input: "p1: v1 !/*comment*/important;",
+    expected: [
+      {name: "p1", value: "v1", priority: "important", offsets: [0, 29]},
+    ]
+  },
+  // Test priority without terminating ";".
+  {
+    input: "p1: v1 !important",
+    expected: [
+      {name: "p1", value: "v1", priority: "important", offsets: [0, 17]},
+    ]
+  },
+  // Test trailing "!" without terminating ";".
+  {
+    input: "p1: v1 !",
+    expected: [
+      {name: "p1", value: "v1 !", priority: "", offsets: [0, 8]},
+    ]
+  },
   // Test invalid priority
   {
     input: "p1: v1 important;",
@@ -78,6 +99,32 @@ const TEST_DATA = [
       {name: "p1", value: "v1 important", priority: "", offsets: [0, 17]}
     ]
   },
+  // Test invalid priority (in the middle of the declaration).
+  // See bug 1462553.
+  {
+    input: "p1: v1 !important v2;",
+    expected: [
+      {name: "p1", value: "v1 !important v2", priority: "", offsets: [0, 21]}
+    ]
+  },
+  {
+    input: "p1: v1 !    important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 25]}
+    ]
+  },
+  {
+    input: "p1: v1 !  /*comment*/  important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 36]}
+    ]
+  },
+  {
+    input: "p1: v1 !/*hi*/important v2;",
+    expected: [
+      {name: "p1", value: "v1 ! important v2", priority: "", offsets: [0, 27]}
+    ]
+  },
   // Test various types of background-image urls
   {
     input: "background-image: url(../../relative/image.png)",

devtools/client/shared/test/unit/test_parseSingleValue.js

@@ -22,7 +22,7 @@ const TEST_DATA = [
     input: "blue !  important",
     expected: {value: "blue", priority: "important"}
   },
-  {input: "blue !", expected: {value: "blue", priority: ""}},
+  {input: "blue !", expected: {value: "blue !", priority: ""}},
   {input: "blue !mportant", expected: {value: "blue !mportant", priority: ""}},
   {
     input: "  blue   !important ",

devtools/shared/css/parsing-utils.js

@@ -302,7 +302,16 @@ function parseDeclarationsInternal(isCssPropertyKnown, inputString,
   let declarations = [getEmptyDeclaration()];
   let lastProp = declarations[0];
 
-  let current = "", hasBang = false;
+  // This tracks the "!important" parsing state.  The states are:
+  // 0 - haven't seen anything
+  // 1 - have seen "!", looking for "important" next (possibly after
+  //     whitespace).
+  // 2 - have seen "!important"
+  let importantState = 0;
+  // This is true if we saw whitespace or comments between the "!" and
+  // the "important".
+  let importantWS = false;
+  let current = "";
   while (true) {
     let token = lexer.nextToken();
     if (!token) {
@@ -322,19 +331,23 @@ function parseDeclarationsInternal(isCssPropertyKnown, inputString,
         lastProp.offsets[0] = token.startOffset;
       }
       lastProp.offsets[1] = token.endOffset;
-    } else if (lastProp.name && !current && !hasBang &&
+    } else if (lastProp.name && !current && !importantState &&
                !lastProp.priority && lastProp.colonOffsets[1]) {
       // Whitespace appearing after the ":" is attributed to it.
       lastProp.colonOffsets[1] = token.endOffset;
+    } else if (importantState === 1) {
+      importantWS = true;
     }
 
     if (token.tokenType === "symbol" && token.text === ":") {
+      // Either way, a "!important" we've seen is no longer valid now.
+      importantState = 0;
+      importantWS = false;
       if (!lastProp.name) {
         // Set the current declaration name if there's no name yet
         lastProp.name = cssTrim(current);
         lastProp.colonOffsets = [token.startOffset, token.endOffset];
         current = "";
-        hasBang = false;
 
         // When parsing a comment body, if the left-hand-side is not a
         // valid property name, then drop it and stop parsing.
@@ -357,28 +370,44 @@ function parseDeclarationsInternal(isCssPropertyKnown, inputString,
         current = "";
         break;
       }
+      if (importantState === 2) {
+        lastProp.priority = "important";
+      } else if (importantState === 1) {
+        current += "!";
+        if (importantWS) {
+          current += " ";
+        }
+      }
       lastProp.value = cssTrim(current);
       current = "";
-      hasBang = false;
+      importantState = 0;
+      importantWS = false;
       declarations.push(getEmptyDeclaration());
       lastProp = declarations[declarations.length - 1];
     } else if (token.tokenType === "ident") {
-      if (token.text === "important" && hasBang) {
-        lastProp.priority = "important";
-        hasBang = false;
+      if (token.text === "important" && importantState === 1) {
+        importantState = 2;
       } else {
-        if (hasBang) {
+        if (importantState > 0) {
           current += "!";
+          if (importantWS) {
+            current += " ";
+          }
+          if (importantState === 2) {
+            current += "important ";
+          }
+          importantState = 0;
+          importantWS = false;
         }
         // Re-escape the token to avoid dequoting problems.
         // See bug 1287620.
         current += CSS.escape(token.text);
       }
     } else if (token.tokenType === "symbol" && token.text === "!") {
-      hasBang = true;
+      importantState = 1;
     } else if (token.tokenType === "whitespace") {
       if (current !== "") {
-        current += " ";
+        current = current.trimRight() + " ";
       }
     } else if (token.tokenType === "comment") {
       if (parseComments && !lastProp.name && !lastProp.value) {
@@ -392,9 +421,20 @@ function parseDeclarationsInternal(isCssPropertyKnown, inputString,
         let lastDecl = declarations.pop();
         declarations = [...declarations, ...newDecls, lastDecl];
       } else {
-        current += " ";
+        current = current.trimRight() + " ";
       }
     } else {
+      if (importantState > 0) {
+        current += "!";
+        if (importantWS) {
+          current += " ";
+        }
+        if (importantState === 2) {
+          current += "important ";
+        }
+        importantState = 0;
+        importantWS = false;
+      }
       current += inputString.substring(token.startOffset, token.endOffset);
     }
   }
@@ -409,6 +449,11 @@ function parseDeclarationsInternal(isCssPropertyKnown, inputString,
       }
     } else {
       // Trailing value found, i.e. value without an ending ;
+      if (importantState === 2) {
+        lastProp.priority = "important";
+      } else if (importantState === 1) {
+        current += "!";
+      }
       lastProp.value = cssTrim(current);
       let terminator = lexer.performEOFFixup("", true);
       lastProp.terminator = terminator + ";";

editor/libeditor/EditorBase.cpp

@@ -489,11 +489,12 @@ EditorBase::GetDesiredSpellCheckState()
   return element->Spellcheck();
 }
 
-NS_IMETHODIMP
+void
 EditorBase::PreDestroy(bool aDestroyingFrames)
 {
-  if (mDidPreDestroy)
-    return NS_OK;
+  if (mDidPreDestroy) {
+    return;
+  }
 
   Selection* selection = GetSelection();
   if (selection) {
@@ -537,7 +538,6 @@ EditorBase::PreDestroy(bool aDestroyingFrames)
   }
 
   mDidPreDestroy = true;
-  return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -1189,13 +1189,6 @@ EditorBase::CanPaste(int32_t aSelectionType, bool* aCanPaste)
   return NS_ERROR_NOT_IMPLEMENTED;
 }
 
-NS_IMETHODIMP
-EditorBase::CanPasteTransferable(nsITransferable* aTransferable,
-                                 bool* aCanPaste)
-{
-  return NS_ERROR_NOT_IMPLEMENTED;
-}
-
 NS_IMETHODIMP
 EditorBase::SetAttribute(Element* aElement,
                          const nsAString& aAttribute,

editor/libeditor/EditorBase.h

@@ -969,6 +969,15 @@ public:
    */
   nsresult PostCreate();
 
+ /**
+   * PreDestroy is called before the editor goes away, and gives the editor a
+   * chance to tell its documentStateObservers that the document is going away.
+   * @param aDestroyingFrames set to true when the frames being edited
+   * are being destroyed (so there is no need to modify any nsISelections,
+   * nor is it safe to do so)
+   */
+  virtual void PreDestroy(bool aDestroyingFrames);
+
   /**
    * All editor operations which alter the doc should be prefaced
    * with a call to StartOperation, naming the action and direction.

editor/libeditor/EditorCommands.cpp

@@ -606,7 +606,8 @@ PasteTransferableCommand::IsCommandEnabled(const char* aCommandName,
   if (!textEditor->IsSelectionEditable()) {
     return NS_OK;
   }
-  return textEditor->CanPasteTransferable(nullptr, aIsEnabled);
+  *aIsEnabled = textEditor->CanPasteTransferable(nullptr);
+  return NS_OK;
 }
 
 NS_IMETHODIMP
@@ -667,13 +668,8 @@ PasteTransferableCommand::GetCommandStateParams(const char* aCommandName,
   TextEditor* textEditor = editor->AsTextEditor();
   MOZ_ASSERT(textEditor);
 
-  bool canPaste;
-  nsresult rv = textEditor->CanPasteTransferable(trans, &canPaste);
-  if (NS_WARN_IF(NS_FAILED(rv))) {
-    return rv;
-  }
-
-  return aParams->SetBooleanValue(STATE_ENABLED, canPaste);
+  return aParams->SetBooleanValue(STATE_ENABLED,
+                                  textEditor->CanPasteTransferable(trans));
 }
 
 /******************************************************************************

editor/libeditor/HTMLEditor.cpp

@@ -332,11 +332,11 @@ HTMLEditor::Init(nsIDocument& aDoc,
   return NS_OK;
 }
 
-NS_IMETHODIMP
+void
 HTMLEditor::PreDestroy(bool aDestroyingFrames)
 {
   if (mDidPreDestroy) {
-    return NS_OK;
+    return;
   }
 
   nsCOMPtr<nsIDocument> document = GetDocument();
@@ -352,7 +352,7 @@ HTMLEditor::PreDestroy(bool aDestroyingFrames)
   // stay around (which they would, since the frames have an owning reference).
   HideAnonymousEditingUIs();
 
-  return TextEditor::PreDestroy(aDestroyingFrames);
+  EditorBase::PreDestroy(aDestroyingFrames);
 }
 
 NS_IMETHODIMP

editor/libeditor/HTMLEditor.h

@@ -133,6 +133,7 @@ public:
                                             bool aSuppressTransaction) override;
   using EditorBase::RemoveAttributeOrEquivalent;
   using EditorBase::SetAttributeOrEquivalent;
+  virtual bool CanPasteTransferable(nsITransferable* aTransferable) override;
 
   // nsStubMutationObserver overrides
   NS_DECL_NSIMUTATIONOBSERVER_CONTENTAPPENDED
@@ -212,7 +213,7 @@ public:
   // Overrides of EditorBase interface methods
   virtual nsresult EndUpdateViewBatch() override;
 
-  NS_IMETHOD PreDestroy(bool aDestroyingFrames) override;
+  virtual void PreDestroy(bool aDestroyingFrames) override;
 
   virtual nsresult GetPreferredIMEState(widget::IMEState* aState) override;
 
@@ -329,8 +330,6 @@ public:
   NS_IMETHOD CanPaste(int32_t aSelectionType, bool* aCanPaste) override;
 
   NS_IMETHOD PasteTransferable(nsITransferable* aTransferable) override;
-  NS_IMETHOD CanPasteTransferable(nsITransferable* aTransferable,
-                                  bool* aCanPaste) override;
 
   NS_IMETHOD DebugUnitTests(int32_t* outNumTests,
                             int32_t* outNumTestsFailed) override;

editor/libeditor/HTMLEditorDataTransfer.cpp

@@ -1429,7 +1429,7 @@ HTMLEditor::Paste(int32_t aSelectionType)
                                 bHavePrivateHTMLFlavor, true);
 }
 
-NS_IMETHODIMP
+nsresult
 HTMLEditor::PasteTransferable(nsITransferable* aTransferable)
 {
   // Use an invalid value for the clipboard type as data comes from aTransferable
@@ -1525,29 +1525,24 @@ HTMLEditor::CanPaste(int32_t aSelectionType,
   return NS_OK;
 }
 
-NS_IMETHODIMP
-HTMLEditor::CanPasteTransferable(nsITransferable* aTransferable,
-                                 bool* aCanPaste)
+bool
+HTMLEditor::CanPasteTransferable(nsITransferable* aTransferable)
 {
-  NS_ENSURE_ARG_POINTER(aCanPaste);
-
   // can't paste if readonly
   if (!IsModifiable()) {
-    *aCanPaste = false;
-    return NS_OK;
+    return false;
   }
 
   // If |aTransferable| is null, assume that a paste will succeed.
   if (!aTransferable) {
-    *aCanPaste = true;
-    return NS_OK;
+    return true;
   }
 
   // Peek in |aTransferable| to see if it contains a supported MIME type.
 
   // Use the flavors depending on the current editor mask
   const char ** flavors;
-  unsigned length;
+  size_t length;
   if (IsPlaintextEditor()) {
     flavors = textEditorFlavors;
     length = ArrayLength(textEditorFlavors);
@@ -1556,20 +1551,18 @@ HTMLEditor::CanPasteTransferable(nsITransferable* aTransferable,
     length = ArrayLength(textHtmlEditorFlavors);
   }
 
-  for (unsigned int i = 0; i < length; i++, flavors++) {
+  for (size_t i = 0; i < length; i++, flavors++) {
     nsCOMPtr<nsISupports> data;
     uint32_t dataLen;
     nsresult rv = aTransferable->GetTransferData(*flavors,
                                                  getter_AddRefs(data),
                                                  &dataLen);
     if (NS_SUCCEEDED(rv) && data) {
-      *aCanPaste = true;
-      return NS_OK;
+      return true;
     }
   }
 
-  *aCanPaste = false;
-  return NS_OK;
+  return false;
 }
 
 /**

editor/libeditor/TextEditor.cpp

@@ -1737,13 +1737,10 @@ TextEditor::OutputToString(const nsAString& aFormatType,
   // Protect the edit rules object from dying
   RefPtr<TextEditRules> rules(mRules);
 
-  nsString resultString;
   RulesInfo ruleInfo(EditAction::outputText);
-  ruleInfo.outString = &resultString;
+  ruleInfo.outString = &aOutputString;
   ruleInfo.flags = aFlags;
-  // XXX Struct should store a nsAReadable*
-  nsAutoString str(aFormatType);
-  ruleInfo.outputFormat = &str;
+  ruleInfo.outputFormat = &aFormatType;
   Selection* selection = GetSelection();
   if (NS_WARN_IF(!selection)) {
     return NS_ERROR_FAILURE;
@@ -1754,8 +1751,7 @@ TextEditor::OutputToString(const nsAString& aFormatType,
     return rv;
   }
   if (handled) {
-    // This case will get triggered by password fields.
-    aOutputString.Assign(*(ruleInfo.outString));
+    // This case will get triggered by password fields or single text node only.
     return rv;
   }
 

editor/libeditor/TextEditor.h

@@ -73,13 +73,18 @@ public:
   NS_IMETHOD Paste(int32_t aSelectionType) override;
   NS_IMETHOD CanPaste(int32_t aSelectionType, bool* aCanPaste) override;
   NS_IMETHOD PasteTransferable(nsITransferable* aTransferable) override;
-  NS_IMETHOD CanPasteTransferable(nsITransferable* aTransferable,
-                                  bool* aCanPaste) override;
 
   NS_IMETHOD OutputToString(const nsAString& aFormatType,
                             uint32_t aFlags,
                             nsAString& aOutputString) override;
 
+  /** Can we paste |aTransferable| or, if |aTransferable| is null, will a call
+    * to pasteTransferable later possibly succeed if given an instance of
+    * nsITransferable then? True if the doc is modifiable, and, if
+    * |aTransfeable| is non-null, we have pasteable data in |aTransfeable|.
+    */
+  virtual bool CanPasteTransferable(nsITransferable* aTransferable);
+
   // Overrides of EditorBase
   virtual nsresult RemoveAttributeOrEquivalent(
                      Element* aElement,

editor/libeditor/TextEditorDataTransfer.cpp

@@ -374,23 +374,17 @@ TextEditor::CanPaste(int32_t aSelectionType,
   return NS_OK;
 }
 
-
-NS_IMETHODIMP
-TextEditor::CanPasteTransferable(nsITransferable* aTransferable,
-                                 bool* aCanPaste)
+bool
+TextEditor::CanPasteTransferable(nsITransferable* aTransferable)
 {
-  NS_ENSURE_ARG_POINTER(aCanPaste);
-
   // can't paste if readonly
   if (!IsModifiable()) {
-    *aCanPaste = false;
-    return NS_OK;
+    return false;
   }
 
   // If |aTransferable| is null, assume that a paste will succeed.
   if (!aTransferable) {
-    *aCanPaste = true;
-    return NS_OK;
+    return true;
   }
 
   nsCOMPtr<nsISupports> data;
@@ -399,12 +393,10 @@ TextEditor::CanPasteTransferable(nsITransferable* aTransferable,
                                                getter_AddRefs(data),
                                                &dataLen);
   if (NS_SUCCEEDED(rv) && data) {
-    *aCanPaste = true;
-  } else {
-    *aCanPaste = false;
+    return true;
   }
 
-  return NS_OK;
+  return false;
 }
 
 bool

editor/nsIEditor.idl

@@ -59,15 +59,6 @@ interface nsIEditor  : nsISupports
                                    in DOMString sourceAttrName,
                                    in boolean aSuppressTransaction);
 
-  /**
-   * preDestroy is called before the editor goes away, and gives the editor a
-   * chance to tell its documentStateObservers that the document is going away.
-   * @param aDestroyingFrames set to true when the frames being edited
-   * are being destroyed (so there is no need to modify any nsISelections,
-   * nor is it safe to do so)
-   */
-  void preDestroy(in boolean aDestroyingFrames);
-
   /** edit flags for this editor.  May be set at any time. */
   attribute unsigned long flags;
 
@@ -299,13 +290,6 @@ interface nsIEditor  : nsISupports
     */
   boolean canPaste(in long aSelectionType);
 
-  /** Can we paste |aTransferable| or, if |aTransferable| is null, will a call
-    * to pasteTransferable later possibly succeed if given an instance of
-    * nsITransferable then? True if the doc is modifiable, and, if
-    * |aTransfeable| is non-null, we have pasteable data in |aTransfeable|.
-    */
-  boolean canPasteTransferable([optional] in nsITransferable aTransferable);
-
   /* ------------ Selection methods -------------- */
 
   /** sets the document selection to the entire contents of the document */

ipc/chromium/src/chrome/common/ipc_channel_posix.cc

@@ -423,10 +423,27 @@ bool Channel::ChannelImpl::ProcessIncomingMessages() {
       fds = wire_fds;
       num_fds = num_wire_fds;
     } else {
+      // This code may look like a no-op in the case where
+      // num_wire_fds == 0, but in fact:
+      //
+      // 1. wire_fds will be nullptr, so passing it to memcpy is
+      // undefined behavior according to the C standard, even though
+      // the memcpy length is 0.
+      //
+      // 2. prev_size will be an out-of-bounds index for
+      // input_overflow_fds_; this is undefined behavior according to
+      // the C++ standard, even though the element only has its
+      // pointer taken and isn't accessed (and the corresponding
+      // operation on a C array would be defined).
+      //
+      // UBSan makes #1 a fatal error, and assertions in libstdc++ do
+      // the same for #2 if enabled.
+      if (num_wire_fds > 0) {
         const size_t prev_size = input_overflow_fds_.size();
         input_overflow_fds_.resize(prev_size + num_wire_fds);
         memcpy(&input_overflow_fds_[prev_size], wire_fds,
                num_wire_fds * sizeof(int));
+      }
       fds = &input_overflow_fds_[0];
       num_fds = input_overflow_fds_.size();
     }

layout/base/nsLayoutUtils.cpp

@@ -258,8 +258,9 @@ MayHaveAnimationOfProperty(EffectSet* effects, nsCSSPropertyID aProperty)
   return true;
 }
 
-static bool
-MayHaveAnimationOfProperty(const nsIFrame* aFrame, nsCSSPropertyID aProperty)
+bool
+nsLayoutUtils::MayHaveAnimationOfProperty(const nsIFrame* aFrame,
+                                          nsCSSPropertyID aProperty)
 {
   switch (aProperty) {
     case eCSSProperty_transform:
@@ -276,7 +277,7 @@ bool
 nsLayoutUtils::HasAnimationOfProperty(EffectSet* aEffectSet,
                                       nsCSSPropertyID aProperty)
 {
-  if (!aEffectSet || !MayHaveAnimationOfProperty(aEffectSet, aProperty)) {
+  if (!aEffectSet || !::MayHaveAnimationOfProperty(aEffectSet, aProperty)) {
     return false;
   }
 
@@ -312,7 +313,7 @@ nsLayoutUtils::HasEffectiveAnimation(const nsIFrame* aFrame,
                                      nsCSSPropertyID aProperty)
 {
   EffectSet* effects = EffectSet::GetEffectSet(aFrame);
-  if (!effects || !MayHaveAnimationOfProperty(effects, aProperty)) {
+  if (!effects || !::MayHaveAnimationOfProperty(effects, aProperty)) {
     return false;
   }
 
@@ -326,6 +327,17 @@ nsLayoutUtils::HasEffectiveAnimation(const nsIFrame* aFrame,
   );
 }
 
+bool
+nsLayoutUtils::MayHaveEffectiveAnimation(const nsIFrame* aFrame,
+                                         nsCSSPropertyID aProperty)
+{
+  EffectSet* effects = EffectSet::GetEffectSet(aFrame);
+  if (!effects || !::MayHaveAnimationOfProperty(effects, aProperty)) {
+    return false;
+  }
+  return true;
+}
+
 static float
 GetSuitableScale(float aMaxScale, float aMinScale,
                  nscoord aVisibleDimension, nscoord aDisplayDimension)

layout/base/nsLayoutUtils.h

@@ -2332,6 +2332,8 @@ public:
    */
   static bool HasAnimationOfProperty(const nsIFrame* aFrame,
                                      nsCSSPropertyID aProperty);
+  static bool MayHaveAnimationOfProperty(const nsIFrame* aFrame,
+                                         nsCSSPropertyID aProperty);
 
   /**
    * Returns true if |aEffectSet| has an animation of |aProperty| regardless of
@@ -2346,6 +2348,8 @@ public:
    */
   static bool HasEffectiveAnimation(const nsIFrame* aFrame,
                                     nsCSSPropertyID aProperty);
+  static bool MayHaveEffectiveAnimation(const nsIFrame* aFrame,
+                                        nsCSSPropertyID aProperty);
 
   /**
    * Checks if off-main-thread animations are enabled.

layout/generic/nsFrame.cpp

@@ -2954,9 +2954,12 @@ nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder* aBuilder,
   // layer (for async animations), see
   // nsSVGIntegrationsUtils::PaintMaskAndClipPath or
   // nsSVGIntegrationsUtils::PaintFilter.
+  // Use MayNeedActiveLayer to decide, since we don't want to condition the wrapping
+  // display item on values that might change silently between paints (opacity activity
+  // can depend on the will-change budget).
   bool useOpacity = HasVisualOpacity(effectSet) &&
                     !nsSVGUtils::CanOptimizeOpacity(this) &&
-                    (!usingSVGEffects || nsDisplayOpacity::NeedsActiveLayer(aBuilder, this));
+                    (!usingSVGEffects || nsDisplayOpacity::MayNeedActiveLayer(this));
   bool useBlendMode = effects->mMixBlendMode != NS_STYLE_BLEND_NORMAL;
   bool useStickyPosition = disp->mPosition == NS_STYLE_POSITION_STICKY &&
     IsScrollFrameActive(aBuilder,

layout/generic/nsSubDocumentFrame.cpp

@@ -462,6 +462,9 @@ nsSubDocumentFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
       if (mPreviousCaret) {
         aBuilder->MarkFrameModifiedDuringBuilding(mPreviousCaret);
       }
+      if (aBuilder->GetCaretFrame()) {
+        aBuilder->MarkFrameModifiedDuringBuilding(aBuilder->GetCaretFrame());
+      }
     }
     mPreviousCaret = aBuilder->GetCaretFrame();
   }

layout/painting/ActiveLayerTracker.cpp

@@ -436,7 +436,11 @@ ActiveLayerTracker::IsStyleAnimated(nsDisplayListBuilder* aBuilder,
   if (aProperty == eCSSProperty_transform && aFrame->Combines3DTransformWithAncestors()) {
     return IsStyleAnimated(aBuilder, aFrame->GetParent(), aProperty);
   }
+  if (aBuilder) {
     return nsLayoutUtils::HasEffectiveAnimation(aFrame, aProperty);
+  } else {
+    return nsLayoutUtils::MayHaveEffectiveAnimation(aFrame, aProperty);
+  }
 }
 
 /* static */ bool

layout/painting/RetainedDisplayListBuilder.cpp

@@ -335,10 +335,11 @@ public:
     for (nsDisplayItem* i : *items) {
       if (i != aItem && i->Frame() == aItem->Frame() &&
           i->GetPerFrameKey() == aItem->GetPerFrameKey()) {
-        *aOutIndex = i->GetOldListIndex(mOldList, mOuterKey);
+        if (i->GetOldListIndex(mOldList, mOuterKey, aOutIndex)) {
           return true;
         }
       }
+    }
     return false;
   }
 

layout/painting/nsDisplayList.cpp

@@ -6526,6 +6526,12 @@ nsDisplayOpacity::NeedsActiveLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFr
   return false;
 }
 
+/* static */ bool
+nsDisplayOpacity::MayNeedActiveLayer(nsIFrame* aFrame)
+{
+  return ActiveLayerTracker::IsStyleMaybeAnimated(aFrame, eCSSProperty_opacity);
+}
+
 void
 nsDisplayOpacity::ApplyOpacity(nsDisplayListBuilder* aBuilder,
                              float aOpacity,

layout/painting/nsDisplayList.h

@@ -2863,14 +2863,16 @@ public:
 #endif
     mOldListIndex = aIndex;
   }
-  OldListIndex GetOldListIndex(nsDisplayList* aList, uint32_t aListKey)
+  bool GetOldListIndex(nsDisplayList* aList, uint32_t aListKey, OldListIndex* aOutIndex)
   {
-#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
     if (mOldList != reinterpret_cast<uintptr_t>(aList)) {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
       MOZ_CRASH_UNSAFE_PRINTF("Item found was in the wrong list! type %d (outer type was %d at depth %d, now is %d)", GetPerFrameKey(), mOldListKey, mOldNestingDepth, aListKey);
-    }
 #endif
-    return mOldListIndex;
+      return false;
+    }
+    *aOutIndex = mOldListIndex;
+    return true;
   }
 
   const nsRect& GetPaintRect() const {
@@ -2910,7 +2912,6 @@ protected:
 
 #ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
 public:
-  uintptr_t mOldList = 0;
   uint32_t mOldListKey = 0;
   uint32_t mOldNestingDepth = 0;
   bool mMergedItem = false;
@@ -2918,6 +2919,7 @@ public:
 protected:
 #endif
   OldListIndex mOldListIndex;
+  uintptr_t mOldList = 0;
 
   bool      mForceNotVisible;
   bool      mDisableSubpixelAA;
@@ -5384,6 +5386,7 @@ public:
   bool OpacityAppliedToChildren() const { return mOpacityAppliedToChildren; }
 
   static bool NeedsActiveLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame);
+  static bool MayNeedActiveLayer(nsIFrame* aFrame);
   NS_DISPLAY_DECL_NAME("Opacity", TYPE_OPACITY)
   virtual void WriteDebugInfo(std::stringstream& aStream) override;
 

layout/svg/nsSVGUtils.cpp

@@ -1295,7 +1295,7 @@ nsSVGUtils::CanOptimizeOpacity(nsIFrame *aFrame)
     return false;
   }
 
-  if (nsLayoutUtils::HasAnimationOfProperty(aFrame, eCSSProperty_opacity)) {
+  if (nsLayoutUtils::MayHaveAnimationOfProperty(aFrame, eCSSProperty_opacity)) {
     return false;
   }
 

netwerk/dns/nsHostResolver.cpp

@@ -1885,6 +1885,7 @@ nsHostResolver::Create(uint32_t maxCacheEntries,
 void
 nsHostResolver::GetDNSCacheEntries(nsTArray<DNSCacheEntries> *args)
 {
+    MutexAutoLock lock(mLock);
     for (auto iter = mRecordDB.Iter(); !iter.Done(); iter.Next()) {
         // We don't pay attention to address literals, only resolved domains.
         // Also require a host.

security/sandbox/linux/SandboxFilter.cpp

@@ -1053,6 +1053,14 @@ public:
     case __NR_mremap:
       return Allow();
 
+      // Bug 1462640: Mesa libEGL uses mincore to test whether values
+      // are pointers, for reasons.
+    case __NR_mincore: {
+      Arg<size_t> length(1);
+      return If(length == getpagesize(), Allow())
+             .Else(SandboxPolicyCommon::EvaluateSyscall(sysno));
+    }
+
     case __NR_sigaltstack:
       return Allow();
 

services/common/blocklist-clients.js

@@ -11,8 +11,8 @@ var EXPORTED_SYMBOLS = [
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { OS } = ChromeUtils.import("resource://gre/modules/osfile.jsm", {});
 
-ChromeUtils.defineModuleGetter(this, "RemoteSettings",
-                               "resource://services-common/remote-settings.js");
+ChromeUtils.defineModuleGetter(this, "RemoteSettings", "resource://services-common/remote-settings.js");
+ChromeUtils.defineModuleGetter(this, "jexlFilterFunc", "resource://services-common/remote-settings.js");
 
 const PREF_BLOCKLIST_BUCKET                  = "services.blocklist.bucket";
 const PREF_BLOCKLIST_ONECRL_COLLECTION       = "services.blocklist.onecrl.collection";
@@ -132,17 +132,22 @@ async function updateJSONBlocklist(client, { data: { current: records } }) {
  * This custom filter function is used to limit the entries returned
  * by `RemoteSettings("...").get()` depending on the target app information
  * defined on entries.
- *
- * When landing Bug 1451031, this function will have to check if the `entry`
- * has a JEXL attribute and rely on the JEXL filter function in priority.
- * The legacy target app mechanism will be kept in place for old entries.
  */
-async function targetAppFilter(entry, { appID, version: appVersion }) {
+async function targetAppFilter(entry, environment) {
+  // If the entry has JEXL filters, they should prevail.
+  // The legacy target app mechanism will be kept in place for old entries.
+  // See https://bugzilla.mozilla.org/show_bug.cgi?id=1463377
+  const { filters } = entry;
+  if (filters) {
+    return jexlFilterFunc(entry, environment);
+  }
+
   // Keep entries without target information.
   if (!("versionRange" in entry)) {
     return entry;
   }
 
+  const { appID, version: appVersion } = environment;
   const { versionRange } = entry;
 
   // Gfx blocklist has a specific versionRange object, which is not a list.

services/common/docs/RemoteSettings.rst

@@ -103,6 +103,18 @@ It is possible to package a dump of the server records that will be loaded into
 Now, when ``RemoteSettings("some-key").get()`` is called from an empty profile, the ``some-key.json`` file is going to be loaded before the results are returned.
 
 
+Targets and A/B testing
+=======================
+
+In order to deliver settings to subsets of the population, you can set targets on entries (platform, language, channel, version range, preferences values, samples, etc.) when editing records on the server.
+
+From the client API standpoint, this is completely transparent: the ``.get()`` method — as well as the event data — will always filter the entries on which the target matches.
+
+.. note::
+
+    The remote settings targets follow the same approach as the :ref:`Normandy recipe client <components/normandy>` (ie. JEXL filters),
+
+
 Uptake Telemetry
 ================
 

services/common/remote-settings.js

@@ -4,7 +4,10 @@
 
 "use strict";
 
-var EXPORTED_SYMBOLS = ["RemoteSettings"];
+var EXPORTED_SYMBOLS = [
+  "RemoteSettings",
+  "jexlFilterFunc"
+];
 
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
@@ -21,6 +24,7 @@ ChromeUtils.defineModuleGetter(this, "UptakeTelemetry",
                                "resource://services-common/uptake-telemetry.js");
 ChromeUtils.defineModuleGetter(this, "ClientEnvironmentBase",
                                "resource://gre/modules/components-utils/ClientEnvironment.jsm");
+ChromeUtils.defineModuleGetter(this, "FilterExpressions", "resource://normandy/lib/FilterExpressions.jsm");
 
 const PREF_SETTINGS_SERVER             = "services.settings.server";
 const PREF_SETTINGS_DEFAULT_BUCKET     = "services.settings.default_bucket";
@@ -62,6 +66,27 @@ class ClientEnvironment extends ClientEnvironmentBase {
   }
 }
 
+/**
+ * Default entry filtering function, in charge of excluding remote settings entries
+ * where the JEXL expression evaluates into a falsy value.
+ */
+async function jexlFilterFunc(entry, environment) {
+  const { filters } = entry;
+  if (!filters) {
+    return entry;
+  }
+  let result;
+  try {
+    const context = {
+      environment
+    };
+    result = await FilterExpressions.eval(filters, context);
+  } catch (e) {
+    Cu.reportError(e);
+  }
+  return result ? entry : null;
+}
+
 
 function mergeChanges(collection, localRecords, changes) {
   const records = {};
@@ -150,7 +175,7 @@ async function fetchLatestChanges(url, lastEtag) {
 
 class RemoteSettingsClient {
 
-  constructor(collectionName, { bucketName, signerName, filterFunc, lastCheckTimePref }) {
+  constructor(collectionName, { bucketName, signerName, filterFunc = jexlFilterFunc, lastCheckTimePref }) {
     this.collectionName = collectionName;
     this.bucketName = bucketName;
     this.signerName = signerName;

services/common/tests/unit/test_blocklist_clients.js

@@ -245,6 +245,57 @@ add_task(async function test_sync_event_data_is_filtered_for_target() {
 });
 add_task(clear_state);
 
+add_task(async function test_entries_are_filtered_when_jexl_filters_is_present() {
+  if (IS_ANDROID) {
+    // JEXL filters are not supported on Android.
+    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1463502
+    return;
+  }
+
+  const records = [{
+      willMatch: true,
+    }, {
+      willMatch: true,
+      filters: null
+    }, {
+      willMatch: true,
+      filters: "1 == 1"
+    }, {
+      willMatch: false,
+      filters: "1 == 2"
+    }, {
+      willMatch: true,
+      filters: "1 == 1",
+      versionRange: [{
+        targetApplication: [{
+          guid: "some-guid"
+        }],
+      }]
+    }, {
+      willMatch: false,  // jexl prevails over versionRange.
+      filters: "1 == 2",
+      versionRange: [{
+        targetApplication: [{
+          guid: "xpcshell@tests.mozilla.org",
+          minVersion: "0",
+          maxVersion: "*",
+        }],
+      }]
+    }
+  ];
+  for (let {client} of gBlocklistClients) {
+    const collection = await client.openCollection();
+    for (const record of records) {
+      await collection.create(record);
+    }
+    await collection.db.saveLastModified(42); // Prevent from loading JSON dump.
+    const list = await client.get();
+    equal(list.length, 4);
+    ok(list.every(e => e.willMatch));
+  }
+});
+add_task(clear_state);
+
 
 // get a response for a given request from sample data
 function getSampleResponse(req, port) {

services/common/tests/unit/test_remote_settings_jexl_filters.js

@@ -0,0 +1,171 @@
+const { RemoteSettings } = ChromeUtils.import("resource://services-common/remote-settings.js", {});
+
+let client;
+
+async function createRecords(records) {
+  const collection = await client.openCollection();
+  await collection.clear();
+  for (const record of records) {
+    await collection.create(record);
+  }
+  await collection.db.saveLastModified(42); // Prevent from loading JSON dump.
+}
+
+
+function run_test() {
+  client = RemoteSettings("some-key");
+
+  run_next_test();
+}
+
+add_task(async function test_returns_all_without_target() {
+  await createRecords([{
+    passwordSelector: "#pass-signin"
+  }, {
+    filters: null,
+  }, {
+    filters: "",
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 3);
+});
+
+add_task(async function test_filters_can_be_disabled() {
+  const c = RemoteSettings("no-jexl", { filterFunc: null });
+  const collection = await c.openCollection();
+  await collection.create({
+    filters: "1 == 2"
+  });
+  await collection.db.saveLastModified(42); // Prevent from loading JSON dump.
+
+  const list = await c.get();
+  equal(list.length, 1);
+});
+
+add_task(async function test_returns_entries_where_jexl_is_true() {
+  await createRecords([{
+    willMatch: true,
+    filters: "1"
+  }, {
+    willMatch: true,
+    filters: "[42]"
+  }, {
+    willMatch: true,
+    filters: "1 == 2 || 1 == 1"
+  }, {
+    willMatch: true,
+    filters: 'environment.appID == "xpcshell@tests.mozilla.org"'
+  }, {
+    willMatch: false,
+    filters: "environment.version == undefined"
+  }, {
+    willMatch: true,
+    filters: "environment.unknown == undefined"
+  }, {
+    willMatch: false,
+    filters: "1 == 2"
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 5);
+  ok(list.every(e => e.willMatch));
+});
+
+add_task(async function test_ignores_entries_where_jexl_is_invalid() {
+  await createRecords([{
+    filters: "true === true"  // JavaScript Error: "Invalid expression token: ="
+  }, {
+    filters: "Objects.keys({}) == []" // Token ( (openParen) unexpected in expression
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 0);
+});
+
+add_task(async function test_support_of_date_filters() {
+  await createRecords([{
+    willMatch: true,
+    filters: '"1982-05-08"|date < "2016-03-22"|date'
+  }, {
+    willMatch: false,
+    filters: '"2000-01-01"|date < "1970-01-01"|date'
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 1);
+  ok(list.every(e => e.willMatch));
+});
+
+add_task(async function test_support_of_preferences_filters() {
+  await createRecords([{
+    willMatch: true,
+    filters: '"services.settings.last_etag"|preferenceValue == 42'
+  }, {
+    willMatch: true,
+    filters: '"services.settings.changes.path"|preferenceExists == true'
+  }, {
+    willMatch: true,
+    filters: '"services.settings.changes.path"|preferenceIsUserSet == false'
+  }, {
+    willMatch: true,
+    filters: '"services.settings.last_etag"|preferenceIsUserSet == true'
+  }]);
+
+  // Set a pref for the user.
+  Services.prefs.setIntPref("services.settings.last_etag", 42);
+
+  const list = await client.get();
+  equal(list.length, 4);
+  ok(list.every(e => e.willMatch));
+});
+
+add_task(async function test_support_of_intersect_operator() {
+  await createRecords([{
+    willMatch: true,
+    filters: '{foo: 1, bar: 2}|keys intersect ["foo"]'
+  }, {
+    willMatch: true,
+    filters: '(["a", "b"] intersect ["a", 1, 4]) == "a"'
+  }, {
+    willMatch: false,
+    filters: '(["a", "b"] intersect [3, 1, 4]) == "c"'
+  }, {
+    willMatch: true,
+    filters: `
+      [1, 2, 3]
+        intersect
+      [3, 4, 5]
+    `
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 3);
+  ok(list.every(e => e.willMatch));
+});
+
+add_task(async function test_support_of_samples() {
+  await createRecords([{
+    willMatch: true,
+    filters: '"always-true"|stableSample(1)'
+  }, {
+    willMatch: false,
+    filters: '"always-false"|stableSample(0)'
+  }, {
+    willMatch: true,
+    filters: '"turns-to-true-0"|stableSample(0.5)'
+  }, {
+    willMatch: false,
+    filters: '"turns-to-false-1"|stableSample(0.5)'
+  }, {
+    willMatch: true,
+    filters: '"turns-to-true-0"|bucketSample(0, 50, 100)'
+  }, {
+    willMatch: false,
+    filters: '"turns-to-false-1"|bucketSample(0, 50, 100)'
+  }]);
+
+  const list = await client.get();
+  equal(list.length, 3);
+  ok(list.every(e => e.willMatch));
+});

services/common/tests/unit/test_restrequest.js

@@ -648,7 +648,7 @@ add_task(async function test_abort() {
 
   Assert.equal(request.status, request.ABORTED);
 
-  await Assert.rejects(responsePromise);
+  await Assert.rejects(responsePromise, /NS_BINDING_ABORTED/);
 
   await promiseStopServer(server);
 });

services/common/tests/unit/xpcshell.ini

@@ -22,6 +22,9 @@ tags = blocklist
 tags = remote-settings blocklist
 [test_remote_settings_poll.js]
 tags = remote-settings blocklist
+[test_remote_settings_jexl_filters.js]
+skip-if = os == "android"
+tags = remote-settings
 
 [test_kinto.js]
 tags = blocklist

services/fxaccounts/tests/xpcshell/test_accounts.js

@@ -287,13 +287,15 @@ add_task(async function test_update_account_data() {
     uid: "another_uid",
     assertion: "new_assertion",
   };
-  await Assert.rejects(account.updateUserAccountData(newCreds));
+  await Assert.rejects(account.updateUserAccountData(newCreds),
+    /The specified credentials aren't for the current user/);
 
   // should fail without the uid.
   newCreds = {
     assertion: "new_assertion",
   };
-  await Assert.rejects(account.updateUserAccountData(newCreds));
+  await Assert.rejects(account.updateUserAccountData(newCreds),
+    /The specified credentials aren't for the current user/);
 
   // and should fail with a field name that's not known by storage.
   newCreds = {
@@ -301,7 +303,8 @@ add_task(async function test_update_account_data() {
     uid: "another_uid",
     foo: "bar",
   };
-  await Assert.rejects(account.updateUserAccountData(newCreds));
+  await Assert.rejects(account.updateUserAccountData(newCreds),
+    /The specified credentials aren't for the current user/);
 });
 
 add_task(async function test_getCertificateOffline() {

services/fxaccounts/tests/xpcshell/test_accounts_config.js

@@ -23,7 +23,7 @@ add_task(async function test_non_https_remote_server_uri() {
   Services.prefs.setCharPref(
     "identity.fxaccounts.remote.root",
     "http://example.com/");
-  Assert.rejects(FxAccounts.config.promiseSignUpURI(), null, "Firefox Accounts server must use HTTPS");
+  await Assert.rejects(FxAccounts.config.promiseSignUpURI(), /Firefox Accounts server must use HTTPS/);
   Services.prefs.clearUserPref("identity.fxaccounts.remote.root");
 });
 

services/fxaccounts/tests/xpcshell/test_storage_manager.js

@@ -92,7 +92,7 @@ add_storage_task(async function checkInitializedEmpty(sm) {
   }
   await sm.initialize();
   Assert.strictEqual((await sm.getAccountData()), null);
-  Assert.rejects(sm.updateAccountData({kXCS: "kXCS"}), "No user is logged in");
+  await Assert.rejects(sm.updateAccountData({kXCS: "kXCS"}), /No user is logged in/);
 });
 
 // Initialized with account data (ie, simulating a new user being logged in).
@@ -185,13 +185,14 @@ add_storage_task(async function checkEverythingRead(sm) {
   }
 });
 
-add_storage_task(function checkInvalidUpdates(sm) {
+add_storage_task(async function checkInvalidUpdates(sm) {
   sm.plainStorage = new MockedPlainStorage({uid: "uid", email: "someone@somewhere.com"});
   if (sm.secureStorage) {
     sm.secureStorage = new MockedSecureStorage(null);
   }
-  Assert.rejects(sm.updateAccountData({uid: "another"}), "Can't change");
-  Assert.rejects(sm.updateAccountData({email: "someoneelse"}), "Can't change");
+  await sm.initialize();
+
+  await Assert.rejects(sm.updateAccountData({uid: "another"}), /Can't change uid/);
 });
 
 add_storage_task(async function checkNullUpdatesRemovedUnlocked(sm) {

services/sync/modules/telemetry.js

@@ -26,6 +26,8 @@ ChromeUtils.defineModuleGetter(this, "TelemetryUtils",
                                "resource://gre/modules/TelemetryUtils.jsm");
 ChromeUtils.defineModuleGetter(this, "TelemetryEnvironment",
                                "resource://gre/modules/TelemetryEnvironment.jsm");
+ChromeUtils.defineModuleGetter(this, "ObjectUtils",
+                               "resource://gre/modules/ObjectUtils.jsm");
 ChromeUtils.defineModuleGetter(this, "OS",
                                "resource://gre/modules/osfile.jsm");
 
@@ -86,6 +88,27 @@ function timeDeltaFrom(monotonicStartTime) {
   return -1;
 }
 
+// Converts extra integer fields to strings, rounds floats to three
+// decimal places (nanosecond precision for timings), and removes profile
+// directory paths and URLs from potential error messages.
+function normalizeExtraTelemetryFields(extra) {
+  let result = {};
+  for (let key in extra) {
+    let value = extra[key];
+    let type = typeof value;
+    if (type == "string") {
+      result[key] = cleanErrorMessage(value);
+    } else if (type == "number") {
+      result[key] = Number.isInteger(value) ? value.toString(10) :
+                    value.toFixed(3);
+    } else if (type != "undefined") {
+      throw new TypeError(`Invalid type ${
+        type} for extra telemetry field ${key}`);
+    }
+  }
+  return ObjectUtils.isEmpty(result) ? undefined : result;
+}
+
 // This function validates the payload of a telemetry "event" - this can be
 // removed once there are APIs available for the telemetry modules to collect
 // these events (bug 1329530) - but for now we simulate that planned API as
@@ -577,13 +600,18 @@ class SyncTelemetryImpl {
       return;
     }
 
+    let { object, method, value, extra } = eventDetails;
+    if (extra) {
+      extra = normalizeExtraTelemetryFields(extra);
+      eventDetails = { object, method, value, extra };
+    }
+
     if (!validateTelemetryEvent(eventDetails)) {
       // we've already logged what the problem is...
       return;
     }
     log.debug("recording event", eventDetails);
 
-    let { object, method, value, extra } = eventDetails;
     if (extra && Resource.serverTime && !extra.serverTime) {
       extra.serverTime = String(Resource.serverTime);
     }

services/sync/tests/unit/test_bookmark_duping.js

@@ -74,7 +74,8 @@ async function promiseNoLocalItem(guid) {
   let got = await bms.fetch({ guid });
   ok(!got, `No record remains with GUID ${guid}`);
   // and while we are here ensure the places cache doesn't still have it.
-  await Assert.rejects(PlacesUtils.promiseItemId(guid));
+  await Assert.rejects(PlacesUtils.promiseItemId(guid),
+    /no item found for the given GUID/);
 }
 
 async function validate(collection, expectedFailures = []) {

services/sync/tests/unit/test_browserid_identity.js

@@ -116,7 +116,7 @@ add_task(async function test_initialializeWithAuthErrorAndDeletedAccount() {
     let mockFxAClient = new AuthErrorMockFxAClient();
     browseridManager._fxaService.internal._fxAccountsClient = mockFxAClient;
 
-    await Assert.rejects(browseridManager._ensureValidToken(),
+    await Assert.rejects(browseridManager._ensureValidToken(), AuthenticationError,
                          "should reject due to an auth error");
 
     Assert.ok(signCertificateCalled);
@@ -256,7 +256,8 @@ add_task(async function test_ensureLoggedIn() {
   let fxa = globalBrowseridManager._fxaService;
   let signedInUser = fxa.internal.currentAccountState.storageManager.accountData;
   fxa.internal.currentAccountState.storageManager.accountData = null;
-  await Assert.rejects(globalBrowseridManager._ensureValidToken(true), "expecting rejection due to no user");
+  await Assert.rejects(globalBrowseridManager._ensureValidToken(true),
+    /Can't possibly get keys; User is not signed in/, "expecting rejection due to no user");
   // Restore the logged in user to what it was.
   fxa.internal.currentAccountState.storageManager.accountData = signedInUser;
   Status.login = LOGIN_FAILED_LOGIN_REJECTED;
@@ -302,6 +303,7 @@ add_task(async function test_getTokenErrors() {
   let browseridManager = Service.identity;
 
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       AuthenticationError,
                        "should reject due to 401");
   Assert.equal(Status.login, LOGIN_FAILED_LOGIN_REJECTED, "login was rejected");
 
@@ -317,6 +319,7 @@ add_task(async function test_getTokenErrors() {
   });
   browseridManager = Service.identity;
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       TokenServerClientServerError,
                        "should reject due to non-JSON response");
   Assert.equal(Status.login, LOGIN_FAILED_NETWORK_ERROR, "login state is LOGIN_FAILED_NETWORK_ERROR");
 });
@@ -401,6 +404,7 @@ add_task(async function test_getTokenErrorWithRetry() {
   let browseridManager = Service.identity;
 
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       TokenServerClientServerError,
                        "should reject due to 503");
 
   // The observer should have fired - check it got the value in the response.
@@ -419,6 +423,7 @@ add_task(async function test_getTokenErrorWithRetry() {
   browseridManager = Service.identity;
 
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       TokenServerClientServerError,
                        "should reject due to no token in response");
 
   // The observer should have fired - check it got the value in the response.
@@ -453,6 +458,7 @@ add_task(async function test_getKeysErrorWithBackoff() {
 
   let browseridManager = Service.identity;
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       TokenServerClientServerError,
                        "should reject due to 503");
 
   // The observer should have fired - check it got the value in the response.
@@ -489,6 +495,7 @@ add_task(async function test_getKeysErrorWithRetry() {
 
   let browseridManager = Service.identity;
   await Assert.rejects(browseridManager._ensureValidToken(),
+                       TokenServerClientServerError,
                        "should reject due to 503");
 
   // The observer should have fired - check it got the value in the response.
@@ -731,6 +738,8 @@ async function initializeIdentityWithHAWKResponseFactory(config, cbGetResponse)
   globalBrowseridManager._fxaService = fxa;
   globalBrowseridManager._signedInUser = await fxa.getSignedInUser();
   await Assert.rejects(globalBrowseridManager._ensureValidToken(true),
+                       // TODO: Ideally this should have a specific check for an error.
+                       () => true,
                        "expecting rejection due to hawk error");
 }
 

services/sync/tests/unit/test_fxa_service_cluster.js

@@ -15,7 +15,8 @@ add_task(async function test_findCluster() {
     body: "",
   });
 
-  await Assert.rejects(Service.identity._findCluster());
+  await Assert.rejects(Service.identity._findCluster(),
+    /TokenServerClientServerError/);
 
   _("_findCluster() returns null on authentication errors.");
   initializeIdentityWithTokenServerResponse({

services/sync/tests/unit/test_service_cluster.js

@@ -16,7 +16,8 @@ add_task(async function test_findCluster() {
     Service.identity._ensureValidToken = () => Promise.reject(new Error("Connection refused"));
 
     _("_findCluster() throws on network errors (e.g. connection refused).");
-    await Assert.rejects(Service.identity._findCluster());
+    await Assert.rejects(Service.identity._findCluster(),
+      /Connection refused/);
 
     Service.identity._ensureValidToken = () => Promise.resolve({ endpoint: "http://weave.user.node" });
 

servo/components/style/properties/properties.mako.rs

@@ -3199,6 +3199,7 @@ impl<'a> StyleBuilder<'a> {
     % endif
     % endif
     % endfor
+    <% del property %>
 
     /// Inherits style from the parent element, accounting for the default
     /// computed values that need to be provided as well.
@@ -3256,7 +3257,7 @@ impl<'a> StyleBuilder<'a> {
 
         /// Gets a mutable view of the current `${style_struct.name}` style.
         pub fn mutate_${style_struct.name_lower}(&mut self) -> &mut style_structs::${style_struct.name} {
-            % if not property.style_struct.inherited:
+            % if not style_struct.inherited:
             self.modified_reset = true;
             % endif
             self.${style_struct.ident}.mutate()
@@ -3264,7 +3265,7 @@ impl<'a> StyleBuilder<'a> {
 
         /// Gets a mutable view of the current `${style_struct.name}` style.
         pub fn take_${style_struct.name_lower}(&mut self) -> UniqueArc<style_structs::${style_struct.name}> {
-            % if not property.style_struct.inherited:
+            % if not style_struct.inherited:
             self.modified_reset = true;
             % endif
             self.${style_struct.ident}.take()
@@ -3288,6 +3289,7 @@ impl<'a> StyleBuilder<'a> {
                 StyleStructRef::Borrowed(self.reset_style.${style_struct.name_lower}_arc());
         }
     % endfor
+    <% del style_struct %>
 
     /// Returns whether this computed style represents a floated object.
     pub fn floated(&self) -> bool {

taskcluster/ci/test/talos.yml

@@ -352,10 +352,7 @@ talos-svgr:
     description: "Talos svgr"
     try-name: svgr
     treeherder-symbol: T(s)
-    run-on-projects:
-        by-test-platform:
-            windows10-64-qr/.*: []  # bug 1451305
-            default: ['mozilla-beta', 'mozilla-central', 'mozilla-inbound', 'autoland', 'try']
+    run-on-projects: ['mozilla-beta', 'mozilla-central', 'mozilla-inbound', 'autoland', 'try']
     max-run-time: 1800
     mozharness:
         extra-options:

testing/mozharness/scripts/desktop_partner_repacks.py

@@ -170,8 +170,9 @@ class DesktopPartnerRepacks(ReleaseMixin, BuildbotMixin, PurgeMixin,
             for locale in self.config["limitLocales"]:
                 repack_cmd.extend(["--limit-locale", locale])
 
-        return self.run_command(repack_cmd,
-                                cwd=self.query_abs_dirs()['abs_scripts_dir'])
+        self.run_command(repack_cmd,
+                         cwd=self.query_abs_dirs()['abs_scripts_dir'],
+                         halt_on_failure=True)
 
 
 # main {{{

toolkit/components/places/SyncedBookmarksMirror.jsm

@@ -60,7 +60,6 @@ XPCOMUtils.defineLazyModuleGetters(this, {
   Async: "resource://services-common/async.js",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   Log: "resource://gre/modules/Log.jsm",
-  ObjectUtils: "resource://gre/modules/ObjectUtils.jsm",
   OS: "resource://gre/modules/osfile.jsm",
   PlacesSyncUtils: "resource://gre/modules/PlacesSyncUtils.jsm",
   PlacesUtils: "resource://gre/modules/PlacesUtils.jsm",
@@ -224,8 +223,7 @@ class SyncedBookmarksMirror {
       try {
         let info = await OS.File.stat(path);
         let size = Math.floor(info.size / 1024);
-        options.recordTelemetryEvent("mirror", "open", "success",
-                                     normalizeExtraTelemetryFields({ size }));
+        options.recordTelemetryEvent("mirror", "open", "success", { size });
       } catch (ex) {
         MirrorLog.warn("Error recording stats for mirror database size", ex);
       }
@@ -352,17 +350,6 @@ class SyncedBookmarksMirror {
    */
   async store(records, { needsMerge = true } = {}) {
     let options = { needsMerge };
-    let ignoreCounts = {
-      bookmark: { id: 0, url: 0 },
-      query: { id: 0, url: 0 },
-      folder: { id: 0, root: 0 },
-      child: { id: 0, root: 0 },
-      livemark: { id: 0, feed: 0 },
-      separator: { id: 0 },
-      tombstone: { id: 0, root: 0 },
-    };
-    let extraTelemetryEvents = [];
-    try {
     await this.db.executeBeforeShutdown(
       "SyncedBookmarksMirror: store",
       db => db.executeTransaction(async () => {
@@ -370,52 +357,35 @@ class SyncedBookmarksMirror {
           MirrorLog.trace(`Storing in mirror: ${record.cleartextToString()}`);
           switch (record.type) {
             case "bookmark":
-                await this.storeRemoteBookmark(record, ignoreCounts, options);
+              await this.storeRemoteBookmark(record, options);
               continue;
 
             case "query":
-                await this.storeRemoteQuery(record, ignoreCounts, options);
+              await this.storeRemoteQuery(record, options);
               continue;
 
             case "folder":
-                await this.storeRemoteFolder(record, ignoreCounts, options);
+              await this.storeRemoteFolder(record, options);
               continue;
 
             case "livemark":
-                await this.storeRemoteLivemark(record, ignoreCounts, options);
+              await this.storeRemoteLivemark(record, options);
               continue;
 
             case "separator":
-                await this.storeRemoteSeparator(record, ignoreCounts, options);
+              await this.storeRemoteSeparator(record, options);
               continue;
 
             default:
               if (record.deleted) {
-                  await this.storeRemoteTombstone(record, ignoreCounts,
-                                                  options);
+                await this.storeRemoteTombstone(record, options);
                 continue;
               }
           }
           MirrorLog.warn("Ignoring record with unknown type", record.type);
-            extraTelemetryEvents.push({
-              method: "ignore",
-              value: "unknown-kind",
-              extra: { kind: record.type },
-            });
         }
       }
     ));
-    } finally {
-      for (let { method, value, extra } of extraTelemetryEvents) {
-        this.recordTelemetryEvent("mirror", method, value, extra);
-      }
-      for (let kind in ignoreCounts) {
-        let extra = normalizeExtraTelemetryFields(ignoreCounts[kind]);
-        if (extra) {
-          this.recordTelemetryEvent("mirror", "ignore", kind, extra);
-        }
-      }
-    }
   }
 
   /**
@@ -443,61 +413,69 @@ class SyncedBookmarksMirror {
   async apply({ localTimeSeconds = Date.now() / 1000,
                 remoteTimeSeconds = 0,
                 weakUpload = [] } = {}) {
+    // We intentionally don't use `executeBeforeShutdown` in this function,
+    // since merging can take a while for large trees, and we don't want to
+    // block shutdown. Since all new items are in the mirror, we'll just try
+    // to merge again on the next sync.
+
     let hasChanges = weakUpload.length > 0 || (await this.hasChanges());
     if (!hasChanges) {
       MirrorLog.debug("No changes detected in both mirror and Places");
       return {};
     }
-    // We intentionally don't use `executeBeforeShutdown` in this function,
-    // since merging can take a while for large trees, and we don't want to
-    // block shutdown. Since all new items are in the mirror, we'll just try
-    // to merge again on the next sync.
-    let { missingParents, missingChildren } = await this.fetchRemoteOrphans();
+
+    // The flow ID is used to correlate telemetry events for each sync.
+    let flowID = PlacesUtils.history.makeGuid();
+
+    let { missingParents, missingChildren, parentsWithGaps } =
+      await this.fetchRemoteOrphans();
     if (missingParents.length) {
       MirrorLog.warn("Temporarily reparenting remote items with missing " +
                      "parents to unfiled", missingParents);
-      this.recordTelemetryEvent("mirror", "orphans", "parents",
-        normalizeExtraTelemetryFields({ count: missingParents.length }));
     }
     if (missingChildren.length) {
       MirrorLog.warn("Remote tree missing items", missingChildren);
-      this.recordTelemetryEvent("mirror", "orphans", "children",
-        normalizeExtraTelemetryFields({ count: missingChildren.length }));
+    }
+    if (parentsWithGaps.length) {
+      MirrorLog.warn("Remote tree has parents with gaps in positions",
+                     parentsWithGaps);
     }
 
     let { missingLocal, missingRemote, wrongSyncStatus } =
-      await this.fetchInconsistencies();
+      await this.fetchSyncStatusMismatches();
     if (missingLocal.length) {
       MirrorLog.warn("Remote tree has merged items that don't exist locally",
                      missingLocal);
-      this.recordTelemetryEvent("mirror", "inconsistencies", "local",
-        normalizeExtraTelemetryFields({ count: missingLocal.length }));
     }
     if (missingRemote.length) {
       MirrorLog.warn("Local tree has synced items that don't exist remotely",
                      missingRemote);
-      this.recordTelemetryEvent("mirror", "inconsistencies", "remote",
-        normalizeExtraTelemetryFields({ count: missingRemote.length }));
     }
     if (wrongSyncStatus.length) {
       MirrorLog.warn("Local tree has wrong sync statuses for items that " +
                      "exist remotely", wrongSyncStatus);
-      this.recordTelemetryEvent("mirror", "inconsistencies", "syncStatus",
-        normalizeExtraTelemetryFields({ count: wrongSyncStatus.length }));
     }
 
-    let applyStats = {};
+    this.recordTelemetryEvent("mirror", "apply", "problems", {
+      flowID,
+      missingParents: missingParents.length,
+      missingChildren: missingChildren.length,
+      parentsWithGaps: parentsWithGaps.length,
+      missingLocal: missingLocal.length,
+      missingRemote: missingRemote.length,
+      wrongSyncStatus: wrongSyncStatus.length,
+    });
 
     // It's safe to build the remote tree outside the transaction because
     // `fetchRemoteTree` doesn't join to Places, only Sync writes to the
     // mirror, and we're holding the Sync lock at this point.
-    MirrorLog.debug("Building remote tree from mirror");
-    let { result: remoteTree, time: remoteTreeTiming } = await withTiming(
-      "Fetch remote tree",
-      () => this.fetchRemoteTree(remoteTimeSeconds)
+    let remoteTree = await withTiming(
+      "Building remote tree from mirror",
+      () => this.fetchRemoteTree(remoteTimeSeconds),
+      (time, tree) => this.recordTelemetryEvent("mirror", "apply",
+        "fetchRemoteTree", { flowID, time, deletions: tree.deletedGuids.size,
+                             nodes: tree.byGuid.size })
     );
-    applyStats.remoteTree = { time: remoteTreeTiming,
-                              count: remoteTree.guidCount };
     if (MirrorLog.level <= Log.Level.Debug) {
       MirrorLog.debug("Built remote tree from mirror\n" +
                       remoteTree.toASCIITreeString());
@@ -505,57 +483,51 @@ class SyncedBookmarksMirror {
 
     let observersToNotify = new BookmarkObserverRecorder(this.db);
 
-    let changeRecords = await this.db.executeTransaction(async () => {
-      MirrorLog.debug("Building local tree from Places");
-      let { result: localTree, time: localTreeTiming } = await withTiming(
-        "Fetch local tree",
-        () => this.fetchLocalTree(localTimeSeconds)
+    let changeRecords;
+    try {
+      changeRecords = await this.db.executeTransaction(async () => {
+        let localTree = await withTiming(
+          "Building local tree from Places",
+          () => this.fetchLocalTree(localTimeSeconds),
+          (time, tree) => this.recordTelemetryEvent("mirror", "apply",
+            "fetchLocalTree", { flowID, time, deletions: tree.deletedGuids.size,
+                                nodes: tree.byGuid.size })
         );
-      applyStats.localTree = { time: localTreeTiming,
-                               count: localTree.guidCount };
         if (MirrorLog.level <= Log.Level.Debug) {
           MirrorLog.debug("Built local tree from Places\n" +
                           localTree.toASCIITreeString());
         }
 
-      MirrorLog.debug("Fetching content info for new mirror items");
-      let {
-        result: newRemoteContents,
-        time: remoteContentsTiming,
-      } = await withTiming(
-        "Fetch new remote contents",
-        () => this.fetchNewRemoteContents()
+        let newRemoteContents = await withTiming(
+          "Fetching content info for new mirror items",
+          () => this.fetchNewRemoteContents(),
+          (time, contents) => this.recordTelemetryEvent("mirror", "apply",
+            "fetchNewRemoteContents", { flowID, time, count: contents.size })
         );
-      applyStats.remoteContents = { time: remoteContentsTiming,
-                                    count: newRemoteContents.size };
 
-      MirrorLog.debug("Fetching content info for new Places items");
-      let {
-        result: newLocalContents,
-        time: localContentsTiming,
-      } = await withTiming(
-        "Fetch new local contents",
-        () => this.fetchNewLocalContents()
+        let newLocalContents = await withTiming(
+          "Fetching content info for new Places items",
+          () => this.fetchNewLocalContents(),
+          (time, contents) => this.recordTelemetryEvent("mirror", "apply",
+            "fetchNewLocalContents", { flowID, time, count: contents.size })
         );
-      applyStats.localContents = { time: localContentsTiming,
-                                   count: newLocalContents.size };
 
-      MirrorLog.debug("Building complete merged tree");
         let merger = new BookmarkMerger(localTree, newLocalContents,
                                         remoteTree, newRemoteContents);
-      let mergedRoot;
-      try {
-        let time;
-        ({ result: mergedRoot, time } = await withTiming(
-          "Build merged tree",
-          () => merger.merge()
-        ));
-        applyStats.merge = { time };
-      } finally {
-        for (let { value, extra } of merger.summarizeTelemetryEvents()) {
-          this.recordTelemetryEvent("mirror", "merge", value, extra);
-        }
+        let mergedRoot = await withTiming(
+          "Building complete merged tree",
+          () => merger.merge(),
+          time => {
+            this.recordTelemetryEvent("mirror", "apply", "merge",
+              { flowID, time, nodes: merger.mergedGuids.size,
+                localDeletions: merger.deleteLocally.size,
+                remoteDeletions: merger.deleteRemotely.size,
+                dupes: merger.dupeCount });
+
+            this.recordTelemetryEvent("mirror", "merge", "structure",
+              merger.structureCounts);
           }
+        );
 
         if (MirrorLog.level <= Log.Level.Debug) {
           MirrorLog.debug([
@@ -577,16 +549,18 @@ class SyncedBookmarksMirror {
             "Merged tree doesn't mention all items from remote tree");
         }
 
-      MirrorLog.debug("Applying merged tree");
+        await withTiming(
+          "Applying merged tree",
+          async () => {
             let deletions = [];
             for await (let deletion of yieldingIterator(merger.deletions())) {
               deletions.push(deletion);
             }
-      let { time: updateTiming } = await withTiming(
-        "Apply merged tree",
-        () => this.updateLocalItemsInPlaces(mergedRoot, deletions)
+            await this.updateLocalItemsInPlaces(mergedRoot, deletions);
+          },
+          time => this.recordTelemetryEvent("mirror", "apply",
+            "updateLocalItemsInPlaces", { flowID, time })
         );
-      applyStats.update = { time: updateTiming };
 
         // At this point, the database is consistent, and we can fetch info to
         // pass to observers. Note that we can't fetch observer info in the
@@ -594,21 +568,31 @@ class SyncedBookmarksMirror {
         // incomplete structure might cause us to miss or record wrong parents and
         // positions.
 
-      MirrorLog.debug("Recording observer notifications");
-      await this.noteObserverChanges(observersToNotify);
+        await withTiming(
+          "Recording observer notifications",
+          () => this.noteObserverChanges(observersToNotify),
+          time => this.recordTelemetryEvent("mirror", "apply",
+            "noteObserverChanges", { flowID, time })
+        );
 
-      let {
-        result: changeRecords,
-        time: stageTiming,
-      } = await withTiming("Stage outgoing items", async () => {
-        MirrorLog.debug("Staging locally changed items for upload");
-        await this.stageItemsToUpload(weakUpload);
+        await withTiming(
+          "Staging locally changed items for upload",
+          () => this.stageItemsToUpload(weakUpload),
+          time => this.recordTelemetryEvent("mirror", "apply",
+            "stageItemsToUpload", { flowID, time })
+        );
 
-        MirrorLog.debug("Fetching records for local items to upload");
-        return this.fetchLocalChangeRecords();
-      });
-      applyStats.stage = { time: stageTiming };
+        let changeRecords = await withTiming(
+          "Fetching records for local items to upload",
+          () => this.fetchLocalChangeRecords(),
+          (time, records) => this.recordTelemetryEvent("mirror", "apply",
+            "fetchLocalChangeRecords", { flowID,
+              count: Object.keys(records).length })
+        );
 
+        await withTiming(
+          "Cleaning up merge tables",
+          async () => {
             await this.db.execute(`DELETE FROM mergeStates`);
             await this.db.execute(`DELETE FROM itemsAdded`);
             await this.db.execute(`DELETE FROM guidsChanged`);
@@ -618,9 +602,21 @@ class SyncedBookmarksMirror {
             await this.db.execute(`DELETE FROM annosChanged`);
             await this.db.execute(`DELETE FROM idsToWeaklyUpload`);
             await this.db.execute(`DELETE FROM itemsToUpload`);
+          },
+          time => this.recordTelemetryEvent("mirror", "apply", "cleanup",
+            { flowID, time })
+        );
 
         return changeRecords;
       });
+    } catch (ex) {
+      // Include the error message in the event payload, since we can't
+      // easily correlate event telemetry to engine errors in the Sync ping.
+      let why = (typeof ex.message == "string" ? ex.message :
+                 String(ex)).slice(0, 85);
+      this.recordTelemetryEvent("mirror", "apply", "error", { flowID, why });
+      throw ex;
+    }
 
     MirrorLog.debug("Replaying recorded observer notifications");
     try {
@@ -629,13 +625,6 @@ class SyncedBookmarksMirror {
       MirrorLog.warn("Error notifying Places observers", ex);
     }
 
-    for (let value in applyStats) {
-      let extra = normalizeExtraTelemetryFields(applyStats[value]);
-      if (extra) {
-        this.recordTelemetryEvent("mirror", "apply", value, extra);
-      }
-    }
-
     return changeRecords;
   }
 
@@ -662,11 +651,10 @@ class SyncedBookmarksMirror {
     return rows.map(row => row.getResultByName("guid"));
   }
 
-  async storeRemoteBookmark(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteBookmark(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring bookmark with invalid ID", record.id);
-      ignoreCounts.bookmark.id++;
       return;
     }
 
@@ -674,7 +662,6 @@ class SyncedBookmarksMirror {
     if (!url) {
       MirrorLog.warn("Ignoring bookmark ${guid} with invalid URL ${url}",
                      { guid, url: record.bmkUri });
-      ignoreCounts.bookmark.url++;
       return;
     }
 
@@ -717,11 +704,10 @@ class SyncedBookmarksMirror {
     }
   }
 
-  async storeRemoteQuery(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteQuery(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring query with invalid ID", record.id);
-      ignoreCounts.query.id++;
       return;
     }
 
@@ -729,7 +715,6 @@ class SyncedBookmarksMirror {
     if (!url) {
       MirrorLog.warn("Ignoring query ${guid} with invalid URL ${url}",
                      { guid, url: record.bmkUri });
-      ignoreCounts.query.url++;
       return;
     }
 
@@ -743,7 +728,6 @@ class SyncedBookmarksMirror {
       if (!tagFolderName) {
         MirrorLog.warn("Ignoring tag query ${guid} with invalid tag name " +
                        "${tagFolderName}", { guid, tagFolderName });
-        ignoreCounts.query.url++;
         return;
       }
       url = new URL(`place:tag=${tagFolderName}`);
@@ -773,17 +757,15 @@ class SyncedBookmarksMirror {
         url: url.href, description, smartBookmarkName });
   }
 
-  async storeRemoteFolder(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteFolder(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring folder with invalid ID", record.id);
-      ignoreCounts.folder.id++;
       return;
     }
     if (guid == PlacesUtils.bookmarks.rootGuid) {
       // The Places root shouldn't be synced at all.
       MirrorLog.warn("Ignoring Places root record", record);
-      ignoreCounts.folder.root++;
     }
 
     let serverModified = determineServerModified(record);
@@ -810,13 +792,11 @@ class SyncedBookmarksMirror {
           MirrorLog.warn("Ignoring child of folder ${parentGuid} with " +
                          "invalid ID ${childRecordId}", { parentGuid: guid,
                                                           childRecordId });
-          ignoreCounts.child.id++;
           continue;
         }
         if (childGuid == PlacesUtils.bookmarks.rootGuid ||
             PlacesUtils.bookmarks.userContentRoots.includes(childGuid)) {
           MirrorLog.warn("Ignoring move for root", childGuid);
-          ignoreCounts.child.root++;
           continue;
         }
         await this.db.executeCached(`
@@ -841,11 +821,10 @@ class SyncedBookmarksMirror {
       }
   }
 
-  async storeRemoteLivemark(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteLivemark(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring livemark with invalid ID", record.id);
-      ignoreCounts.livemark.id++;
       return;
     }
 
@@ -853,7 +832,6 @@ class SyncedBookmarksMirror {
     if (!feedURL) {
       MirrorLog.warn("Ignoring livemark ${guid} with invalid feed URL ${url}",
                      { guid, url: record.feedUri });
-      ignoreCounts.livemark.feed++;
       return;
     }
 
@@ -874,11 +852,10 @@ class SyncedBookmarksMirror {
         siteURL: siteURL ? siteURL.href : null });
   }
 
-  async storeRemoteSeparator(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteSeparator(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring separator with invalid ID", record.id);
-      ignoreCounts.separator.id++;
       return;
     }
 
@@ -894,18 +871,16 @@ class SyncedBookmarksMirror {
         dateAdded });
   }
 
-  async storeRemoteTombstone(record, ignoreCounts, { needsMerge }) {
+  async storeRemoteTombstone(record, { needsMerge }) {
     let guid = validateGuid(record.id);
     if (!guid) {
       MirrorLog.warn("Ignoring tombstone with invalid ID", record.id);
-      ignoreCounts.tombstone.id++;
       return;
     }
 
     if (guid == PlacesUtils.bookmarks.rootGuid ||
         PlacesUtils.bookmarks.userContentRoots.includes(guid)) {
       MirrorLog.warn("Ignoring tombstone for root", guid);
-      ignoreCounts.tombstone.root++;
       return;
     }
 
@@ -929,19 +904,29 @@ class SyncedBookmarksMirror {
     let infos = {
       missingParents: [],
       missingChildren: [],
+      parentsWithGaps: [],
     };
 
     let orphanRows = await this.db.execute(`
-      SELECT v.guid AS guid, 1 AS missingParent, 0 AS missingChild
+      SELECT v.guid AS guid, 1 AS missingParent, 0 AS missingChild,
+             0 AS parentWithGaps
       FROM items v
       LEFT JOIN structure s ON s.guid = v.guid
       WHERE NOT v.isDeleted AND
             s.guid IS NULL
       UNION ALL
-      SELECT s.guid AS guid, 0 AS missingParent, 1 AS missingChild
+      SELECT s.guid AS guid, 0 AS missingParent, 1 AS missingChild,
+             0 AS parentsWithGaps
       FROM structure s
       LEFT JOIN items v ON v.guid = s.guid
-      WHERE v.guid IS NULL`);
+      WHERE v.guid IS NULL
+      UNION ALL
+      SELECT s.parentGuid AS guid, 0 AS missingParent, 0 AS missingChild,
+             1 AS parentWithGaps
+      FROM structure s
+      GROUP BY s.parentGuid
+      HAVING (sum(DISTINCT position + 1) -
+                  (count(*) * (count(*) + 1) / 2)) <> 0`);
 
     for await (let row of yieldingIterator(orphanRows)) {
       let guid = row.getResultByName("guid");
@@ -953,6 +938,10 @@ class SyncedBookmarksMirror {
       if (missingChild) {
         infos.missingChildren.push(guid);
       }
+      let parentWithGaps = row.getResultByName("parentWithGaps");
+      if (parentWithGaps) {
+        infos.parentsWithGaps.push(guid);
+      }
     }
 
     return infos;
@@ -971,7 +960,7 @@ class SyncedBookmarksMirror {
    *           - `missingRemote`: NORMAL items in the local tree that aren't
    *             mentioned in the remote tree.
    */
-  async fetchInconsistencies() {
+  async fetchSyncStatusMismatches() {
     let infos = {
       missingLocal: [],
       missingRemote: [],
@@ -1895,28 +1884,6 @@ class DatabaseCorruptError extends Error {
   }
 }
 
-// Converts extra integer fields to strings, and rounds timings to nanosecond
-// precision.
-function normalizeExtraTelemetryFields(extra) {
-  let result = {};
-  for (let key in extra) {
-    let value = extra[key];
-    let type = typeof value;
-    if (type == "string") {
-      result[key] = value;
-    } else if (type == "number") {
-      if (value > 0) {
-        result[key] = Number.isInteger(value) ? value.toString(10) :
-                      value.toFixed(3);
-      }
-    } else if (type != "undefined") {
-      throw new TypeError(`Invalid type ${
-        type} for extra telemetry field ${key}`);
-    }
-  }
-  return ObjectUtils.isEmpty(result) ? undefined : result;
-}
-
 // Indicates if the mirror should be replaced because the database file is
 // corrupt.
 function isDatabaseCorrupt(error) {
@@ -2835,15 +2802,31 @@ async function inflateTree(tree, pseudoTree, parentNode) {
   }
 }
 
-// Executes a function and returns a `{ result, time }` tuple, where `result` is
-// the function's return value, and `time` is the time taken to execute the
-// function.
-async function withTiming(name, func) {
+/**
+ * Measures and logs the time taken to execute a function, using a monotonic
+ * clock.
+ *
+ * @param  {String} name
+ *         The name of the operation, used for logging.
+ * @param  {Function} func
+ *         The function to time.
+ * @param  {Function} recordTiming
+ *         A function with the signature `(time: Number, result: Object?)`,
+ *         where `time` is the measured time, and `result` is the return
+ *         value of the timed function.
+ * @return The return value of the timed function.
+ */
+async function withTiming(name, func, recordTiming) {
+  MirrorLog.debug(name);
+
   let startTime = Cu.now();
   let result = await func();
   let elapsedTime = Cu.now() - startTime;
+
   MirrorLog.trace(`${name} took ${elapsedTime.toFixed(3)}ms`);
-  return { result, time: elapsedTime };
+  recordTiming(elapsedTime, result);
+
+  return result;
 }
 
 /**
@@ -3194,10 +3177,6 @@ class BookmarkTree {
     this.deletedGuids = new Set();
   }
 
-  get guidCount() {
-    return this.byGuid.size + this.deletedGuids.size;
-  }
-
   isDeleted(guid) {
     return this.deletedGuids.has(guid);
   }
@@ -3382,22 +3361,6 @@ class BookmarkMerger {
       remoteDeletes: 0, // Remote folder deletion wins over local change.
     };
     this.dupeCount = 0;
-    this.extraTelemetryEvents = [];
-  }
-
-  summarizeTelemetryEvents() {
-    let events = [...this.extraTelemetryEvents];
-    if (this.dupeCount > 0) {
-      events.push({
-        value: "dupes",
-        extra: normalizeExtraTelemetryFields({ count: this.dupeCount }),
-      });
-    }
-    let structureExtra = normalizeExtraTelemetryFields(this.structureCounts);
-    if (structureExtra) {
-      events.push({ value: "structure", extra: structureExtra });
-    }
-    return events;
   }
 
   async merge() {
@@ -3548,11 +3511,6 @@ class BookmarkMerger {
     if (!localNode.hasCompatibleKind(remoteNode)) {
       MirrorLog.error("Merging local ${localNode} and remote ${remoteNode} " +
                       "with different kinds", { localNode, remoteNode });
-      this.extraTelemetryEvents.push({
-        value: "kind-mismatch",
-        extra: { local: localNode.kindToString().toLowerCase(),
-                 remote: remoteNode.kindToString().toLowerCase() },
-      });
       throw new SyncedBookmarksMirror.ConsistencyError(
         "Can't merge different item kinds");
     }

toolkit/components/places/tests/sync/test_bookmark_corruption.js

@@ -8,16 +8,7 @@ async function getCountOfBookmarkRows(db) {
 }
 
 add_task(async function test_corrupt_roots() {
-  let telemetryEvents = [];
-  let buf = await openMirror("corrupt_roots", {
-    recordTelemetryEvent(object, method, value, extra) {
-      if (object == "mirror" && ["open", "apply"].includes(method)) {
-        // Ignore timings, mirror database file, and tree sizes.
-        return;
-      }
-      telemetryEvents.push({ object, method, value, extra });
-    },
-  });
+  let buf = await openMirror("corrupt_roots");
 
   info("Set up empty mirror");
   await PlacesTestUtils.markBookmarksAsSynced();
@@ -48,17 +39,6 @@ add_task(async function test_corrupt_roots() {
 
   let changesToUpload = await buf.apply();
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
-  deepEqual(telemetryEvents, [{
-    object: "mirror",
-    method: "ignore",
-    value: "child",
-    extra: { root: "1" },
-  }, {
-    object: "mirror",
-    method: "ignore",
-    value: "tombstone",
-    extra: { root: "1" },
-  }], "Should record telemetry for ignored invalid roots");
 
   deepEqual(changesToUpload, {}, "Should not reupload invalid roots");
 

toolkit/components/places/tests/sync/test_bookmark_deduping.js

@@ -28,7 +28,7 @@ add_task(async function test_duping_local_newer() {
 
   // The mirror is out of sync because `bookmarkAAA5` is marked as merged,
   // even though it's not in Places, but we should still recover.
-  deepEqual(await buf.fetchInconsistencies(), {
+  deepEqual(await buf.fetchSyncStatusMismatches(), {
     missingLocal: ["bookmarkAAA5"],
     missingRemote: [],
     wrongSyncStatus: [],
@@ -86,11 +86,9 @@ add_task(async function test_duping_local_newer() {
   });
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
-    value: "dupes",
-    extra: { count: "2" },
-  }, {
     value: "structure",
-    extra: { new: "1" },
+    extra: { new: 1, remoteRevives: 0, localDeletes: 0, localRevives: 0,
+             remoteDeletes: 0 },
   }], "Should record telemetry with dupe counts");
 
   let menuInfo = await PlacesUtils.bookmarks.fetch(

toolkit/components/places/tests/sync/test_bookmark_deletion.js

@@ -114,7 +114,8 @@ add_task(async function test_complex_orphaning() {
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "2", localDeletes: "1", remoteDeletes: "1" },
+    extra: { new: 2, remoteRevives: 0, localDeletes: 1, localRevives: 0,
+             remoteDeletes: 1 },
   }], "Should record telemetry with structure change counts");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
@@ -306,7 +307,8 @@ add_task(async function test_locally_modified_remotely_deleted() {
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "1", localRevives: "1", remoteDeletes: "2" },
+    extra: { new: 1, remoteRevives: 0, localDeletes: 0, localRevives: 1,
+             remoteDeletes: 2 },
   }], "Should record telemetry for local item and remote folder deletions");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);
@@ -453,7 +455,8 @@ add_task(async function test_locally_deleted_remotely_modified() {
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "1", remoteRevives: "1", localDeletes: "2" },
+    extra: { new: 1, remoteRevives: 1, localDeletes: 2, localRevives: 0,
+             remoteDeletes: 0 },
   }], "Should record telemetry for remote item and local folder deletions");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);

toolkit/components/places/tests/sync/test_bookmark_kinds.js

@@ -418,15 +418,12 @@ add_task(async function test_mismatched_but_compatible_folder_types() {
 });
 
 add_task(async function test_mismatched_but_incompatible_folder_types() {
-  let sawMismatchEvent = false;
+  let sawMismatchError = false;
   let recordTelemetryEvent = (object, method, value, extra) => {
-    // expecting to see a kind-mismatch event.
-    if (value == "kind-mismatch" &&
-        extra.local && typeof extra.local == "string" &&
-        extra.local == "livemark" &&
-        extra.remote && typeof extra.remote == "string" &&
-        extra.remote == "folder") {
-      sawMismatchEvent = true;
+    // expecting to see an error for kind mismatches.
+    if (method == "apply" && value == "error" &&
+        extra && extra.why == "Can't merge different item kinds") {
+      sawMismatchError = true;
     }
   };
   let buf = await openMirror("mismatched_incompatible_types",
@@ -458,7 +455,7 @@ add_task(async function test_mismatched_but_incompatible_folder_types() {
 
     info("Apply remote, should fail");
     await Assert.rejects(buf.apply(), /Can't merge different item kinds/);
-    Assert.ok(sawMismatchEvent, "saw the correct mismatch event");
+    Assert.ok(sawMismatchError, "saw the correct mismatch event");
   } finally {
     await buf.finalize();
     await PlacesUtils.bookmarks.eraseEverything();
@@ -539,15 +536,12 @@ add_task(async function test_different_but_compatible_bookmark_types() {
 });
 
 add_task(async function test_incompatible_types() {
-  let sawMismatchEvent = false;
+  let sawMismatchError = false;
   let recordTelemetryEvent = (object, method, value, extra) => {
-    // expecting to see a kind-mismatch event.
-    if (value == "kind-mismatch" &&
-        extra.local && typeof extra.local == "string" &&
-        extra.local == "bookmark" &&
-        extra.remote && typeof extra.remote == "string" &&
-        extra.remote == "folder") {
-      sawMismatchEvent = true;
+    // expecting to see an error for kind mismatches.
+    if (method == "apply" && value == "error" &&
+        extra && extra.why == "Can't merge different item kinds") {
+      sawMismatchError = true;
     }
   };
   try {
@@ -582,7 +576,7 @@ add_task(async function test_incompatible_types() {
     await PlacesTestUtils.markBookmarksAsSynced();
 
     await Assert.rejects(buf.apply(), /Can't merge different item kinds/);
-    Assert.ok(sawMismatchEvent, "saw expected mismatch event");
+    Assert.ok(sawMismatchError, "saw expected mismatch event");
   } finally {
     await PlacesUtils.bookmarks.eraseEverything();
     await PlacesSyncUtils.bookmarks.reset();

toolkit/components/places/tests/sync/test_bookmark_structure_changes.js

@@ -647,7 +647,8 @@ add_task(async function test_complex_move_with_additions() {
   deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
   deepEqual(mergeTelemetryEvents, [{
     value: "structure",
-    extra: { new: "1" },
+    extra: { new: 1, remoteRevives: 0, localDeletes: 0, localRevives: 0,
+             remoteDeletes: 0 },
   }], "Should record telemetry with structure change counts");
 
   let idsToUpload = inspectChangeRecords(changesToUpload);

toolkit/components/places/tests/sync/test_bookmark_validation.js

@@ -48,7 +48,7 @@ add_task(async function test_inconsistencies() {
   await PlacesUtils.bookmarks.remove("bookmarkCCCC");
   await PlacesUtils.bookmarks.remove("bookmarkDDDD");
 
-  deepEqual(await buf.fetchInconsistencies(), {
+  deepEqual(await buf.fetchSyncStatusMismatches(), {
     missingLocal: [],
     missingRemote: [],
     wrongSyncStatus: [],
@@ -96,7 +96,7 @@ add_task(async function test_inconsistencies() {
   }]);
 
   let { missingLocal, missingRemote, wrongSyncStatus } =
-    await buf.fetchInconsistencies();
+    await buf.fetchSyncStatusMismatches();
   deepEqual(missingLocal, ["bookmarkGGGG"],
     "Should report merged remote items that don't exist locally");
   deepEqual(missingRemote.sort(), ["bookmarkBBBB", "bookmarkCCCC"],