1691202 - DoH CFR disappears immediately when triggered by opening a site that immediately displays a modal prompt

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P2)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

STR:

1. Launch Firefox with a fresh profile (I used beta 86, but this was brought to my attention by a user on Nightly 87)
2. Go to about:config and set doh-rollout.enabled=true (make sure you have a passing heuristics environment, i.e. no enterprise policies and no canary on the network)
3. Confirm that DoH is working by checking the value of doh-rollout.mode. It should be 2.
4. In a new tab, navigate to google.com or facebook.com

Expected results:
DoH CFR shows up and persists

Actual results:
DoH CFR pops up and immediately goes away

This STR is reproducible on my connection in Berlin, where Google and Facebook both display a modal cookie consent popup on the page. I could not reproduce this with pages that don't use that style of modal prompt, and out of the handful of sites I tried, these were the ones I found. YouTube seems to have a similar prompt, except it's about logging in, not cookies, but in any case the issue wasn't reproducible with YouTube for me.

There's something that's causing the popup to be dismissed; I haven't had time to try and track it down myself.

Note that the popup's actions are NOT triggered, so the popup still tries to show up again if I fast-forward my system clock by a day. I'm not yet sure how this plays with the lifetime frequency limitation of 3 that's set on the DoH CFR.

Andrei, is this something you have time to help with or get attention for? I might have time to debug this a little more soon, I will comment below if I find anything.

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

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

I'm not yet sure how this plays with the lifetime frequency limitation of 3 that's set on the DoH CFR.

Looks like it applies as usual - after the third time that the popup is shown it doesn't try to show up again.

Comment 2

[Andrei Oprea [:andreio]]

It doesn't seem surprising that another modal acquiring focus would dismiss it, we probably don't want 2 overlapping modals anyway. Is this worth fixing beyond maybe incrementing lifetime by 1?
My reasoning is that the DOH doorhanger doesn't request any action from the user it only presents information.
There's an additional parameter (visitsCount) that we could use to show the doorhanger on the "second, or higher, request to any domain" i.e. when they navigate to the 2nd+ page of the same domain during the same session. We assume they dismiss things on the 1st request and the second is then available for the DOH doorhanger (the 2 is configurable to any number).

Comment 3

[(inactive) Nihanth Subramanya [:nhnt11]]

(In reply to Andrei Oprea [:andreio] from comment #2)

It doesn't seem surprising that another modal acquiring focus would dismiss it, we probably don't want 2 overlapping modals anyway. Is this worth fixing beyond maybe incrementing lifetime by 1?

I think it is surprising, because the modal in question is modal to the web-content (and implemented within it, by the site) and not to the browser. Dynamics in the site should not cause browser-level popup notifications to go away. Based on something someone said in Slack, the Fission CFR might be subject to the same issue.

The fix for this might be in the Popup Notifications infrastructure or an even lower level, depending on what exactly is causing the early dismissal, so maybe the bug should live in a different component - but I think it's manifesting at the CFR level.

My reasoning is that the DOH doorhanger doesn't request any action from the user it only presents information.

That's not true - the DoH doorhanger presents an opportunity to the user to opt-out. Depending on which button they click, we take action, one of which is to disable DoH and prevent sending browsing data to a third-party.

There's an additional parameter (visitsCount) that we could use to show the doorhanger on the "second, or higher, request to any domain" i.e. when they navigate to the 2nd+ page of the same domain during the same session. We assume they dismiss things on the 1st request and the second is then available for the DOH doorhanger (the 2 is configurable to any number).

That's interesting, thanks for surfacing it! I think we may want to explore that in the near future (we've recently received some feedback from the global comment period) but I don't think it's relevant to this bug.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

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

(In reply to Andrei Oprea [:andreio] from comment #2)

There's an additional parameter (visitsCount) that we could use to show the doorhanger on the "second, or higher, request to any domain" i.e. when they navigate to the 2nd+ page of the same domain during the same session. We assume they dismiss things on the 1st request and the second is then available for the DOH doorhanger (the 2 is configurable to any number).

That's interesting, thanks for surfacing it! I think we may want to explore that in the near future (we've recently received some feedback from the global comment period) but I don't think it's relevant to this bug.

