1578459 - [Protections Panel] Category item order flickers when panel is opened for the first time after a location change

Status: VERIFIED FIXED

(Firefox :: Site Identity, task, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Theoretically it could happen in any situation when the popup is opened if a content blocking event was processed while it was closed, but after a location change is the most obvious case.

This can be fixed by moving the reorderCategoryItems function call to the popupshowing event rather than popupshown.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1578459 - [Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Comment 2

[(inactive) Nihanth Subramanya [:nhnt11]]

Easiest way to test this is to open two sites with different category items (e.g. https://youtube.com and https://senglehardt.com/test/trackingprotection/test_pages/fingerprinting_and_cryptomining_and_cookies.html) and open the panel after switching between them.

On Nightly, you can see the items rearranging after the popup is shown. With the patch, they are reordered before the popup opens.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84744d179773
[Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Comment 4

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 84744d179773 (bug 1578459) for bc failures in browser_trackingUI_.

Backout: https://hg.mozilla.org/integration/autoland/rev/bcc99e56182d181ab662d7209ca7e5949caa9051

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=84744d179773fc5edef4dcbeafb6e4d8d382c405&selectedJob=265399517

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265399517&repo=autoland&lineNumber=16481

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

The failure was intermittent but high frequency (8/11 runs). The screenshot showed that the category item was marked .notFound, but was still disabled - i.e. reorderCategoryItems was never called. This means that the content blocking event was received between onpopupshowing and onpopupshown - since if it was called after the popup was shown, the items would have reordered immediately.

I made two try pushes: [1] in which the test waits for the category item order to be invalidated before opening the popup, and [2] that makes onContentBlockingEvent call reorderCategoryItems immediately in the showing state of the popup in addition to open.

If [2] works, we should land it. If not, I have high confidence that [1] will work.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f05f46b983e8e2ad511133f13c6d468b4c0564
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0278faa55d4844c05117bed487fec0db056015

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #5)

The failure was intermittent but high frequency (8/11 runs). The screenshot showed that the category item was marked .notFound, but was still disabled - i.e. reorderCategoryItems was never called. This means that the content blocking event was received between onpopupshowing and onpopupshown - since if it was called after the popup was shown, the items would have reordered immediately.

I made two try pushes: [1] in which the test waits for the category item order to be invalidated before opening the popup, and [2] that makes onContentBlockingEvent call reorderCategoryItems immediately in the showing state of the popup in addition to open.

If [2] works, we should land it. If not, I have high confidence that [1] will work.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f05f46b983e8e2ad511133f13c6d468b4c0564
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a0278faa55d4844c05117bed487fec0db056015

[1] Works when the timing issue happens, but fails when the timing is normal (i.e. test now fails on non-QR).
[2] Seems to work as expected, waiting for retriggers to confirm.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b21608129eeb
[Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Comment 8

[Raul Gurzau (:RaulG)]

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

Comment 9

[Ciprian Georgiu, Desktop QA]

Reproduced this bug using the steps from comment 2, on macOS 10.13 and Windows 10 x64. I did not observed the issue on Ubuntu 18.04 x64.

This is verified fixed on latest Nightly 71.0a1.

Comment 10

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 9090811 [details]
Bug 1578459 - [Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Beta/Release Uplift Approval Request

• User impact if declined: UI changes layout while visible, resulting in a visual jump/flicker.
• 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): Small change, polish, verified in Nightly a while ago and working fine.
• String changes made/needed:

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9090811 [details]
Bug 1578459 - [Protections Panel] Reorder category items in popupshowing instead of shown. r=johannh

Fix for Skyline feature, verified in Nightly. Let's take this for beta 13.

Comment 12

[Noemi Erli[:noemi_erli]]

Conflict while grafting:
merging browser/base/content/browser-siteProtections.js
merging browser/components/controlcenter/content/protectionsPanel.inc.xul
warning: conflicts while merging browser/base/content/browser-siteProtections.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

<<<<<<< local
this._categoryItemOrderInvalidated = true;
if (["showing", "open"].includes(this._protectionsPopup.state)) {
this.reorderCategoryItems();
}

if (anyDetected) {
  this.noTrackersDetectedDescription.hidden = true;
}

graft
// Check whether the user has added an exception for this site.
let hasException = ContentBlockingAllowList.includes(
gBrowser.selectedBrowser
);

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Noemi: For this conflict, we need to keep the local version of the hunk. I'm on PTO today but please let me know if you need me to make a new rebased patch for beta. I can do that in the morning. Not sure about the process - i.e. whether you can resolve it based on what I said or if you need a new patch from me.

Comment 14

[Noemi Erli[:noemi_erli]]

Nihanth, here's a better look at the conflict: https://pastebin.com/raw/QaYu5A52.
By keeping the local version you mean discard all the lines after ======= ?

Comment 15

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

https://hg.mozilla.org/releases/mozilla-beta/rev/06ddf8fc87f7

Comment 16

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

Backed out for failing browser-chrome's browser_trackingUI_trackers_subview.js:

https://hg.mozilla.org/releases/mozilla-beta/rev/31e07561bea62b5452de74decb006bb4e38e6e73

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&group_state=expanded&selectedJob=269743614&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Cretry%2Csuperseded&revision=06ddf8fc87f7d6edae3eed1101cb82c1f8f47409

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=269743614&repo=mozilla-beta
[task 2019-10-04T09:04:27.426Z] 09:04:27 INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Indicates whether the tracker was blocked or allowed -
[task 2019-10-04T09:04:27.426Z] 09:04:27 INFO - Received onSecurityChange event 1 of 1
[task 2019-10-04T09:04:27.427Z] 09:04:27 INFO - Testing trackers subview with TP enabled.
[task 2019-10-04T09:04:27.429Z] 09:04:27 INFO - Buffered messages logged at 09:03:01
[task 2019-10-04T09:04:27.430Z] 09:04:27 INFO - Console message: [JavaScript Warning: "The resource at “http://trackertest.org/” was blocked because content blocking is enabled." {file: "http://tracking.example.org/browser/browser/base/content/test/trackingUI/trackingPage.html" line: 0}]
[task 2019-10-04T09:04:27.431Z] 09:04:27 INFO - TEST-PASS | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | TP category item is visible -
[task 2019-10-04T09:04:27.432Z] 09:04:27 INFO - Buffered messages finished
[task 2019-10-04T09:04:27.433Z] 09:04:27 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/trackingUI/browser_trackingUI_trackers_subview.js | Test timed out -

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Rebased patch for beta

Confirmed locally that the old patch fails tests on my debug build and the new patch passes.

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/5a6c32dc7d49