1246291 - [e10s] about:preferences#advanced - "Warn me when websites try to redirect or reload the page" doesn't allow redirects

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Victor Ndambakuwa]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.103 Safari/537.36

Steps to reproduce:

about:preferences#advanced - Warn me when websites try to redirect or reload the page


Actual results:

Redirect


Expected results:

No Redirect

Comment 1

[YF (Yang)]

This problem occurs in which page?

Comment 2

[Tracy Walker [:tracy]]

I see this on Nightly. It is a recent regression. I'll track it down shortly

STR:

1) ensure the Pref/option for Advanced | General | "Warn me when websites try to redirect or reload the page" is selected
2) load http://mxr.mozilla.org/mozilla-central/ and enter A Search for topic

actual results:

one) there is no redirect warning bar
two) the page ends up loading http://mxr.mozilla.org/mozilla-central/find?text=EncodingRunnable&string= which is a blank page, no results.

expected results (works in non-e10s window): 

one) a redirecting warning bar appears at the top of the page with option to "Allow."  click the Allow button.
then
two) http://mxr.mozilla.org/mozilla-central/search?string=EncodingRunnable is loaded with mxr search results

seen on:
Windows 7, Nightly 47.0a1, 20160212030242 - Fail
Mac 10.11, Nightly 47.0a1, 20160212030242 - Fail
Ubuntu 14.04, Nightly 47.0a1, 20160212030242 - Fail

Comment 3

[Tracy Walker [:tracy]]

mozregression found https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=c882b40728836e5b3a7194f2e7bda4cf61e22f9f&tochange=cb036027df84e7e547bf3f9614b71857e5059741

pointing to bug 1055464

I thought we already had a bug related to this pref on file.  Anyway, that bug isn't actually fixed as there still is no warning dialog in e10s.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Something really weird is going on here.

The core problem is that the notification bar that gives the user the choice to allow the refresh is not displaying.

The weird thing is that we're doing everything right in order to show that notification bar.

It works for pages like: https://bug1150311.bmoattachments.org/attachment.cgi?id=8590090

But not for the mxr page. Very odd. Looking into it now.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Ah, okay - we're removing the notification immediately after its added using this stack:

 removeTransientNotifications@chrome://global/content/bindings/notification.xml:252:60
TabsProgressListener.onLocationChange@chrome://browser/content/browser.js:4698:5
callListeners@chrome://browser/content/tabbrowser.xml:501:24
_callProgressListeners@chrome://browser/content/tabbrowser.xml:522:13
mTabProgressListener/<._callProgressListeners@chrome://browser/content/tabbrowser.xml:571:22
mTabProgressListener/<.onLocationChange@chrome://browser/content/tabbrowser.xml:816:17
RemoteWebProgressManager.prototype._callProgressListeners@resource://gre/modules/RemoteWebProgress.jsm:174:11
RemoteWebProgressManager.prototype.receiveMessage@resource://gre/modules/RemoteWebProgress.jsm:242:7

So we've got an ordering problem for this particular type of page refresh.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Okay, so some new data.

The refresh that MXR is doing is via a HTTP response header that looks like this (for a search of "test"):

Connection: Keep-Alive
Content-Length: 0
Content-Type: text/html; charset=UTF-8
Date: Fri, 12 Feb 2016 21:19:19 GMT
Expires: Fri, 12 Feb 2016 21:39:19 GMT
Keep-Alive: timeout=5, max=1000
Last-Modified: Fri, 12 Feb 2016 20:00:03 GMT
Link: <http://mxr.mozilla.org/mozilla-central>; rel="Index"; title="mozilla-central", <http://mxr.mozilla.org/mozilla-central/ident>; rel="Glossary"; title="Identifier search", <http://mxr.mozilla.org/mozilla-central/search>; rel="Search"; title="Text search", <http://mxr.mozilla.org/mozilla-central/find>; rel="Contents"; title="Find file", <http://mxr.mozilla.org/mozilla-central/>; rel="Up"; title="Parent"
Server: Apache
Set-Cookie: colorwithjs=; path=/; expires= Sat, 01-Jan-2000 00:00:00 GMT
X-Backend-Server: mxr-web1.webapp.scl3.mozilla.com
X-Cache-Info: caching
refresh: 0; url=../mozilla-central/search?string=test

Here's the order of nsIWebProgressListener events in that case:

onStateChange
onStatusChange
onRefreshAttempted
onLocationChange
onSecurityChange
onProgressChange
onStateChange

Note that the onLocationChange occurs _after_ the onRefreshAttempted.

Now, for the case in https://bug1150311.bmoattachments.org/attachment.cgi?id=8590090, where a meta tag is used to redirect the page, we get a slightly different ordering:

onStateChange
onStatusChange
onLocationChange
onSecurityChange
onSecurityChange
onRefreshAttempted
onStatusChange
onProgressChange
onStateChange

Because onRefreshAttempted is being fired _after_ onLocationChange, the code in browser.js doesn't end up removing the refresh blocked notification.

So there's the bug. Investigating solutions now.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Also note that the MXR case appears to have _never_ worked properly in Firefox, e10s or not.

Comment 8

[Tracy Walker [:tracy]]

Well, it never worked until you landed 1055464, after which the warning dialog does appear in a non-e10s window.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Partial win! \o/ :D

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246291 - Add property to nsIWebProgress to indicate that a refresh was blocked. r?bz

This property is useful for cases where refreshes are caused by
the "refresh" key in an HTTP response header. When that occurs,
onRefreshAttempted for nsIWebProgressListener's are fired before
onLocationChange. For meta-tag refreshes, however, onRefreshAttempted
is fired _after_ onLocationChange.

So this allows us to detect refreshes having been blocked in the
first case for onLocationChange handlers.

Review commit: https://reviewboard.mozilla.org/r/34851/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34851/

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246291 - Don't clear transient notifications if a refresh was blocked early. r?Mossop

A refresh can occur when reading a "refresh" key from the HTTP
response headers, which will result in onRefreshAttempted firing
before onLocationChange. If, however, a refresh is caused by a
meta tag, this will fire after onLocationChange.

We can now detect the former with a refreshWasBlocked property
on the nsIWebProgress, so that the onLocationChange handler in
browser.js can filter out the HTTP header cases, where
onRefreshAttempted is fired earlier.

Review commit: https://reviewboard.mozilla.org/r/34853/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34853/

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm not going to get to this before Tuesday, sorry.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8719054 [details]
MozReview Request: Bug 1246291 - Add property to nsIWebProgress to indicate that a refresh was blocked. r?bz

https://reviewboard.mozilla.org/r/34851/#review31719

::: uriloader/base/nsDocLoader.cpp:1337
(Diff revision 1)
> +  mRefreshWasBlocked = !allowRefresh;

So this will set the boolean on every docloader starting at the one that wants to refresh and going up to the one that the nsIWebProgressListener that blocks the refresh is on, right?

In other words, the boolean really means "refresh of me or some descendant was blocked".  But we only reset it when "me" navigates.  This seems like a fairly odd setup.  I can see how it might work in some special cases (e.g. the ones where this boolean is only queried on the docloader that the nsIWebProgressListener that blocks refresh is on), but if that's the case maybe we should only set the boolean if we're the docloader with the relevant nsIWebProgressListener?

In any case, the IDL needs to clearly document what the boolean _actually_ means, whatever that is.  As things stand, I can't tell whether the consumer introduced in the other patch is correct or not...

Comment 14

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

Tracking for 46+ since this sounds like a possible blocker for e10s.

Comment 15

[Tracy Walker [:tracy]]

How does this bug relate to bug 1150311?

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 8719055 [details]
MozReview Request: Bug 1246291 - Don't clear transient notifications if a refresh was blocked early. r?Mossop

https://reviewboard.mozilla.org/r/34853/#review31929

It's not entirely clear to me what is going on here, some more explanation would be helpful. But I'd also need to see a test before reviewing as we obviously don't have one for this case.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7b5afc401d0

Comment 18

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/34851/#review31719

> So this will set the boolean on every docloader starting at the one that wants to refresh and going up to the one that the nsIWebProgressListener that blocks the refresh is on, right?
> 
> In other words, the boolean really means "refresh of me or some descendant was blocked".  But we only reset it when "me" navigates.  This seems like a fairly odd setup.  I can see how it might work in some special cases (e.g. the ones where this boolean is only queried on the docloader that the nsIWebProgressListener that blocks refresh is on), but if that's the case maybe we should only set the boolean if we're the docloader with the relevant nsIWebProgressListener?
> 
> In any case, the IDL needs to clearly document what the boolean _actually_ means, whatever that is.  As things stand, I can't tell whether the consumer introduced in the other patch is correct or not...