Ah, I'm sorry, I just read this again and understood what you're saying. I think I misread it the first time. This could work as a mitigation until the underlying issue is fixed... I wonder though about whether it might mean we never show the popup at all for some users, or whether there's a concern about the lookups that happen before we finally offer the opt out UI. I'll consult with the team and circle back.

Comment 5

[Andrei Oprea [:andreio]]

Do you think there's still something to do here?

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

I do. There are two decisions to be made here:

1. Do we need to do anything about the fact that web content seems to be able to dismiss browser-chrome panels?

This needs to be confirmed a bit more rigorously and assessed for security implications. Depending on the conclusion, we might need to fix something at a lower level.

Gijs, I'm unfortunately a bit swamped at the moment. Could you help find an owner to investigate whether this panel dismissal issue is "real"?

2. Do we need to do anything on the CFR level to mitigate the situation?

Tyler, can you help figure out a good workaround here? Tl;dr: the DoH consent popup seems to be dismissed automatically on sites that display some kind of modal prompt (e.g. cookie consent). Details in comment 0. Andrei suggests we can e.g. show the doorhanger on the second visit to a given site instead of the first visit, with the hope that the cookie-consent dialogs are dismissed on the first visit. But this has the implication that the popup can be delayed by arbitrarily long periods of browser-usage, and I'd say the opt-out should be offered as early as possible.

Comment 7

[:Gijs (away/busy until Mar 10)]

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

I do. There are two decisions to be made here:

1. Do we need to do anything about the fact that web content seems to be able to dismiss browser-chrome panels?

This needs to be confirmed a bit more rigorously and assessed for security implications. Depending on the conclusion, we might need to fix something at a lower level.

Gijs, I'm unfortunately a bit swamped at the moment. Could you help find an owner to investigate whether this panel dismissal issue is "real"?

302 Johann.

Comment 8

[Victoria Wang [:victoria]]

Attachment: Encrypted DNS Lookups CFR

I noticed this bug with the new "More secure, encrypted DNS lookups" dialogue that appears in release 89.0.2 with a new MacOS profile. When making a first google search, the panel shows up for less than a second and disappears.

This seems like a very visible (though brief) bug since every new user who starts with a google search will see it.

Comment 9

[:Gijs (away/busy until Mar 10)]

Neil, do you have a moment to figure out why the popup is dismissed? That is, I'm hoping that the callstack (from JS and/or C++) for the hidePopup call on the panel could tell us more about what our options are in terms of addressing this...

Comment 10

[Andrei Oprea [:andreio]]

In order to make sure this message shows up you need to set the following prefs:

doh-rollout.enabled=true
doh-rollout.disable-heuristics=false
doh-rollout.skipHeuristicsCheck=false
doh-rollout.doorhanger-decision=false

And then navigate to any website. Shows up in Firefox 89+.

Comment 11

[Neil Deakin]

hidePopup is called from the following place:

File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_dismiss Line: 876
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_update Line: 1447
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_remove Line: 777
File: resource://activity-stream/lib/CFRPageActions.jsm Name: _popupStateChange Line: 251
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_fireCallback Line: 1699
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_removeHelper Line: 845
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_remove/< Line: 767
File: resource://gre/modules/PopupNotifications.jsm Name: PopupNotifications_remove Line: 766
File: resource://activity-stream/lib/CFRPageActions.jsm Name: hideAddressBarNotifier Line: 187
File: resource://activity-stream/lib/CFRPageActions.jsm Name: updatePageActions Line: 884
File: chrome://browser/content/browser.js Name: onLocationChange Line: 5239
File: chrome://browser/content/tabbrowser.js Name: callListeners Line: 891
File: chrome://browser/content/tabbrowser.js Name: _callProgressListeners Line: 905
File: chrome://browser/content/tabbrowser.js Name: _callProgressListeners Line: 6167
File: chrome://browser/content/tabbrowser.js Name: onLocationChange Line: 6579

Comment 12

[Ed Lee :Mardak]

