1634819 - Login autocomplete broken if field parent is a Shadow Root: can't access property "tagName", parentElement is null

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Matthew N. [:MattN]]

STR:

1. Visit about:logins and add a (fake) saved login for https://www.virustotal.com
2. Load https://www.virustotal.com/gui/sign-in
3. Focus the password field (clear it if it's non-empty)

Expected result:
Password autocomplete appears

Actual result:
Error in Browser Console (at least with signon.debug=true)

The problem is that NewPasswordModel assume that the parentElement of the <input> is non-null but the Shadow Root isn't an element, it's just a Node, so it is null. In this case we should use Node.parentNode.nodeName or handle element.parentElement being null.

LoginAutoComplete: TypeError: "can't access property "tagName", parentElement is null"
    hasLabelMatchingRegex resource://gre/modules/NewPasswordModel.jsm:152
    hasCurrentLabel resource://gre/modules/NewPasswordModel.jsm:362
    getSubfactsFromFunction resource://gre/modules/third_party/fathom/fathom.jsm:1687
    fact resource://gre/modules/third_party/fathom/fathom.jsm:1734
    updateFnode resource://gre/modules/third_party/fathom/fathom.jsm:2288
    forEach resource://gre/modules/third_party/fathom/fathom.jsm:608
    results resource://gre/modules/third_party/fathom/fathom.jsm:2278
    _execute resource://gre/modules/third_party/fathom/fathom.jsm:2683
    get resource://gre/modules/third_party/fathom/fathom.jsm:2598
    _computeType resource://gre/modules/third_party/fathom/fathom.jsm:2107
    scoreFor resource://gre/modules/third_party/fathom/fathom.jsm:2008
    _isProbablyANewPasswordField resource://gre/modules/LoginAutoComplete.jsm:691
    _requestAutoCompleteResultsFromParent resource://gre/modules/LoginAutoComplete.jsm:636
    startSearch resource://gre/modules/LoginAutoComplete.jsm:597
LoginAutoComplete.jsm:605:37

Comment 1

[Tim Spurway [:tspurway]]

hey matt, could you add a severity? also, do you think this would drive/ride a dot release?

Comment 2

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: New regression in 76 around a feature we are marketing. Saved login autocomplete and password generation are broken with specific forms of shadow DOM that are fairly common nowadays. The context menu is a workaround so that's why I didn't choose S1.

I think this should ride-along but not drive a dot release. I suspect Bianca will be starting on this today.

Bianca, note that you should fix this on model 3.1 so we can uplift, not on master from the fathom-login-forms repo.

Comment 3

[Bianca Danforth [:bdanforth]]

Attachment: 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%.

Going with simply 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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f63a19d7effa
Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN

Comment 5

[Bianca Danforth [:bdanforth]]

Comment on attachment 9146360 [details]
Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN

Beta/Release Uplift Approval Request

• User impact if declined: In certain cases where a ShadowRoot is part of a form with a password field, the Password Manager autocomplete popup will not work on the password field due to an unhandled error. We estimate a small percentage of pages to be affected by this (<< 10%).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR are in the description for the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1634819#c0).
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change just adds some checks to make sure that an object exists (i.e. is not null) before we try to access a property on it. It does not change the logic for showing the autocomplete popup.
• String changes made/needed:

Comment 6

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f63a19d7effa

Comment 7

[Cristian Fogel [:cfogel]]

Confirmed issue with 77.0a1(20200501094247) & 78.0a1(20200508033807) on macOS 10.15.3.
Verified with the treeherder builds for Nighlty branch.

Comment 8

[Pascal Chevrel:pascalc]

Comment on attachment 9146360 [details]
Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN

Fixes a P1 regression and verified by QA in nightly, uplift approved for the next beta, thanks.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed29b69288dd

Comment 10

[Cristian Fogel [:cfogel]]

Verified with 77.0b4 on macOS 10.15.3, Windows 10, Ubuntu 20.

Comment 11

[Bianca Danforth [:bdanforth]]

Comment on attachment 9146360 [details]
Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN

Beta/Release Uplift Approval Request

• User impact if declined: In certain cases where a ShadowRoot is part of a form with a password field, the Password Manager autocomplete popup will not work on the password field due to an unhandled error. We estimate a small percentage of pages to be affected by this (<< 10%).
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change just adds some checks to make sure that an object exists (i.e. is not null) before we try to access a property on it. It does not change the logic for showing the autocomplete popup.

We are hoping this uplift could ride along if there is a dot release, though it does not warrant a dot release by itself.

• String changes made/needed:

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9146360 [details]
Bug 1634819 - Existence check element.parentElement and element.previousElementSibling in new password heuristics; r=MattN

Not planning a 76 dot release at this time with 77 going to RC next week.