Turns out this approach is no good either, since it opens us up to not clearing transient notifications when the page we're transitioning to uses an HTTP refresh header.

I have another idea - I'll post patches soon.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246291 - Only initialize RefreshBlocker if enabled. r?Mossop

Review commit: https://reviewboard.mozilla.org/r/35477/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35477/

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

Review commit: https://reviewboard.mozilla.org/r/35479/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35479/

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

Review commit: https://reviewboard.mozilla.org/r/35481/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35481/

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Clearing the needinfo, since I think we've got a way forward here.

Comment 23

[Dave Townsend [:mossop]]

Comment on attachment 8720799 [details]
MozReview Request: Bug 1246291 - Only initialize RefreshBlocker if enabled. r?Mossop

https://reviewboard.mozilla.org/r/35477/#review32157

Comment 24

[Dave Townsend [:mossop]]

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

https://reviewboard.mozilla.org/r/35479/#review32161

::: browser/base/content/tab-content.js:788
(Diff revision 1)
> +    }

It's a little unclear to me whether onRefreshAttempted is called before or after the refresh delay. If it is after then this will probably break things. I don't think that is the case but I'd like to see tests of both header and actual <meta> refreshes that have a short delay.

Comment 25

[Dave Townsend [:mossop]]

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

https://reviewboard.mozilla.org/r/35481/#review32163

Comment 26

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/35479/#review32161

> It's a little unclear to me whether onRefreshAttempted is called before or after the refresh delay. If it is after then this will probably break things. I don't think that is the case but I'd like to see tests of both header and actual <meta> refreshes that have a short delay.

Good call. I'll add more tests.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35481/diff/1-2/

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35481/diff/2-3/

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

https://reviewboard.mozilla.org/r/35481/#review32295

Excellent!

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

No idea why MozReview didn't flag you for this one. Hrm.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

https://reviewboard.mozilla.org/r/35479/#review32305

Comment 32

[Dave Townsend [:mossop]]

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

https://reviewboard.mozilla.org/r/35479/#review32307

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ceae4f61c09
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d03291f8fb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/142c9145085f

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8ceae4f61c09
https://hg.mozilla.org/mozilla-central/rev/7d03291f8fb4
https://hg.mozilla.org/mozilla-central/rev/142c9145085f

Comment 35

[Tracy Walker [:tracy]]

verified with today's latest Nightly build.  Now getting the expected results per comment 2

Comment 36

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

Want to request uplift to aurora here?

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Indeed I do - requests inbound...

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720799 [details]
MozReview Request: Bug 1246291 - Only initialize RefreshBlocker if enabled. r?Mossop

Approval Request Comment
[Feature/regressing bug #]:

The refresh blocker - exposed under about:preferences in Advanced > General.

[User impact if declined]:

Users who have automatic refresh / reloads disabled will be unable to let a refresh proceed if it was initiated via an HTTP header, if they have e10s enabled.

[Describe test coverage new/current, TreeHerder]:

Automated tests have been added for this case, and the other case (<meta> tag case).

[Risks and why]: 

Very low risk. The good news is that this feature has actually been broken for probably years and we're finally fixing it.

[String/UUID change made/needed]:

None.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

See above.

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

See above.

Comment 41

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

Comment on attachment 8720799 [details]
MozReview Request: Bug 1246291 - Only initialize RefreshBlocker if enabled. r?Mossop

New tests added, fixes a long-standing issue. We also need this for e10s release in 46. Please uplift to aurora

Comment 42

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

Comment on attachment 8720800 [details]
MozReview Request: Bug 1246291 - Only send RefreshBlocked message to the parent once onLocationChange and onRefreshAttempted have both fired. r?Mossop

Please uplift, needed for e10s

Comment 43

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

Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

More tests! Huzzah

Comment 44

[Carsten Book [:Tomcat]]

has problems to uplift to aurora

merging browser/base/content/tab-content.js
warning: conflicts while merging browser/base/content/tab-content.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue

could you take a look, thanks!

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Ah, just had to land the (a+'d) dependency first:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2655e53aa68f
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/fe4e8aa50b3c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ed49273accc3

Comment 46

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Yes

Actual Results: 
As expected

Comment 47

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1246291.html