1305050 - Autocomplete delete action triggers a chrome level error

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Jonathan Kingston [:jkt] he/him]

It looks like _resultsCache was removed in toolkit/components/satchel/AutoCompletePopup.jsm however there is still some code active when a user deletes the identity with the delete key so opening a bug.

From what I can tell the bug that caused this was:
https://bugzilla.mozilla.org/show_bug.cgi?id=1294502

Comment 1

[Jonathan Kingston [:jkt] he/him]

The error I get is:
"TypeError: this._resultCache is undefined[Learn More]  AutoCompletePopup.jsm:185:9"

Comment 2

[Jonathan Kingston [:jkt] he/him]

Attachment: jkt-bug-1305050.patch

It looks like we can just remove these lines of code as no longer used.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Attachment: jkt-bug-1305050.patch

Removing another line of code which also wasn't specified.

Comment 4

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8794206 [details] [diff] [review]
jkt-bug-1305050.patch

Hey Matt,
This removes the log I was getting when removing a login.
Would you be able to review this?
Thanks

Comment 5

[Jonathan Kingston [:jkt] he/him]

Attachment: jkt-bug-1305050.patch

Actually update the patch :/

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8794232 [details] [diff] [review]
jkt-bug-1305050.patch

So after this patch we still remove the login from the visible dropdown too in e10s and non-e10s?

I don't understand how a test didn't catch this. Does the error occur in the logs for https://dxr.mozilla.org/mozilla-central/rev/058cf01f6cf2d2526c28b864a78afd4b97189b2a/toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html#420
Can you make one of the testcases in that file using `doKey("delete"…` fail without your patch so we can prevent this in the future?

Comment 7

[Jonathan Kingston [:jkt] he/him]

The error does fire with that test however the test still passes, I guess we should have a test checking for thrown errors there as well.

Comment 8

[Jonathan Kingston [:jkt] he/him]

Looks like the chrome context error isn't accessible at all here, I may need to put a similar test in that context to catch thrown errors.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Is this ready for review, jkt?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Sorry - I just noticed you've already received feedback.

Comment 11

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

Review commit: https://reviewboard.mozilla.org/r/83438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/83438/

Comment 12

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

Sorry this test took so long, should be ok now. I fixed the tests to output messages for expect condition and also the counting logins was wrong too.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

https://reviewboard.mozilla.org/r/83438/#review82692