(In reply to Neil Deakin from comment #11)

hidePopup is called from the following place:
File: resource://activity-stream/lib/CFRPageActions.jsm Name: _popupStateChange Line: 251

Thanks. I added some logging for when the popup state changes at the beginning of this method. Then ./mach run --setpref doh-rollout.enabled=true and navigated to google

showing _popupStateChange@resource://activity-stream/lib/CFRPageActions.jsm:242:40
PopupNotifications_fireCallback@resource://gre/modules/PopupNotifications.jsm:1699:40
PopupNotifications_showPanel/notificationsToShow<@resource://gre/modules/PopupNotifications.jsm:1219:26
PopupNotifications_showPanel@resource://gre/modules/PopupNotifications.jsm:1214:47
PopupNotifications_update@resource://gre/modules/PopupNotifications.jsm:1428:14
PopupNotifications_show@resource://gre/modules/PopupNotifications.jsm:613:14
_renderPopup@resource://activity-stream/lib/CFRPageActions.jsm:749:63

shown _popupStateChange@resource://activity-stream/lib/CFRPageActions.jsm:242:40
PopupNotifications_fireCallback@resource://gre/modules/PopupNotifications.jsm:1699:40
PopupNotifications_showPanel/</this._popupshownListener/<@resource://gre/modules/PopupNotifications.jsm:1335:16
PopupNotifications_showPanel/</this._popupshownListener@resource://gre/modules/PopupNotifications.jsm:1334:29

And what we're probably interested in:

removed _popupStateChange@resource://activity-stream/lib/CFRPageActions.jsm:242:40
PopupNotifications_fireCallback@resource://gre/modules/PopupNotifications.jsm:1699:40
PopupNotifications_removeHelper@resource://gre/modules/PopupNotifications.jsm:845:10
PopupNotifications_remove/<@resource://gre/modules/PopupNotifications.jsm:767:12
PopupNotifications_remove@resource://gre/modules/PopupNotifications.jsm:766:23
hideAddressBarNotifier@resource://activity-stream/lib/CFRPageActions.jsm:187:38
updatePageActions@resource://activity-stream/lib/CFRPageActions.jsm:884:20
onLocationChange@chrome://browser/content/browser.js:5239:20
callListeners@chrome://browser/content/tabbrowser.js:891:31
_callProgressListeners@chrome://browser/content/tabbrowser.js:905:22
_callProgressListeners@chrome://browser/content/tabbrowser.js:6169:46
onLocationChange@chrome://browser/content/tabbrowser.js:6581:14

Looks like it's going through this recommendation.retain branch https://searchfox.org/mozilla-central/rev/1add3985d32470582ce83f40a212a976e4fc7da5/browser/components/newtab/lib/CFRPageActions.jsm#881-885

Comment 13

[Ed Lee :Mardak]

Perhaps the fix is to have CFR ignore location changes that are LOCATION_CHANGE_SAME_DOCUMENT? Looks like onLocationChange for google has flags initially 0 then a second time with flags = 1 (maybe from some pushState ?)

https://searchfox.org/mozilla-central/rev/1add3985d32470582ce83f40a212a976e4fc7da5/uriloader/base/nsIWebProgressListener.idl#423-429,454

There's even an existing isSameDocument in the same scope:
https://searchfox.org/mozilla-central/rev/1add3985d32470582ce83f40a212a976e4fc7da5/browser/base/content/browser.js#5154-5155,5239

Comment 14

[:Gijs (away/busy until Mar 10)]

(In reply to Ed Lee :Mardak from comment #13)

Perhaps the fix is to have CFR ignore location changes that are LOCATION_CHANGE_SAME_DOCUMENT? Looks like onLocationChange for google has flags initially 0 then a second time with flags = 1 (maybe from some pushState ?)

https://searchfox.org/mozilla-central/rev/1add3985d32470582ce83f40a212a976e4fc7da5/uriloader/base/nsIWebProgressListener.idl#423-429,454

There's even an existing isSameDocument in the same scope:
https://searchfox.org/mozilla-central/rev/1add3985d32470582ce83f40a212a976e4fc7da5/browser/base/content/browser.js#5154-5155,5239

Yes, I think this is right (ie not calling updatePageActions for isSameDocument calls to onLocationChange).

It used to be that youtube navigated between videos this way if you clicked from one page to another, but I expect that CFR recommendations are normally per-origin, rather than dependent on URL paths/querys/refs?

Comment 15

[Andrei Oprea [:andreio]]

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

It used to be that youtube navigated between videos this way if you clicked from one page to another, but I expect that CFR recommendations are normally per-origin, rather than dependent on URL paths/querys/refs?

Will this break a scenario where user navigates to example.com clicks some internal link and ends up on example.com/foo and we actually want to target example.com/foo? (this has come up in the past)

Comment 16

[:Gijs (away/busy until Mar 10)]

(In reply to Andrei Oprea [:andreio] from comment #15)

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

It used to be that youtube navigated between videos this way if you clicked from one page to another, but I expect that CFR recommendations are normally per-origin, rather than dependent on URL paths/querys/refs?

Will this break a scenario where user navigates to example.com clicks some internal link and ends up on example.com/foo and we actually want to target example.com/foo? (this has come up in the past)

So... not normally, but it would if example.com uses history.pushState for "loading" example.com/foo and keeps the same document, rather than actually navigating from one page to another (and thus loading a new document). I just checked, and at least for me in a private window right now, youtube does still use pushState when going from one video to the next (you can check relatively easily by adding a random property on document in the web console, and then clicking a different video - the URL in the address bar changes, but the extra property on document is still there).

If the path component is important, another option you have is caching the last URI you see in onLocationChange, and checking uri.equalsExceptRef with the new URI, which would ignore a hash/ref change but not a path change, or you could add custom logic to also ignore changes to the URI's query component if you don't care about that...

Comment 17

[Tyler Duzan]

I apologize that my response to the above question was delayed so long. I had some conversations with various folks via Slack regarding this and I think mostly addressed it there, but to clarify here as well I think that it's important that users are provided an opportunity to opt out of DoH as early as possible in order to meet our intended commitment to user consent in our DoH rollouts.

Comment 18

[Johann Hofmann [:johannh]]

I don't think my needinfo is still needed here :)

Comment 19

[:Gijs (away/busy until Mar 10)]

Attachment: Bug 1691202 - do not dismiss persistent CFR popups when navigating, r?Mardak

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60630339d316
do not dismiss persistent CFR popups when navigating, r=Mardak

Comment 21

[Ed Lee :Mardak]

Comment on attachment 9267669 [details]
Bug 1691202 - do not dismiss persistent CFR popups when navigating, r?Mardak

Beta/Release Uplift Approval Request

• User impact if declined: Some redirected web pages, e.g., Google search results, trigger the DOH rollout message to be shown then immediately hidden
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: 1. Enable doh-rollout.enabled

2. Google search from address bar

• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small condition change to skip existing message-hiding behavior for messages that should be persisted
• String changes made/needed: none

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/735609cc0318
do not dismiss persistent CFR popups when navigating, r=Mardak

Comment 23

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/735609cc0318

Comment 24

[Donal Meehan [:dmeehan]]

Comment on attachment 9267669 [details]
Bug 1691202 - do not dismiss persistent CFR popups when navigating, r?Mardak

Approved for 99.0b4. Thanks.

Comment 25

[Donal Meehan [:dmeehan]]

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

Comment 26

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/60630339d316

Comment 27

[Patriciu Pop [:ppop], Ecosystem QA]

I've verified that the issue is no longer reproducible and the doorhanger is triggered and remains displayed until user action, even on pages with pop-ups. Tested using Firefox Nightly 100.0a1 Build ID 20220315091352 on Windows 10 x64, macOS 11.6.3, and Ubuntu 20 x64.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9267669 [details]
Bug 1691202 - do not dismiss persistent CFR popups when navigating, r?Mardak

Approved for 98.0.2.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/0282a59dcbbeb393c1efd38000be91a97a9cbb7e

Comment 30

[Brian Grinstead [:bgrins]]

Patriciu, could you please test this change on the latest beta (4) and also directly off the builds from mozilla-release: https://treeherder.mozilla.org/jobs?repo=mozilla-release&revision=0282a59dcbbeb393c1efd38000be91a97a9cbb7e

Comment 31

[Carmen Fat [:cfat] - Ecosystem QA]

I have verified this issue on the latest Beta 99.0b4 (Build ID: 20220315185755) and also on the 98.0.2 try-build (Build ID: 20220315213455) on Windows 10 x64, macOS 10.15.7, and Linux Ubuntu 20.04 x64.

• The DoH doorhanger is triggered and remains displayed until user action after:

1. performing a search in the Address Bar.
2. displayed on pages with pop-ups.