[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.

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

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.

(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!

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

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

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

Tracking for 63 because of comment #6

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?

(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. :)

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

(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.

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

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

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

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?

(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?

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

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 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

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

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 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

Pushed by dharvey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d163fc054fdc Dont show doorhanger on about:newtab. r=johannh

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

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

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 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 on attachment 9006838 [details] [diff] [review] 0001-Bug-1478348-Dont-show-doorhanger-on-about-newtab.-r-.patch Approved for 63 beta 7, thanks.

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

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

Is this something we should consider backporting to ESR60 also?

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.

Yeah, seems fine for ESR60 to me.

(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 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 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.

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

WFM, thanks.