::: toolkit/components/passwordmgr/test/browser/browser_basic_form_autocomplete.js:1
(Diff revision 1)
> +/*

This test doesn't really belong as a browser-chrome one since I believe our other autocomplete ones are mochitest-plain and as a result you're duplicating a helper. I think you said you were having trouble with  Services.console.registerListener and the SpecialPowers equivalent but I suspect that was just due to e10s. I think that SpecialPowers should be given the relevant ability to see the console messages you want instead of changing suites since the problem won't just affect pwmgr.

I also don't think this needs to be its own test. We can check for TypeErrors in all of the mochitest-plain tests via pwmgr_common.js like I thought you started on.

Now that I have more context I'm actually fine with skipping a test for this bug since I'm guessing you're not going to want to convert back to mochitest-plain.

::: toolkit/components/passwordmgr/test/browser/browser_basic_form_autocomplete.js:2
(Diff revision 1)
> +/*
> + * Test capture popup notifications

I think this is stale

::: toolkit/components/passwordmgr/test/browser/browser_basic_form_autocomplete.js:5
(Diff revision 1)
> +let LoginManager = Cc["@mozilla.org/login-manager;1"].
> +                   getService(Ci.nsILoginManager);

Nit: use `Services.logins` instead

::: toolkit/components/passwordmgr/test/mochitest/test_basic_form_autocomplete.html:435
(Diff revision 1)
>    is(numLogins, 5, "Correct number of logins before deleting one");
>  
> +  let countChangedPromise = notifyMenuChanged(3);
>    var deletionPromise = promiseStorageChanged(["removeLogin"]);
>    // On OS X, shift-backspace and shift-delete work, just delete does not.
>    // On Win/Linux, shift-backspace does not work, delete and shift-delete do.
>    doKey("delete", shiftModifier);
>    yield deletionPromise;

If we have 5 logins and then delete one, why are we expecting 3?

::: toolkit/components/satchel/AutoCompletePopup.jsm
(Diff revision 1)
> -    if (idx !== -1) {
> -      this.removeEntry(idx, false);
> -    }

I'm going to defer reviewing this hunk to mconley…

::: toolkit/components/satchel/test/parent_utils.js:77
(Diff revision 1)
>        return gAutocompletePopup.tree.view.rowCount === expectedCount &&
>          (!expectedFirstValue ||
>            expectedCount <= 1 ||
>            gAutocompletePopup.tree.view.getCellText(0, gAutocompletePopup.tree.columns[0]) ===
>            expectedFirstValue);
> -    }).then(() => {
> +    }, 'Waiting for row count change: ' + expectedCount + ' First value: ' + expectedFirstValue).then(() => {

Nit: double quotes

::: toolkit/components/satchel/test/parent_utils.js:87
(Diff revision 1)
>  
>    checkSelectedIndex(expectedIndex) {
>      ContentTaskUtils.waitForCondition(() => {
>        return gAutocompletePopup.popupOpen &&
>               gAutocompletePopup.selectedIndex === expectedIndex;
> -    }).then(() => {
> +    }, 'Checking selected index').then(() => {

Nit: double quotes

Comment 14

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

https://reviewboard.mozilla.org/r/83438/#review82692

> This test doesn't really belong as a browser-chrome one since I believe our other autocomplete ones are mochitest-plain and as a result you're duplicating a helper. I think you said you were having trouble with  Services.console.registerListener and the SpecialPowers equivalent but I suspect that was just due to e10s. I think that SpecialPowers should be given the relevant ability to see the console messages you want instead of changing suites since the problem won't just affect pwmgr.
> 
> I also don't think this needs to be its own test. We can check for TypeErrors in all of the mochitest-plain tests via pwmgr_common.js like I thought you started on.
> 
> Now that I have more context I'm actually fine with skipping a test for this bug since I'm guessing you're not going to want to convert back to mochitest-plain.

Ok will check if that works as this is mostly a copy of en existing test.

> If we have 5 logins and then delete one, why are we expecting 3?

Because the one with a blank username doesn't appear.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

https://reviewboard.mozilla.org/r/83438/#review82986

r=me! This should probably also be uplifted to Aurora, since bug 1294502 landed in 51.

::: toolkit/components/satchel/AutoCompletePopup.jsm
(Diff revision 1)
> -    // It's possible to race and have the deleted login no longer be in our
> -    // resultCache's logins, so we remove it from the database above and only
> -    // deal with our resultCache below.
> -    let idx = this._resultCache.logins.findIndex(cur => {
> -      return login.guid === cur.QueryInterface(Ci.nsILoginMetaInfo).guid
> -    });
> -    if (idx !== -1) {
> -      this.removeEntry(idx, false);
> -    }

Yeah, this is a leftover fragment that should not be in here anymore. Thanks for catching this!

Comment 16

[Jonathan Kingston [:jkt] he/him]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/83438/diff/1-2/

Comment 17

[Jonathan Kingston [:jkt] he/him]

I removed the test mentioned as Bug 1305962 work should be able to pick this up better (the patch there also checks for this error).

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8797831 [details]
Bug 1305050 - Removing chrome error in deleting password autocomplete and improving testing to catch issues.

https://reviewboard.mozilla.org/r/83438/#review83462

Thanks

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ae10e0596387
Removing chrome error in deleting password autocomplete and improving testing to catch issues. r=MattN,mconley

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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