1663750 - ETP Custom Content Blocking List choosing UI is broken in Nightly 82

Status: RESOLVED FIXED

(Firefox :: Protections UI, defect)

Description

[Steven Englehardt [:englehardt]]

The list chooser is broken again in Nightly 82. We very recently fixed a regression in Bug 1657679 that caused this issue.

STR:

1. In a new profile enable the preference browser.contentblocking.customBlockList.preferences.ui.enabled. This preference will be enabled for any users that had the Level 2 blocklist enabled when we previously deprecated the list chooser UI.
2. Go to about:preferences#privacy
3. Choose ETP Custom and click "Change block list"

ER: A single popup opens with two list options

AR: Two popups open, both of which are blank. Oddly enough, the double pop-up only occurs the first time. Clearing all Firefox data (via "Clear Recent History") will cause the double pop-up to re-appear, but only once until storage is cleared again.

Comment 1

[Steven Englehardt [:englehardt]]

There appear to be two separate regression here: the double pop-up issue, and the empty list chooser.

Running mozregression for the double-popup led to the following:

2020-09-08T12:39:17.914000: INFO : Narrowed integration regression window from [626c0384, 5a12a490] (3 builds) to [2da97f87, 5a12a490] (2 builds) (~1 steps left)
2020-09-08T12:39:18.475000: DEBUG : Found commit message:
Bug 1651958 - Updated prompt tests to support SubDialog chrome prompts. r=Gijs

Differential Revision: https://phabricator.services.mozilla.com/D84380

Running mozregression for the empty list chooser led to the following:

2020-09-08T12:50:14.616000: INFO : Narrowed integration regression window from [4bb25142, d6959d52] (5 builds) to [6a68aa4d, d6959d52] (2 builds) (~1 steps left)
2020-09-08T12:50:15.393000: DEBUG : Found commit message:
Bug 1661030 - Updated tests for external protocol dialog in TabDialogBox. r=Gijs,mixedpuppy

Differential Revision: https://phabricator.services.mozilla.com/D88425

2020-09-08T12:50:15.397000: DEBUG : Did not find a branch, checking all integration branches
2020-09-08T12:50:15.415000: INFO : The bisection is done.

Both of these point back to changes that are only in test files. Will need to dig further.

Comment 2

[Steven Englehardt [:englehardt]]

Attachment: Screenshot from 2020-09-08 12-27-29.png

Here's a visual of both issues: two pop-ups and an empty list chooser.

Comment 3

[Steven Englehardt [:englehardt]]

The bugs associated with the regressions:

• The empty list chooser regression is from Bug 1661030.
• The double popup window regression is from Bug 1651958.

Comment 4

[Steven Englehardt [:englehardt]]

pbz: would you mind to look into this?

Comment 5

[:Gijs (he/him)]

Are there automated tests for this feature? If there are, it'd be useful to understand why they didn't notice the breakage; if there aren't, that'd help explain why we didn't notice this until well after this change landed...

The first of these changes is now late on beta, so we have very limited opportunities to fix. Do we have any idea how many people on release are going to be in this group of users?

Comment 6

[Steven Englehardt [:englehardt]]

(In reply to :Gijs (he/him) from comment #5)

Are there automated tests for this feature? If there are, it'd be useful to understand why they didn't notice the breakage; if there aren't, that'd help explain why we didn't notice this until well after this change landed...

Unfortunately no, there aren't automated tests, which has bitten us before. We'd like to remove this UI (Bug 1658044), but the timeline for that is uncertain so adding tests in the meantime seems necessary. I've filed Bug 1663777 for that.

The first of these changes is now late on beta, so we have very limited opportunities to fix. Do we have any idea how many people on release are going to be in this group of users?

We don't have Telemetry, but are planning to add some in support of Bug 1658044. I suspect this impacts a very small number of users as the number of users with Strict Tracking protection enabled is a only few percent. Only a fraction of those users will still have access to this UI and only a fraction further will actually interact with it.

Since the only impact of the earlier regression is the double pop-up, I think it's fine to let that ride to release. We should be sure to fix the empty list chooser issue, and uplift to beta if necessary. The main reason a user would go to this UI is to downgrade the list to fix breakage, and that's not possible when the list chooser is empty.

Comment 7

[Paul Zühlcke [:pbz]]

Attachment: Bug 1663750 - blocklists.xhtml: Fixed accessing undefined window.arguments. r=gijs

Bug 1661030 refactored SubDialog to support passing multiple window arguments.
Before that window.arguments was always an array with one element, even if the
caller did not pass any data.

window.arguments should be null checked before accessing it.
However, in this case the arguments were unused, so I removed the assignment.

Comment 8

[Paul Zühlcke [:pbz]]

Attachment: Bug 1663750 - Removed duplicate click event listener for 'changeBlockListLink'. r=gijs,englehardt

The 'allowDuplicateDialogs' option covered up a bug where we had two click event listeners for the
changeBlockListLink. This should fix the duplicate dialog.
However, there seem to be timing issues with the 'allowDuplicateDialogs' option we should look
into as follow up work.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/32afa0e93b02
Removed duplicate click event listener for 'changeBlockListLink'. r=Gijs,preferences-reviewers

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/372f140d5b96
blocklists.xhtml: Fixed accessing undefined window.arguments. r=Gijs,preferences-reviewers

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/32afa0e93b02
https://hg.mozilla.org/mozilla-central/rev/372f140d5b96

Comment 12

[:Gijs (he/him)]

Steven, can you verify this is fixed on tonight's (today's late) nightly once that's built? We can probably uplift both of these changes to beta if so.

Comment 13

[Steven Englehardt [:englehardt]]

(In reply to :Gijs (he/him) from comment #12)

Steven, can you verify this is fixed on tonight's (today's late) nightly once that's built? We can probably uplift both of these changes to beta if so.

Yes, I've verified that this is now fixed. Thank you!

Comment 14

[:Gijs (he/him)]

Comment on attachment 9174642 [details]
Bug 1663750 - Removed duplicate click event listener for 'changeBlockListLink'. r=gijs,englehardt

Beta/Release Uplift Approval Request

• User impact if declined: Broken behaviour for users attempting to interact with blocklists
• 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: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small well-understood patch - just removing a duplicate event listener

Note: we don't need to uplift the other patch, though we could.

• String changes made/needed: nope

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9174642 [details]
Bug 1663750 - Removed duplicate click event listener for 'changeBlockListLink'. r=gijs,englehardt

Approved for 81.0rc1.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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