1527562 - autocomplete-richlistitem CE is reused for autocomplete-richlistitem-insecure-field and vice-versa

Status: VERIFIED FIXED

(Toolkit :: XUL Widgets, defect, P1)

Description

[Matthew N. [:MattN]]

The issue seems to be that the richlistitem's are being re-used inappropriately which maybe wasn't an issue before but since we're using this.addEventListener("click", … instead of <handler event="click" button="0">[1] I think the appropriate event handling isn't apply after a richlistem switches from one to the other which makes sense with how addEventListener works and I assume <handler> would be removed when a new moz-binding applied.

STR 1 (broken insecure Learn More link)

1. In a new session, save a login on https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page by submitting the login form.
2. Reload the login page and cause the autocomplete popup to open with a key icon on the username or password field (probably need to backspace the autofilled value).
3. Switch to http://http-login.badssl.com/ in another tab
4. Trigger the autocomplete popup in one of the login fields to see the insecure field warning
5. Click on the first row to learn more.

Expected result: SUMO tab opens
Actual result: Nothing

STR 2 (SUMO tab opens at the same time as filling a secure login form)

1. In a new session, load http://http-login.badssl.com/
2. Focus the field and cause the autocomplete popup to open. Clicking the learn more row should open SUMO. Close SUMO
3. Load https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page and save a login by submitting the form
4. Reload https://wiki.mozilla.org/index.php?title=Special:UserLogin&returnto=Main+Page (while not logged in) and trigger the autocomplete popup on the username or password field after backspacing the value.
5. Click on the first row with the username to fill it.

Expected result: Login is autofilled
Actual result: Login is autofilled AND irrelevant SUMO tab is opened.

I think there was an existing bug that maybe wasn't user-visible but event listeners in CE aren't as nice as XBL handlers so this issue is now a real problem. I think the "or" on line 1048[2] and the "||" on line 1050 should be "and" and "&&" respectively, do you agree Alexander? It seems like we should only re-use when the style is the same and it's not in the list of ones not re-usable.

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0bd44bd411#l4.132
[2] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1048-1051

Comment 1

[alexander :surkov (:asurkov)]

Attachment: Bug 1527562 - autocomplete-richlistitem CE is reused for autocomplete-richlistitem-insecure-field and vice-versa, r=MattN

Comment 2

[alexander :surkov (:asurkov)]

Attachment: Bug 1527562 - autocomplete-richlistitem CE is reused for autocomplete-richlistitem-insecure-field and vice-versa, r=MattN

Comment 3

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

I think there was an existing bug that maybe wasn't user-visible but event listeners in CE aren't as nice as XBL handlers so this issue is now a real problem. I think the "or" on line 1048[2] and the "||" on line 1050 should be "and" and "&&" respectively, do you agree Alexander? It seems like we should only re-use when the style is the same and it's not in the list of ones not re-usable.

[2] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1048-1051

Alexander, do you agree that the operator is also wrong?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

I think there was an existing bug that maybe wasn't user-visible but event listeners in CE aren't as nice as XBL handlers so this issue is now a real problem. I think the "or" on line 1048[2] and the "||" on line 1050 should be "and" and "&&" respectively, do you agree Alexander? It seems like we should only re-use when the style is the same and it's not in the list of ones not re-usable.

[2] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1048-1051

Alexander, do you agree that the operator is also wrong?

missed your original comment, right, reusable iif same style or old and new styles are interchangeable, i.e. are not in UNREUSEABLE_STYLES.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #4)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

I think there was an existing bug that maybe wasn't user-visible but event listeners in CE aren't as nice as XBL handlers so this issue is now a real problem. I think the "or" on line 1048[2] and the "||" on line 1050 should be "and" and "&&" respectively, do you agree Alexander? It seems like we should only re-use when the style is the same and it's not in the list of ones not re-usable.

[2] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1048-1051

Alexander, do you agree that the operator is also wrong?

missed your original comment, right, reusable iif same style or old and new styles are interchangeable, i.e. are not in UNREUSEABLE_STYLES.

oh, but it's the same !(a || b), so the condition appears to be right

Comment 6

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #5)

