Removed always-true ClientRequestContext::httpStateIsValid() (#2279)

... and related cbdata checks/comments.

`ClientRequestContext::httpStateIsValid()` always returns true. We
can prove that by contradiction:

* `ClientRequestContext::httpStateIsValid()` return value is
  `cbdataReferenceValid(http)` return value.
* `ClientRequestContext::http` points to a `ClientHttpRequest` object.
* If `cbdataReferenceValid(http)` is false, then the corresponding
  `ClientHttpRequest` object pointed to by `http` has been destructed.
* `ClientHttpRequest` destructor deletes its `calloutContext` data
  member (i.e. `ClientRequestContext`).

Thus, if an `x->httpStateIsValid()` call returns false, then `x` object
has already been deleted, and the call itself was buggy because it was
dereferencing a pointer to a deleted object!

Moreover, `cbdataReferenceValid(nullptr)` is always true, so calling
`httpStateIsValid()` twice for a deleted `ClientHttpRequest` object does
not actually protect the second caller (i.e. does not detect that `http`
is gone) because the first call makes `ClientRequestContext::http` nil,
making `httpStateIsValid()` true again!

None of the above problems actually happen because
`cbdataReferenceValid(http)` is never false inside
`ClientRequestContext::httpStateIsValid()`. By the time it could have
become false, `ClientRequestContext` object itself is already gone.

Also removed a source code comment that falsely claimed that
`ClientRequestContext` callback methods should check whether
`ClientHttpRequest` object is still "valid". In reality,
`ClientRequestContext` methods are not called after `ClientHttpRequest`
object is destructed because they are protected by cbdata guards, and
`ClientHttpRequest` destruction deletes `ClientRequestContext` object,
resulting in those guards skipping any callbacks.

The same comment also incorrectly implied that `ClientHttpRequest` first
becomes "invalid" and then, _after_ the current callback is done,
`ClientHttpRequest` destructor is called. In reality,
`cbdataReferenceValid(x)` becomes false when `delete x` is called and,
hence, when `x` destructor runs.

diff --git a/src/ClientRequestContext.h b/src/ClientRequestContext.h
index a5ac37842b185b27445602f378d34e8583132ff4..6d26c1741fec9724c2c984c16d986b14869ef798 100644 (file)
--- a/src/ClientRequestContext.h
+++ b/src/ClientRequestContext.h
@@ -33,7 +33,6 @@ public:
     ClientRequestContext(ClientHttpRequest *);
     ~ClientRequestContext() override;
 
-    bool httpStateIsValid();
     void hostHeaderVerify();
     void hostHeaderIpVerify(const ipcache_addrs *, const Dns::LookupDetails &);
     void hostHeaderVerifyFailed(const char *A, const char *B);

diff --git a/src/client_side_request.cc b/src/client_side_request.cc
index 6afcec7e25d617f20fc98a3271f76c3218a9090d..3ef4dc80d4ace06efd32248837928dc2637edc48 100644 (file)
--- a/src/client_side_request.cc
+++ b/src/client_side_request.cc
@@ -106,11 +106,6 @@ static void checkFailureRatio(err_type, hier_code);
 
 ClientRequestContext::~ClientRequestContext()
 {
-    /*
-     * Release our "lock" on our parent, ClientHttpRequest, if we
-     * still have one
-     */
-
     cbdataReferenceDone(http);
 
     delete error;
@@ -268,21 +263,6 @@ ClientHttpRequest::~ClientHttpRequest()
     dlinkDelete(&active, &ClientActiveRequests);
 }
 
-bool
-ClientRequestContext::httpStateIsValid()
-{
-    ClientHttpRequest *http_ = http;
-
-    if (cbdataReferenceValid(http_))
-        return true;
-
-    http = nullptr;
-
-    cbdataReferenceDone(http_);
-
-    return false;
-}
-
 #if FOLLOW_X_FORWARDED_FOR
 /**
  * clientFollowXForwardedForCheck() checks the content of X-Forwarded-For:
@@ -302,10 +282,6 @@ static void
 clientFollowXForwardedForCheck(Acl::Answer answer, void *data)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *) data;
-
-    if (!calloutContext->httpStateIsValid())
-        return;
-
     ClientHttpRequest *http = calloutContext->http;
     HttpRequest *request = http->request;
 
@@ -607,10 +583,6 @@ void
 clientAccessCheckDoneWrapper(Acl::Answer answer, void *data)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *) data;
-
-    if (!calloutContext->httpStateIsValid())
-        return;
-
     calloutContext->clientAccessCheckDone(answer);
 }
 
@@ -1024,10 +996,6 @@ void
 clientRedirectDoneWrapper(void *data, const Helper::Reply &result)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *)data;
-
-    if (!calloutContext->httpStateIsValid())
-        return;
-
     calloutContext->clientRedirectDone(result);
 }
 
@@ -1035,10 +1003,6 @@ void
 clientStoreIdDoneWrapper(void *data, const Helper::Reply &result)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *)data;
-
-    if (!calloutContext->httpStateIsValid())
-        return;
-
     calloutContext->clientStoreIdDone(result);
 }
 
@@ -1225,10 +1189,6 @@ static void
 checkNoCacheDoneWrapper(Acl::Answer answer, void *data)
 {
     ClientRequestContext *calloutContext = (ClientRequestContext *) data;
-
-    if (!calloutContext->httpStateIsValid())
-        return;
-
     calloutContext->checkNoCacheDone(answer);
 }
 
@@ -1321,18 +1281,12 @@ static void
 sslBumpAccessCheckDoneWrapper(Acl::Answer answer, void *data)
 {
     ClientRequestContext *calloutContext = static_cast<ClientRequestContext *>(data);
-
-    if (!calloutContext->httpStateIsValid())
-        return;
     calloutContext->sslBumpAccessCheckDone(answer);
 }
 
 void
 ClientRequestContext::sslBumpAccessCheckDone(const Acl::Answer &answer)
 {
-    if (!httpStateIsValid())
-        return;
-
     const Ssl::BumpMode bumpMode = answer.allowed() ?
                                    static_cast<Ssl::BumpMode>(answer.kind) : Ssl::bumpSplice;
     http->sslBumpNeed(bumpMode); // for processRequest() to bump if needed
@@ -1409,8 +1363,6 @@ SslBumpEstablish(const Comm::ConnectionPointer &, char *, size_t, Comm::Flag err
 {
     ClientHttpRequest *r = static_cast<ClientHttpRequest*>(data);
     debugs(85, 5, "responded to CONNECT: " << r << " ? " << errflag);
-
-    assert(r && cbdataReferenceValid(r));
     r->sslBumpEstablish(errflag);
 }
 
@@ -1612,11 +1564,6 @@ ClientHttpRequest::clearRequest()
  * Note that ClientRequestContext is created before the first call
  * to doCallouts().
  *
- * If one of the callouts notices that ClientHttpRequest is no
- * longer valid, it should call cbdataReferenceDone() so that
- * ClientHttpRequest's reference count goes to zero and it will get
- * deleted.  ClientHttpRequest will then delete ClientRequestContext.
- *
  * Note that we set the _done flags here before actually starting
  * the callout.  This is strictly for convenience.
  */
@@ -1768,7 +1715,6 @@ ClientHttpRequest::doCallouts()
         }
     }
 
-    cbdataReferenceDone(calloutContext->http);
     delete calloutContext;
     calloutContext = nullptr;
 
@@ -1872,7 +1818,6 @@ ClientHttpRequest::startAdaptation(const Adaptation::ServiceGroupPointer &g)
 void
 ClientHttpRequest::noteAdaptationAnswer(const Adaptation::Answer &answer)
 {
-    assert(cbdataReferenceValid(this));     // indicates bug
     clearAdaptation(virginHeadSource);
     assert(!adaptedBodySource);