Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN
authorBianca Danforth <bdanforth@mozilla.com>
Thu, 07 May 2020 23:12:47 +0000
changeset 528714 f63a19d7effa285d05013488a3ed32bd3e71be81
parent 528713 65fc0df9b6fa24e16b29620a67b15ebeddbec508
child 528715 ead8f0367372fe5767d33a0aa0a95b07ee76ea75
push id37393
push userrmaries@mozilla.com
push dateFri, 08 May 2020 03:38:07 +0000
treeherdermozilla-central@ead8f0367372 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersMattN
bugs1634819, 1629132
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN The element.parentNode.nodeName approach also handled the error, but when I ran the updated model through the training, validation and testing samples, our false positive rate (FPR) went up from 2.2% to 4.5% (referencing the most recent update to NewPasswordModel.jsm in bug 1629132) with a confidence threshold of 0.75. Unfortunately, that is well above our target FPR of 2-3%. * I am not surprised by this difference: the diff I had tested with `parentNode.nodeName` replaced `parentElement` with `parentNode` and `tagName` with `nodeName` everywhere in this rule. Since `Node.ELEMENT_NODE` is only one of [several other `Node.nodeType`](https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType)'s, I can imagine that for some of the sites in the corpus, `element.parentElement` was not the same node as `element.parentNode`. Therefore the downstream return value could also be different; i.e. the modified rule may then return `false` instead of `true` or vice versa for some or all of these affected pages, and the model would consequently reach a different (and potentially worse) optimization. Returning `false` for this rule in these cases will leave the model's performance unchanged on pages that don't use the Shadow DOM at all or in this specific way. Also add some existence checks further downstream in the same rule where the values could also be `null`. Differential Revision: https://phabricator.services.mozilla.com/D74164
toolkit/components/passwordmgr/NewPasswordModel.jsm
--- a/toolkit/components/passwordmgr/NewPasswordModel.jsm
+++ b/toolkit/components/passwordmgr/NewPasswordModel.jsm
@@ -144,23 +144,33 @@ function makeRuleset(coeffs, biases) {
         return regex.test(
           min(labelledBy, node => euclidean(node, element)).textContent
         );
       }
     }
 
     const parentElement = element.parentElement;
     // Check if the input is in a <td>, and, if so, check the textContent of the containing <tr>
-    if (parentElement.tagName === "TD") {
+    if (
+      // Bug 1634819: element.parentElement is null if element.parentNode is a ShadowRoot
+      parentElement &&
+      parentElement.tagName === "TD" &&
+      parentElement.parentElement
+    ) {
       // TODO: How bad is the assumption that the <tr> won't be the parent of the <td>?
       return regex.test(parentElement.parentElement.textContent);
     }
 
     // Check if the input is in a <dd>, and, if so, check the textContent of the preceding <dt>
-    if (parentElement.tagName === "DD") {
+    if (
+      parentElement &&
+      parentElement.tagName === "DD" &&
+      // previousElementSibling can be null
+      parentElement.previousElementSibling
+    ) {
       return regex.test(parentElement.previousElementSibling.textContent);
     }
     return false;
   }
 
   function closestLabelMatchesRegex(element, regex) {
     const previousElementSibling = element.previousElementSibling;
     if (