(In reply to alexander :surkov (:asurkov) from comment #4)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #3)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #0)

I think there was an existing bug that maybe wasn't user-visible but event listeners in CE aren't as nice as XBL handlers so this issue is now a real problem. I think the "or" on line 1048[2] and the "||" on line 1050 should be "and" and "&&" respectively, do you agree Alexander? It seems like we should only re-use when the style is the same and it's not in the list of ones not re-usable.

[2] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1048-1051

Alexander, do you agree that the operator is also wrong?

missed your original comment, right, reusable iif same style or old and new styles are interchangeable, i.e. are not in UNREUSEABLE_STYLES.

oh, but it's the same !(a || b), so the condition appears to be right

I think you got confused since the line number changed with your array addition… I'm saying

reusable = originalType === style ||

should be

reusable = originalType === style &&

Comment 7

[alexander :surkov (:asurkov)]

ah, no, it shouldn't be I think, because we want to reuse an item if

• its style wasn't changed (for example, "autofill-profile" is changed to "autofill-profile") OR
• its old and new styles are both interchangeable, for example, style "bla-bla" is changed to style "foo-foo" (they both share same CE and thus we can reuse them)

Right?

Comment 8

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #7)

ah, no, it shouldn't be I think, because we want to reuse an item if

• its style wasn't changed (for example, "autofill-profile" is changed to "autofill-profile") OR
• its old and new styles are both interchangeable, for example, style "bla-bla" is changed to style "foo-foo" (they both share same CE and thus we can reuse them)

From reading the comment at line 1040[1] I thought that we want an AND since it seemed to imply that once _adjustAcItem is used on an item, it shouldn't be re-used. IIRC the issue is that the different autofill item types have different heights and that's what UNREUSEABLE_STYLES is for… I thought it was so that even if items have the same style that they still don't get re-used e.g. an autofill-profile is rendered at one height on a narrow field and another on a wide field, see https://luke-chang.github.io/autofill-demo/layout.html

Also, the problem we're seeing in this bug is that the constructor doesn't run when a row is re-used. To me that means we should never re-use if the style isn't the same and therefore an AND makes sense.

I think we only need to change to an && and not modify UNREUSEABLE_STYLES.

[1] https://searchfox.org/mozilla-central/rev/cb7faaf6b4ad2528390186f1ce64618dea71031e/toolkit/content/widgets/autocomplete.xml#1040-1051

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

(In reply to alexander :surkov (:asurkov) from comment #7)

ah, no, it shouldn't be I think, because we want to reuse an item if

• its style wasn't changed (for example, "autofill-profile" is changed to "autofill-profile") OR
• its old and new styles are both interchangeable, for example, style "bla-bla" is changed to style "foo-foo" (they both share same CE and thus we can reuse them)

From reading the comment at line 1040[1] I thought that we want an AND since it seemed to imply that once _adjustAcItem is used on an item, it shouldn't be re-used. IIRC the issue is that the different autofill item types have different heights and that's what UNREUSEABLE_STYLES is for… I thought it was so that even if items have the same style that they still don't get re-used e.g. an autofill-profile is rendered at one height on a narrow field and another on a wide field, see https://luke-chang.github.io/autofill-demo/layout.html

not sure, I know little about this code. There's also 2nd comment there though "// Reuse the item when its style is exactly equal to the previous style or neither of their style are in the UNREUSEABLE_STYLES." which also says "or". I assumed the point was to reuse autocomplete-richlistitem CE binding for everything that isn't listed in UNREUSEABLE_STYLES.

Also, the problem we're seeing in this bug is that the constructor doesn't run when a row is re-used. To me that means we should never re-use if the style isn't the same and therefore an AND makes sense.

I think we only need to change to an && and not modify UNREUSEABLE_STYLES.

It feels like a separate issue with me, and I'm not 100% sure this is a correct change, but if you are certain we should do that here, then I can adjust my patch.

Comment 10

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #9)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #8)

(In reply to alexander :surkov (:asurkov) from comment #7)

ah, no, it shouldn't be I think, because we want to reuse an item if

• its style wasn't changed (for example, "autofill-profile" is changed to "autofill-profile") OR
• its old and new styles are both interchangeable, for example, style "bla-bla" is changed to style "foo-foo" (they both share same CE and thus we can reuse them)

From reading the comment at line 1040[1] I thought that we want an AND since it seemed to imply that once _adjustAcItem is used on an item, it shouldn't be re-used. IIRC the issue is that the different autofill item types have different heights and that's what UNREUSEABLE_STYLES is for… I thought it was so that even if items have the same style that they still don't get re-used e.g. an autofill-profile is rendered at one height on a narrow field and another on a wide field, see https://luke-chang.github.io/autofill-demo/layout.html

not sure, I know little about this code. There's also 2nd comment there though "// Reuse the item when its style is exactly equal to the previous style or neither of their style are in the UNREUSEABLE_STYLES." which also says "or". I assumed the point was to reuse autocomplete-richlistitem CE binding for everything that isn't listed in UNREUSEABLE_STYLES.

The insecure password one already existed when UNREUSEABLE_STYLES was added though… I don't trust the comment since it was written at the same time as the code.

Also, the problem we're seeing in this bug is that the constructor doesn't run when a row is re-used. To me that means we should never re-use if the style isn't the same and therefore an AND makes sense.

I think we only need to change to an && and not modify UNREUSEABLE_STYLES.

It feels like a separate issue with me, and I'm not 100% sure this is a correct change, but if you are certain we should do that here, then I can adjust my patch.

I'm not sure it's a separate issue but I do know that you're using UNREUSEABLE_STYLES for a purpose that it wasn't originally meant for. I think just changing to && shouldn't break functionality, it will just mean doing more work in some cases when switching between item types.

Comment 11

[Matthew N. [:MattN]]

I also think it just makes sense to have the custom elements be independent so that you don't need to remember to update that list when adding a new richlistitem.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #10)

It feels like a separate issue with me, and I'm not 100% sure this is a correct change, but if you are certain we should do that here, then I can adjust my patch.

I'm not sure it's a separate issue but I do know that you're using UNREUSEABLE_STYLES for a purpose that it wasn't originally meant for. I think just changing to && shouldn't break functionality, it will just mean doing more work in some cases when switching between item types.

agreed, it shouldn't regress anywhere, except perhaps perceived performance, however actually I would be surprised to see any downgrades there, as there's no many elements involved in practice. So, it should be ok to make the change, but iirc there's a whole bunch of styles associated with default autocomplete-richlistitem. Are you sure you want to drop them from reusing as well? How about dropping |originalType === style| from the condition? It would make us recreate all items which styles are in UNREUSEABLE_STYLES and reusing everything else.

Comment 13

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #12)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #10)

It feels like a separate issue with me, and I'm not 100% sure this is a correct change, but if you are certain we should do that here, then I can adjust my patch.

I'm not sure it's a separate issue but I do know that you're using UNREUSEABLE_STYLES for a purpose that it wasn't originally meant for. I think just changing to && shouldn't break functionality, it will just mean doing more work in some cases when switching between item types.

agreed, it shouldn't regress anywhere, except perhaps perceived performance, however actually I would be surprised to see any downgrades there, as there's no many elements involved in practice. So, it should be ok to make the change, but iirc there's a whole bunch of styles associated with default autocomplete-richlistitem. Are you sure you want to drop them from reusing as well? How about dropping |originalType === style| from the condition? It would make us recreate all items which styles are in UNREUSEABLE_STYLES and reusing everything else.

Oh, yeah, I forgot the address bar uses this code too… We can go with your original patch then.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/453da38535d4
autocomplete-richlistitem CE is reused for autocomplete-richlistitem-insecure-field and vice-versa, r=MattN

Comment 15

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/453da38535d4

Comment 16

[Adrian Florinescu [:aflorinescu] [PTO till 06/12, redirect to [e-mail]/[e-mail]]]

Using comment0 steps, verified as fixed:

Mac 10.13.6

67.0b14 20190424140259
68.0a1 20190429215338

Windows 10 Pro x64

68.0a1 20190429215338
67.0b14 20190424140259

Ubuntu 16.04 x64

67.0b14 20190424140259
68.0a1 20190429215338