1478348 - Doorhanger remains displayed after navigating back to new tab

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P2)

Description

[Peter Magyari (Desktop QA)]

[Environment]
Windows 10 64-bit
Firefox Nightly 63.0a1 Build ID 20180724223402

[Steps]
1.Open a new tab
2.Navigate to a link which triggers the block autoplay doorhanger.
  (ex. https://www.youtube.com/watch?v=FlsCjmMhFmw) 
3.Click on the "back" navigation button.

[Expected Result]
When you navigate "back" the doorhanger should not longer be displayed.

[Actual Result]
The doorhanger remains displayed on the new tab.

Comment 1

[Alastor Wu [:alwu]]

I can't reproduce this issue on Nightly 63.0a1 (2018-07-25). After clicked "back", the doorhanger would disappear.

https://drive.google.com/file/d/1LzAtknuOPaAZRzH8afU9mqokzv808nEV/view?usp=sharing

Comment 2

[Peter Magyari (Desktop QA)]

Hello,

I can see from the video that you've attached that you have opened the link in the same tab, and the issue is only reproducible if you open it in a whole new tab.

Comment 3

[Alastor Wu [:alwu]]

(In reply to Peter from comment #2)
> Hello,
> 
> I can see from the video that you've attached that you have opened the link
> in the same tab, and the issue is only reproducible if you open it in a
> whole new tab.

oh! yes, I can reproduce this issue now, thank you!

Comment 4

[Alastor Wu [:alwu]]

This bug is not autoplay bug which also happens on other permission, eg. geolocation.

Comment 5

[Johann Hofmann [:johannh]]

Oh, that's not great. I can reproduce this in release, how come nobody has run into this yet?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Johannh: We feel this should block shipping block autoplay, are you able to look into this?

Comment 7

[Pascal Chevrel:pascalc (PTO until April 26)]

Tracking for 63 because of comment #6

Comment 8

[Johann Hofmann [:johannh]]

I'm busy with some high priority work and won't be able to commit to fixing this. Prathiksha, I know you were looking for something to work on, do you think you'd be able to investigate into this some time this month?

Comment 9

[:prathiksha]

(In reply to Johann Hofmann [:johannh] from comment #8)
> I'm busy with some high priority work and won't be able to commit to fixing
> this. Prathiksha, I know you were looking for something to work on, do you
> think you'd be able to investigate into this some time this month?

yeah, I can do that. :)

Comment 10

[Dale Harvey (:daleharvey)]

Prathiksha are you looking into this? we probably want to be making sure everything lands before merge day (23rd) so preferably in for review today or monday. I can take this if you want

Comment 11

[:prathiksha]

(In reply to Dale Harvey (:daleharvey) from comment #10)
> Prathiksha are you looking into this? we probably want to be making sure
> everything lands before merge day (23rd) so preferably in for review today
> or monday. I can take this if you want

Hi Dale. I don't have a patch for this yet. Feel free to take it.

Comment 12

[Dale Harvey (:daleharvey)]

This has been broken since February and was broken by https://github.com/mozilla/gecko-dev/commit/cd6c4f5d918fec7cded609f8a00022a8d40b38e2, which no longer exists in the tree

If I change https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#570 to |this.remove(notification)| it fixes the bug but not sure if thats the correct fix, seems like it might not already do that for a reason however its not clear to me that this._update (which we call without 'dismissShowing') is supposed to do it either

Comment 13

[Dale Harvey (:daleharvey)]

Johannh should I submit a patch for .remove() or is something else up here? cheers

Comment 14

[Dale Harvey (:daleharvey)]

ok my debugging was somehow wrong earlier, this.remove doesnt fix it, in browser.js we call |UpdatePopupNotificationsVisibility| [1] when navigating back to about:newtab, this is done before PopupNotifications.LocationChange is called (which filters out the notification), so it believes there is a notification to show and calls _showPanel, showPanel does some async stuff, inside that loop LocationChange gets called, filters the notification and hides the panel, only for showPanel to then resume its work and show the panel again [2]

 [1] - https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2843
 [2] - https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/PopupNotifications.jsm#1073

I think to fix we want to avoid anchorVisibilityChange getting called twice from locationChange events, probably by taking it out of the browser.js code path, but will see what effect that has

Comment 15

[Dale Harvey (:daleharvey)]

So one fix I was considering for this was to pass through the fact it was a location change and not calling into popupnotifications if so, however it looks like the code actually used to a similiar thing and was removed in https://bugzilla.mozilla.org/show_bug.cgi?id=1328304

Got a test for this and various not ok fixes, Johann do you have a suggestion for how to cleanly fix this?

Comment 16

[Johann Hofmann [:johannh]]

(In reply to Dale Harvey (:daleharvey) from comment #15)
> So one fix I was considering for this was to pass through the fact it was a
> location change and not calling into popupnotifications if so, however it
> looks like the code actually used to a similiar thing and was removed in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1328304
> 
> Got a test for this and various not ok fixes, Johann do you have a
> suggestion for how to cleanly fix this?

So, the special thing with about:newtab here seems to be that we change both the "proxyState" and the location (so both SetPageProxyState and onLocationChange are called and conflict with each other).

Is this happening regularly anywhere else? If not, we could consider special-casing about:newtab in one of the two code paths (wherever it makes most sense) to avoid this conflict. That seems like the best way forward since disentangling the whole ting seems like a hard task and like with bug 1328304 we may end up with 5 regressions for one fix.

What do you think?

Comment 17

[Dale Harvey (:daleharvey)]

We call both on every page load, it breaks with about:newtab because we specifically consider blank pages [1] (but not moz-extension pages?) invalid, the call to show the doorhanger only happens when the proxy state changes [2] 

 [1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2678
 [2] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2822

I think the quick fix would be to pass a flag via |URLBarSetURI| to not call |UpdatePopupNotificationsVisibility| on LocationChange, but in https://bugzilla.mozilla.org/show_bug.cgi?id=1328304#c20 you seemed pretty keen on it being removed

Another fix would be to just set valid=true instead of the check in [1], however that means the (i) button will persist on about:newtab pages and we dont want that, we could use a seperate method to show/hide the (i) button icon?

I cant think of any clean way to just special case about:newtab, we need |SetPageProxyState| to be called onLocationChange but not |UpdatePopupNotificationsVisibility| whether they are about pages or not, the only way to do that is for |SetPageProxyState| to know whether its in a LocationChange event

Comment 18

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Actually this seems pretty clean, while having about:newtab set to invalid seems confusing to me, this makes sure we dont try to change the popup visibility on browser.js LocationChange

Comment 19

[Dale Harvey (:daleharvey)]

Comment on attachment 9004367 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Found some complications here after a try run, reworking this

Comment 20

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

You were right about this being fiddly, every fix for this seems to break another test somewhere. 

So to clarify the problem, |PopupNotifications.anchorVisibilityChange()| is called twice on page loads where the proxy state changes state, once because
 TabsProgressListener.onLocationChange => PopupNotifications.LocationChange
and the other because
 XULBrowserWindow.onLocationChange => URLBarSetURI => SetPageProxyState

Now a fix seemed to be that SetPageProxyState state doesnt need to hide the popup, only when the user is typing so we can do the updatepopupvisibility on updatepageproxystate only, however that broke tests when we closed or opened tabs because TabsProgressListener.onLocationChange doesnt get called when opening or closing tabs, only navigations, so figured we could move popupnotification.onlocation change to xulbrowserwindow however that breaks other tests

Will carry on looking into why these tests are breaking (some I found were genuine, like missing the case when the user presses esc), but wouldnt mind some input into what is the best approach here

Comment 21

[Dale Harvey (:daleharvey)]

Attachment: 0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Ok finally got something that looks to be passing all tests, apologies for the delay.

I asked about this approach before but was hoping for a cleaner way however after a few experiments I think this is the cleanest its going to get. We tell onLocationChange that it is being triggered by a tab switch and only on tab switches we call popupnotifications, otherwise it gets handled by tabsprogresslistener.onLocationChange

The unrelated test that need changed looked to be based on broken behaviour as it was waiting for a notification close after already navigating to a new origin and confirming the notification had been closed

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 9006838 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Review of attachment 9006838 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like an acceptable solution to me. I don't think we can come up with anything less hacky in any reasonable amount of time and this actually looks pretty clean given the complexity of the issue.

Thank you for all your work and persistence on this bug.

::: browser/base/content/browser.js
@@ +2653,4 @@
>   *
>   * @param aURI [optional]
>   *        nsIURI to set. If this is unspecified, the current URI will be used.
> + * @param updatePopup [optional]

nit: please call this updatePopupNotifications

@@ +2814,4 @@
>   *        related user interface elments should be shown because the URI in the
>   *        location bar matches the loaded page. The string "invalid" indicates
>   *        that the URI in the location bar is different than the loaded page.
> + * @param updatePopup

updatePopupNotifications

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d163fc054fdc
Dont show doorhanger on about:newtab. r=johannh

Comment 24

[Daniel Varga [:dvarga]]

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

Comment 25

[Pascal Chevrel:pascalc (PTO until April 26)]

Johann, do you think that this is safe to uplift to beta or should we let it ride the trains? Thanks

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

My question in comment#25 was actually for Dale, fixing it.

Comment 27

[Johann Hofmann [:johannh]]

FWIW, I think this is a medium risk patch but certainly within the acceptable range for mid-beta. I would also defer to Dale on this decision :)

Comment 28

[Dale Harvey (:daleharvey)]

Comment on attachment 9006838 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Yeh I think its probably worth an uplift here, will request

Approval Request Comment
[Feature/Bug causing the regression]: #1417138
[User impact if declined]: Doorhanger being shown in confusing place
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Nope
[Needs manual test from QE? If yes, steps to reproduce]: open about:home, type permission.site in url bar, request a permission that shows doorhanger, click back, doohanger should hide 
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Medium Risk
[Why is the change risky/not risky?]: Hard to see how the patch could break other code but there are a few code paths here
[String changes made/needed]: n/a

Comment 29

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9006838 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

Approved for 63 beta 7, thanks.

Comment 30

[Peter Magyari (Desktop QA)]

Verified as fixed on Windows 10, Mac Os 10.12, Ubuntu 18.04 using the latest Firefox Nightly 64.0a1 (20180916220033)

Comment 31

[Natalia Csoregi [:nataliaCs]]

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

Comment 32

[Peter Magyari (Desktop QA)]

Verified as fixed on Win 10, Mac Os 10.12 and Ubuntu 18.04 using the latest Firefox Beta 63.0b7(20180917143811)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting to ESR60 also?

Comment 34

[Dale Harvey (:daleharvey)]

Sorry forgot to actually post my reply.

I dont have a lot of experience with ESR uplifts so I dont know where the bar is at, I think as long as this applies it should be relatively safe. However its also a bug that went (quite surprisingly) unnoticed for a few months and doesnt cause anything particularly serious to happen.

Comment 35

[Johann Hofmann [:johannh]]

Yeah, seems fine for ESR60 to me.

Comment 36

[:Gijs (he/him)]

(In reply to Dale Harvey (:daleharvey) from comment #34)
> Sorry forgot to actually post my reply.
> 
> I dont have a lot of experience with ESR uplifts so I dont know where the
> bar is at, I think as long as this applies it should be relatively safe.
> However its also a bug that went (quite surprisingly) unnoticed for a few
> months and doesnt cause anything particularly serious to happen.

Dale, can you request ESR approval then, given comment #35? Thanks!

(Normally either patch author (preferred) or reviewer does uplift requests, to ensure people don't request uplift without context.)

Comment 37

[Dale Harvey (:daleharvey)]

Comment on attachment 9006838 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: Quite visible / low risk bug

User impact if declined: Permission prompts show in the wrong place

Fix Landed on Version: beta

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Covered by tests, qa'd and manually verified

String or UUID changes made by this patch: N/A

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9006838 [details] [diff] [review]
0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch

This is going to need a bit of rebasing for ESR60 (mostly with the new test from what I can see). Please attach a rebased patch and re-request approval.

Comment 39

[Dale Harvey (:daleharvey)]

If it needs rebasing then I do not think its worth it, especially since the block autoplay itself will certainly not be part of esr

Comment 40

[Ryan VanderMeulen [:RyanVM]]

WFM, thanks.