Closed Bug 1246291 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Preferences, defect)

46 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 47
Tracking Status
e10s m9+ ---
firefox46 + fixed
firefox47 + fixed

People

(Reporter: victor, Assigned: mconley)

References

Details

(Keywords: regression, reproducible)

Attachments

(4 files, 2 obsolete files)

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
This problem occurs in which page?
Flags: needinfo?(victor)
Component: Untriaged → Preferences
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(victor)
tracking-e10s: --- → ?
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.
Depends on: 1055464
Summary: Firefox Developer Edition 46.0a2 (2016-02-05) about:preferences#advanced - Warn me when websites try to redirect or reload the page → [e10s] about:preferences#advanced - "Warn me when websites try to redirect or reload the page" doesn't allow redirects
Flags: needinfo?(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.
Assignee: nobody → mconley
Flags: needinfo?(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.
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.
Also note that the MXR case appears to have _never_ worked properly in Firefox, e10s or not.
Well, it never worked until you landed 1055464, after which the warning dialog does appear in a non-e10s window.
Partial win! \o/ :D
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/
Attachment #8719054 - Flags: review?(bzbarsky)
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/
Attachment #8719055 - Flags: review?(dtownsend)
I'm not going to get to this before Tuesday, sorry.
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...
Attachment #8719054 - Flags: review?(bzbarsky)
Tracking for 46+ since this sounds like a possible blocker for e10s.
Flags: needinfo?(mconley)
How does this bug relate to bug 1150311?
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.
Attachment #8719055 - Flags: review?(dtownsend)
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.
Attachment #8719054 - Attachment is obsolete: true
Attachment #8719055 - Attachment is obsolete: true
Clearing the needinfo, since I think we've got a way forward here.
Flags: needinfo?(mconley)
Comment on attachment 8720799 [details]
MozReview Request: Bug 1246291 - Only initialize RefreshBlocker if enabled. r?Mossop

https://reviewboard.mozilla.org/r/35477/#review32157
Attachment #8720799 - Flags: review?(dtownsend) → review+
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.
Attachment #8720800 - Flags: review?(dtownsend)
Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

https://reviewboard.mozilla.org/r/35481/#review32163
Attachment #8720801 - Flags: review?(dtownsend)
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 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/
Attachment #8720801 - Attachment description: MozReview Request: Bug 1246291 - Regression test. r?Mossop → MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop
Attachment #8720801 - Flags: review?(dtownsend)
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 on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

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

Excellent!
Attachment #8720801 - Flags: review?(dtownsend) → review+
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.
Attachment #8720800 - Flags: review?(dtownsend)
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
Attachment #8720800 - Flags: review?(dtownsend)
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
verified with today's latest Nightly build.  Now getting the expected results per comment 2
Status: RESOLVED → VERIFIED
Want to request uplift to aurora here?
Flags: needinfo?(mconley)
Indeed I do - requests inbound...
Flags: needinfo?(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.
Attachment #8720799 - Flags: approval-mozilla-aurora?
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.
Attachment #8720800 - Flags: approval-mozilla-aurora?
Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

See above.
Attachment #8720801 - Flags: approval-mozilla-aurora?
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
Attachment #8720799 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8720800 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8720801 [details]
MozReview Request: Bug 1246291 - Beef up RefreshBlocker regression tests. r?Mossop

More tests! Huzzah
Attachment #8720801 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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!
Flags: needinfo?(mconley)
[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
You need to log in before you can comment on or make changes to this bug.