Closed
Bug 1236277
Opened 8 years ago
Closed 8 years ago
"Connection was reset" errors visiting BBC news site via redirection from old URL
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: smichaud, Assigned: dragana)
References
()
Details
Attachments
(2 files, 3 obsolete files)
3.03 KB,
application/pcap
|
Details | |
6.16 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
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•8 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.
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.
Comment 3•8 years ago
|
||
pcap.. stream 0 has a rst with 0 response data bytes.. stream 1 is a manual reload that worked. no difference on request side
Comment 4•8 years ago
|
||
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?
Comment 6•8 years ago
|
||
fwiw I also filled out the "report a problem with our website" link on the bbc,
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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•8 years ago
|
||
Attachment #8704095 -
Attachment is obsolete: true
Attachment #8705135 -
Flags: review?(mcmanus)
Comment 10•8 years ago
|
||
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•8 years ago
|
||
Attachment #8705135 -
Attachment is obsolete: true
Attachment #8705521 -
Flags: review+
Assignee | ||
Comment 12•8 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•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63b7d98914c7
Comment 14•8 years ago
|
||
(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)
Comment 15•8 years ago
|
||
(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•8 years ago
|
||
fix a test.
Attachment #8705521 -
Attachment is obsolete: true
Attachment #8706340 -
Flags: review+
Assignee | ||
Comment 17•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c59c0c173445
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 18•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ee0565977e1
Keywords: checkin-needed
Comment 19•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ee0565977e1
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•