Autocomplete delete action triggers a chrome level error

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Form Manager
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jkt, Assigned: jkt)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Comment 1

2 years ago
The error I get is:
"TypeError: this._resultCache is undefined[Learn More]  AutoCompletePopup.jsm:185:9"
(Assignee)

Comment 2

2 years ago
Created attachment 8794205 [details] [diff] [review]
jkt-bug-1305050.patch

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

Updated

2 years ago
Assignee: nobody → jkt
(Assignee)

Comment 3

2 years ago
Created attachment 8794206 [details] [diff] [review]
jkt-bug-1305050.patch

Removing another line of code which also wasn't specified.
Attachment #8794205 - Attachment is obsolete: true
(Assignee)

Comment 4

2 years ago
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
Attachment #8794206 - Flags: review?(MattN+bmo)
(Assignee)

Comment 5

2 years ago
Created attachment 8794232 [details] [diff] [review]
jkt-bug-1305050.patch

Actually update the patch :/
Attachment #8794206 - Attachment is obsolete: true
Attachment #8794206 - Flags: review?(MattN+bmo)
Attachment #8794232 - Flags: review?(MattN+bmo)
Blocks: 1294502
Status: NEW → ASSIGNED
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?
Attachment #8794232 - Flags: review?(MattN+bmo)
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Updated

2 years ago
Depends on: 1305962
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

2 years ago
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.
Attachment #8797831 - Flags: review?(MattN+bmo)
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
Attachment #8797831 - Flags: review?(MattN+bmo)
(Assignee)

Comment 14

2 years ago
mozreview-review-reply
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 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!
Attachment #8797831 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 17

2 years ago
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 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
Attachment #8797831 - Flags: review?(MattN+bmo) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae10e0596387
Removing chrome error in deleting password autocomplete and improving testing to catch issues. r=MattN,mconley
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ae10e0596387
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.