Closed Bug 1236277 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: smichaud, Assigned: dragana)

References

()

Details

Attachments

(2 files, 3 obsolete files)

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).
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.
Attached file 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?
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,
Attached patch bug_1236277.patch (obsolete) — Splinter Review
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)
Attached patch bug_1236277.patch (obsolete) — Splinter Review
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+
Attached patch bug_1236277.patch (obsolete) — Splinter Review
Attachment #8705135 - Attachment is obsolete: true
Attachment #8705521 - Flags: review+
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)
(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 :)
fix a test.
Attachment #8705521 - Attachment is obsolete: true
Attachment #8706340 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ee0565977e1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1242926
Depends on: 1269055
Depends on: 1277522
You need to log in before you can comment on or make changes to this bug.