"Connection was reset" errors visiting BBC news site via redirection from old URL

RESOLVED FIXED in Firefox 46

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: smichaud, Assigned: dragana)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

I started seeing these errors in the last few days, and I suspect they're related to a recent change or changes in the BBC news site.  But I don't see them in other browsers (e.g. Chrome or Safari).  And, though they're normally intermittent, I have STR that are 100% effective on both OS X and Windows.  So I think it's probably worth investigating further.

Whatever is going here (whether Firefox's behavior is a bug or a feature), it goes back a long way.  On OS X my STR "work" (you see the error) back at least to FF 15.  (On Windows I only tested with FF 34 betas 1 through 4 and FF 43.0.1.)  I tested on OS X 10.8.5 and Windows 7 (32 bit).

STR:

1) Run Firefox with a new profile (say in the Profile Manager).
2) Visit http://news.bbc.co.uk/ and see the "connection was reset" error.

If you click the "try again" button the connection will succeed.

Currently, whenever you visit http://news.bbc.co.uk/ you're redirected to http://www.bbc.com/news.  You don't see the error if you visit the latter URL in step 2.  So I suspect this bug is triggered by redirection.  And, since the bug is normally intermittent, I also suspect there's some kind of timing issue.

http://news.bbc.co.uk/ was for many years the canonical URL for the BBC news website.  I don't know when it changed to http://www.bbc.com/news, but I suspect it was very recently (a few days ago).
(Assignee)

Comment 1

2 years ago
I have try this on linux. Sometime the connection is reset and some time we get a response (a redirect).

Chrome sometimes gate the same reset and sometimes not. The only difference the it seams that chrome sometimes refreshes the page (the reset page) and then it gets the correct one.

Nothing we can do about a reset connection.

Comment 2

2 years ago
I'm getting this error on other browsers. At least Chrome and Edge on different computers. The other does not have antivirus protection other than Microsoft built in (essentials) and the other one has Kaspersky. It's highly likely that the issue is in part with my freeBSD based pfsense firewall as remoting to another location that is using openvpn and a completely different ISP is working perfectly fine.
Created attachment 8703627 [details]
bbc.pcap

pcap.. stream 0 has a rst with 0 response data bytes.. stream 1 is a manual reload that worked. no difference on request side
based on stream 0 of the capture, its tempting to mark this INVALID.

however, if this transaction happened to be done on a reused persistent connection we would retry it automatically due to the inherent race condition there. It seems doing one retry here would also make us more robust and its not opening any new replay scenarios that we don't already have.

dragana, do you want to take a shot at that?
(Assignee)

Comment 5

2 years ago
taking it.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
fwiw I also filled out the "report a problem with our website" link on the bbc,
(Assignee)

Comment 7

2 years ago
Created attachment 8704095 [details] [diff] [review]
bug_1236277.patch

Is this correct place to do this. The bbc site has been fixed so I cannot reproduce the rst any more.
Attachment #8704095 - Flags: feedback?(mcmanus)
Comment on attachment 8704095 [details] [diff] [review]
bug_1236277.patch

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +973,5 @@
>          // a TLS session (perhaps via a tunnel) is setup.
>          bool reallySentData =
>              mSentData && (!mConnection || mConnection->BytesWritten());
>  
> +        if (!mReceivedData) {

I think maybe this should be for idempotent methods only .. what do you think of?

if (!mReceivedData &&
    ((mRequestHeader && mRequestHeader->IsSafeMethod()) || 
     (SentData && (!mConnection || mConnection->BytesWritten())))
Attachment #8704095 - Flags: feedback?(mcmanus)
(Assignee)

Comment 9

2 years ago
Created attachment 8705135 [details] [diff] [review]
bug_1236277.patch
Attachment #8704095 - Attachment is obsolete: true
Attachment #8705135 - Flags: review?(mcmanus)
Comment on attachment 8705135 [details] [diff] [review]
bug_1236277.patch

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

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +933,5 @@
>      // should) assume that we wrote to a stale connection and we must therefore
>      // repeat the request over a new connection.
>      //
> +    // We have decided to retry not only in case of the reused connections, but
> +    // all of them(bug 1236277).

update this.. all safe methods maybe...

can you also lower the restart attempts limit to 3? Its absurdly high right now.
Attachment #8705135 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8705521 [details] [diff] [review]
bug_1236277.patch
Attachment #8705135 - Attachment is obsolete: true
Attachment #8705521 - Flags: review+
(Assignee)

Comment 12

2 years ago
Actually we do not have that pref at all, network.http.request.max-attempts does not exist

http://mxr.mozilla.org/mozilla-central/search?string=max-attempt&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

I have looked until 2007. :)

Just to check, if you know something, is it somewhere else?
Flags: needinfo?(mcmanus)
(Assignee)

Comment 13

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b7d98914c7
(In reply to Dragana Damjanovic [:dragana] from comment #12)
> Actually we do not have that pref at all, network.http.request.max-attempts
> does not exist
> 

yeah, its a hidden pref. (i.e. it can be overridden but the default lives only in c++ code). not a huge fan of that myself, but once upon a time there were a lot of them.

I think you should just add the pref, set it to 3, and change the ctor to match.
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #14)

> 
> I think you should just add the pref, set it to 3, and change the ctor to
> match.

which is what you did :)
(Assignee)

Comment 16

2 years ago
Created attachment 8706340 [details] [diff] [review]
bug_1236277.patch

fix a test.
Attachment #8705521 - Attachment is obsolete: true
Attachment #8706340 - Flags: review+
(Assignee)

Comment 17

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c59c0c173445
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee0565977e1
Keywords: checkin-needed

Comment 19

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ee0565977e1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1242926
(Assignee)

Updated

2 years ago
Depends on: 1269055

Updated

a year ago
Depends on: 1277522
You need to log in before you can comment on or make changes to this bug.