Bug 1719135 - Make gTRRService atomic and also avoid doing trrLookup when trr service is not ready, r=necko-reviewers,dragana

Differential Revision: https://phabricator.services.mozilla.com/D120621
2021-07-26 09:37:37 +00:00 · 2021-07-26 09:37:37 +00:00 · 298fea6ff9
--- a/netwerk/dns/ODoHService.cpp
+++ b/netwerk/dns/ODoHService.cpp
@ -227,7 +227,7 @@ nsresult ODoHService::UpdateODoHConfigFromURI() {
    return UpdateODoHConfigFromHTTPSRR();
  }

-  nsCOMPtr<nsIEventTarget> target = gTRRService->MainThreadOrTRRThread();
+  nsCOMPtr<nsIEventTarget> target = TRRService::Get()->MainThreadOrTRRThread();
  if (!target) {
    return NS_ERROR_UNEXPECTED;
  }
@ -306,7 +306,7 @@ nsresult ODoHService::UpdateODoHConfigFromHTTPSRR() {
    return NS_ERROR_NOT_AVAILABLE;
  }

-  if (!gTRRService) {
+  if (!TRRService::Get()) {
    return NS_ERROR_NOT_AVAILABLE;
  }

@ -317,7 +317,7 @@ nsresult ODoHService::UpdateODoHConfigFromHTTPSRR() {
  }

  nsCOMPtr<nsICancelable> tmpOutstanding;
-  nsCOMPtr<nsIEventTarget> target = gTRRService->MainThreadOrTRRThread();
+  nsCOMPtr<nsIEventTarget> target = TRRService::Get()->MainThreadOrTRRThread();
  // We'd like to bypass the DNS cache, since ODoHConfigs will be updated
  // manually by ODoHService.
  uint32_t flags =
@ -353,8 +353,8 @@ ODoHService::Notify(nsITimer* aTimer) {

 void ODoHService::ODoHConfigUpdateDone(uint32_t aTTL,
                                       Span<const uint8_t> aRawConfig) {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

  mQueryODoHConfigInProgress = false;
@ -399,7 +399,8 @@ void ODoHService::ODoHConfigUpdateDone(uint32_t aTTL,

  if (!mPendingRequests.IsEmpty()) {
    nsTArray<RefPtr<ODoH>> requests = std::move(mPendingRequests);
-    nsCOMPtr<nsIEventTarget> target = gTRRService->MainThreadOrTRRThread();
+    nsCOMPtr<nsIEventTarget> target =
+        TRRService::Get()->MainThreadOrTRRThread();
    for (auto& query : requests) {
      target->Dispatch(query.forget());
    }
@ -409,8 +410,8 @@ void ODoHService::ODoHConfigUpdateDone(uint32_t aTTL,
 NS_IMETHODIMP
 ODoHService::OnLookupComplete(nsICancelable* aRequest, nsIDNSRecord* aRec,
                              nsresult aStatus) {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

  nsCOMPtr<nsIDNSHTTPSSVCRecord> httpsRecord;
@ -454,8 +455,8 @@ NS_IMETHODIMP
 ODoHService::OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext,
                              nsresult aStatus, uint32_t aLength,
                              const uint8_t* aContent) {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());
  LOG(("ODoHService::OnStreamComplete aLength=%d\n", aLength));

@ -468,8 +469,8 @@ ODoHService::OnStreamComplete(nsIStreamLoader* aLoader, nsISupports* aContext,
 }

 const Maybe<nsTArray<ObliviousDoHConfig>>& ODoHService::ODoHConfigs() {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

  return mODoHConfigs;
@ -477,16 +478,16 @@ const Maybe<nsTArray<ObliviousDoHConfig>>& ODoHService::ODoHConfigs() {

 void ODoHService::AppendPendingODoHRequest(ODoH* aRequest) {
  LOG(("ODoHService::AppendPendingODoHQuery\n"));
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

  mPendingRequests.AppendElement(aRequest);
 }

 bool ODoHService::RemovePendingODoHRequest(ODoH* aRequest) {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

  return mPendingRequests.RemoveElement(aRequest);
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@ -114,11 +114,11 @@ TRR::Notify(nsITimer* aTimer) {

 NS_IMETHODIMP
 TRR::Run() {
-  MOZ_ASSERT_IF(XRE_IsParentProcess() && gTRRService,
-                NS_IsMainThread() || gTRRService->IsOnTRRThread());
+  MOZ_ASSERT_IF(XRE_IsParentProcess() && TRRService::Get(),
+                NS_IsMainThread() || TRRService::Get()->IsOnTRRThread());
  MOZ_ASSERT_IF(XRE_IsSocketProcess(), NS_IsMainThread());

-  if ((gTRRService == nullptr) || NS_FAILED(SendHTTPRequest())) {
+  if ((TRRService::Get() == nullptr) || NS_FAILED(SendHTTPRequest())) {
    RecordReason(TRRSkippedReason::TRR_SEND_FAILED);
    FailData(NS_ERROR_FAILURE);
    // The dtor will now be run
@ -138,7 +138,7 @@ nsresult TRR::CreateQueryURI(nsIURI** aOutURI) {
  nsAutoCString uri;
  nsCOMPtr<nsIURI> dnsURI;
  if (UseDefaultServer()) {
-    gTRRService->GetURI(uri);
+    TRRService::Get()->GetURI(uri);
  } else {
    uri = mRec->mTrrServer;
  }
@ -160,13 +160,13 @@ bool TRR::MaybeBlockRequest() {
    MOZ_ASSERT(mRec);

    // If TRRService isn't enabled anymore for the req, don't do TRR.
-    if (!gTRRService->Enabled(mRec->mEffectiveTRRMode)) {
+    if (!TRRService::Get()->Enabled(mRec->mEffectiveTRRMode)) {
      RecordReason(TRRSkippedReason::TRR_MODE_NOT_ENABLED);
      return true;
    }

-    if (UseDefaultServer() &&
-        gTRRService->IsTemporarilyBlocked(mHost, mOriginSuffix, mPB, true)) {
+    if (UseDefaultServer() && TRRService::Get()->IsTemporarilyBlocked(
+                                  mHost, mOriginSuffix, mPB, true)) {
      if (mType == TRRTYPE_A) {
        // count only blocklist for A records to avoid double counts
        Telemetry::Accumulate(Telemetry::DNS_TRR_BLACKLISTED3,
@ -178,7 +178,7 @@ bool TRR::MaybeBlockRequest() {
      return true;
    }

-    if (gTRRService->IsExcludedFromTRR(mHost)) {
+    if (TRRService::Get()->IsExcludedFromTRR(mHost)) {
      RecordReason(TRRSkippedReason::TRR_EXCLUDED);
      return true;
    }
@ -285,7 +285,7 @@ nsresult TRR::SendHTTPRequest() {

  nsAutoCString cred;
  if (UseDefaultServer()) {
-    gTRRService->GetCredentials(cred);
+    TRRService::Get()->GetCredentials(cred);
  }
  if (!cred.IsEmpty()) {
    rv = httpChannel->SetRequestHeader("Authorization"_ns, cred, false);
@ -342,7 +342,7 @@ nsresult TRR::SendHTTPRequest() {

  NS_NewTimerWithCallback(
      getter_AddRefs(mTimeout), this,
-      mTimeoutMs ? mTimeoutMs : gTRRService->GetRequestTimeout(),
+      mTimeoutMs ? mTimeoutMs : TRRService::Get()->GetRequestTimeout(),
      nsITimer::TYPE_ONE_SHOT);

  mChannel = channel;
@ -507,7 +507,7 @@ nsresult TRR::ReceivePush(nsIHttpChannel* pushed, nsHostRecord* pushedRec) {
    return NS_ERROR_UNEXPECTED;
  }

-  if (gTRRService->IsExcludedFromTRR(mHost)) {
+  if (TRRService::Get()->IsExcludedFromTRR(mHost)) {
    return NS_ERROR_FAILURE;
  }

@ -811,10 +811,10 @@ nsresult TRR::FollowCname(nsIChannel* aChannel) {
      ResolverType() == DNSResolverType::ODoH
          ? new ODoH(mHostResolver, mRec, mCname, mType, mCnameLoop, mPB)
          : new TRR(mHostResolver, mRec, mCname, mType, mCnameLoop, mPB);
-  if (!gTRRService) {
+  if (!TRRService::Get()) {
    return NS_ERROR_FAILURE;
  }
-  return gTRRService->DispatchTRRRequest(trr);
+  return TRRService::Get()->DispatchTRRRequest(trr);
 }

 nsresult TRR::On200Response(nsIChannel* aChannel) {
@ -874,7 +874,7 @@ void TRR::ReportStatus(nsresult aStatusCode) {
  // it as failed; otherwise it can cause the confirmation to fail.
  if (UseDefaultServer() && aStatusCode != NS_ERROR_ABORT) {
    // Bad content is still considered "okay" if the HTTP response is okay
-    gTRRService->RecordTRRStatus(aStatusCode);
+    TRRService::Get()->RecordTRRStatus(aStatusCode);
  }
 }

@ -988,8 +988,8 @@ TRR::OnDataAvailable(nsIRequest* aRequest, nsIInputStream* aInputStream,
 void TRR::Cancel(nsresult aStatus) {
  RefPtr<TRRServiceChannel> trrServiceChannel = do_QueryObject(mChannel);
  if (trrServiceChannel && !XRE_IsSocketProcess()) {
-    if (gTRRService) {
-      nsCOMPtr<nsIThread> thread = gTRRService->TRRThread();
+    if (TRRService::Get()) {
+      nsCOMPtr<nsIThread> thread = TRRService::Get()->TRRThread();
      if (thread && !thread->IsOnCurrentThread()) {
        thread->Dispatch(NS_NewRunnableFunction(
            "TRR::Cancel",
--- a/netwerk/dns/TRR.h
+++ b/netwerk/dns/TRR.h
@ -27,7 +27,6 @@ namespace net {

 class TRRService;
 class TRRServiceChannel;
-extern TRRService* gTRRService;

 class TRR : public Runnable,
            public nsITimerCallback,
--- a/netwerk/dns/TRRQuery.cpp
+++ b/netwerk/dns/TRRQuery.cpp
@ -90,8 +90,8 @@ nsresult TRRQuery::DispatchLookup(TRR* pushedTRR, bool aUseODoH) {
    nsTArray<RefPtr<TRR>> requestsToSend;
    do {
      sendAgain = false;
-      if ((TRRTYPE_AAAA == rectype) && gTRRService &&
-          (gTRRService->DisableIPv6() ||
+      if ((TRRTYPE_AAAA == rectype) && TRRService::Get() &&
+          (TRRService::Get()->DisableIPv6() ||
           (StaticPrefs::network_trr_skip_AAAA_when_not_supported() &&
            mHostResolver->GetNCS() &&
            mHostResolver->GetNCS()->GetIPv6() ==
@ -135,7 +135,7 @@ nsresult TRRQuery::DispatchLookup(TRR* pushedTRR, bool aUseODoH) {

    mTRRRequestCounter = requestsToSend.Length();
    for (const auto& request : requestsToSend) {
-      if (NS_SUCCEEDED(gTRRService->DispatchTRRRequest(request))) {
+      if (NS_SUCCEEDED(TRRService::Get()->DispatchTRRRequest(request))) {
        madeQuery = true;
      } else {
        mTRRRequestCounter--;
@ -175,7 +175,7 @@ nsresult TRRQuery::DispatchLookup(TRR* pushedTRR, bool aUseODoH) {
      trr = pushedTRR ? pushedTRR : new TRR(this, mRecord, rectype);
    }

-    if (pushedTRR || NS_SUCCEEDED(gTRRService->DispatchTRRRequest(trr))) {
+    if (pushedTRR || NS_SUCCEEDED(TRRService::Get()->DispatchTRRRequest(trr))) {
      MutexAutoLock trrlock(mTrrLock);
      MOZ_ASSERT(!mTrrByType);
      mTrrByType = trr;
--- a/netwerk/dns/TRRService.cpp
+++ b/netwerk/dns/TRRService.cpp
@ -42,7 +42,6 @@ static const char kDisableIpv6Pref[] = "network.dns.disableIPv6";
 namespace mozilla {
 namespace net {

-TRRService* gTRRService = nullptr;
 StaticRefPtr<nsIThread> sTRRBackgroundThread;
 static Atomic<TRRService*> sTRRServicePtr;

@ -69,6 +68,9 @@ NS_IMPL_QUERY_INTERFACE(TRRService::ConfirmationContext, nsITimerCallback)

 TRRService::TRRService() { MOZ_ASSERT(NS_IsMainThread(), "wrong thread"); }

+// static
+TRRService* TRRService::Get() { return sTRRServicePtr; }
+
 // static
 void TRRService::AddObserver(nsIObserver* aObserver,
                             nsIObserverService* aObserverService) {
@ -162,7 +164,6 @@ nsresult TRRService::Init() {
    prefBranch->AddObserver(kRolloutModePref, this, true);
  }

-  gTRRService = this;
  sTRRServicePtr = this;

  ReadPrefs(nullptr);
@ -536,7 +537,6 @@ nsresult TRRService::Start() {
 TRRService::~TRRService() {
  MOZ_ASSERT(NS_IsMainThread(), "wrong thread");
  LOG(("Exiting TRRService\n"));
-  gTRRService = nullptr;
 }

 nsresult TRRService::DispatchTRRRequest(TRR* aTrrRequest) {
--- a/netwerk/dns/TRRService.h
+++ b/netwerk/dns/TRRService.h
@ -37,6 +37,8 @@ class TRRService : public TRRServiceBase,
  NS_DECL_NSIOBSERVER

  TRRService();
+  static TRRService* Get();
+
  nsresult Init();
  nsresult Start();
  bool Enabled(nsIRequest::TRRMode aRequestMode = nsIRequest::TRR_DEFAULT_MODE);
@ -325,8 +327,6 @@ class TRRService : public TRRServiceBase,
  nsCOMPtr<nsINetworkLinkService> mLinkService;
 };

-extern TRRService* gTRRService;
-
 }  // namespace net
 }  // namespace mozilla

--- a/netwerk/dns/TRRServiceChild.cpp
+++ b/netwerk/dns/TRRServiceChild.cpp
@ -25,22 +25,24 @@ void TRRServiceChild::Init(const bool& aCaptiveIsPassed,
  sDNSService = dns;
  ClearOnShutdown(&sDNSService);
  MOZ_ASSERT(sDNSService);
-  MOZ_ASSERT(gTRRService);

-  gTRRService->mCaptiveIsPassed = aCaptiveIsPassed;
-  gTRRService->mParentalControlEnabled = aParentalControlEnabled;
-  gTRRService->RebuildSuffixList(std::move(aDNSSuffixList));
+  TRRService* trrService = TRRService::Get();
+  MOZ_ASSERT(trrService);
+
+  trrService->mCaptiveIsPassed = aCaptiveIsPassed;
+  trrService->mParentalControlEnabled = aParentalControlEnabled;
+  trrService->RebuildSuffixList(std::move(aDNSSuffixList));
 }

 mozilla::ipc::IPCResult TRRServiceChild::RecvUpdatePlatformDNSInformation(
    nsTArray<nsCString>&& aDNSSuffixList) {
-  gTRRService->RebuildSuffixList(std::move(aDNSSuffixList));
+  TRRService::Get()->RebuildSuffixList(std::move(aDNSSuffixList));
  return IPC_OK();
 }

 mozilla::ipc::IPCResult TRRServiceChild::RecvUpdateParentalControlEnabled(
    const bool& aEnabled) {
-  gTRRService->mParentalControlEnabled = aEnabled;
+  TRRService::Get()->mParentalControlEnabled = aEnabled;
  return IPC_OK();
 }

@ -52,7 +54,7 @@ mozilla::ipc::IPCResult TRRServiceChild::RecvClearDNSCache(

 mozilla::ipc::IPCResult TRRServiceChild::RecvSetDetectedTrrURI(
    const nsCString& aURI) {
-  gTRRService->SetDetectedTrrURI(aURI);
+  TRRService::Get()->SetDetectedTrrURI(aURI);
  return IPC_OK();
 }

--- a/netwerk/dns/nsHostRecord.cpp
+++ b/netwerk/dns/nsHostRecord.cpp
@ -350,8 +350,8 @@ void AddrHostRecord::ResolveComplete() {
  }

  if (mResolverType == DNSResolverType::TRR && !mTRRSuccess && mNativeSuccess &&
-      gTRRService) {
-    gTRRService->AddToBlocklist(nsCString(host), originSuffix, pb, true);
+      TRRService::Get()) {
+    TRRService::Get()->AddToBlocklist(nsCString(host), originSuffix, pb, true);
  }
 }

--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@ -465,8 +465,7 @@ nsresult nsHostResolver::ResolveHost(const nsACString& aHost,
    // callback, and proceed to do the lookup.

    bool excludedFromTRR = false;
-
-    if (gTRRService && gTRRService->IsExcludedFromTRR(host)) {
+    if (TRRService::Get() && TRRService::Get()->IsExcludedFromTRR(host)) {
      flags |= RES_DISABLE_TRR;
      excludedFromTRR = true;

@ -877,7 +876,7 @@ nsresult nsHostResolver::TrrLookup(nsHostRecord* aRec,

  nsIRequest::TRRMode reqMode = rec->mEffectiveTRRMode;
  if (rec->mTrrServer.IsEmpty() &&
-      (!gTRRService || !gTRRService->Enabled(reqMode))) {
+      (!TRRService::Get() || !TRRService::Get()->Enabled(reqMode))) {
    if (NS_IsOffline()) {
      // If we are in the NOT_CONFIRMED state _because_ we lack connectivity,
      // then we should report that the browser is offline instead.
@ -953,8 +952,8 @@ nsresult nsHostResolver::NativeLookup(nsHostRecord* aRec,

 // static
 nsIDNSService::ResolverMode nsHostResolver::Mode() {
-  if (gTRRService) {
-    return gTRRService->Mode();
+  if (TRRService::Get()) {
+    return TRRService::Get()->Mode();
  }

  // If we don't have a TRR service just return MODE_TRROFF so we don't make
@ -976,7 +975,7 @@ void nsHostResolver::ComputeEffectiveTRRMode(nsHostRecord* aRec) {
  // localhost and local are excluded (so we cover *.local hosts) See the
  // network.trr.excluded-domains pref.

-  if (!gTRRService) {
+  if (!TRRService::Get()) {
    aRec->RecordReason(TRRSkippedReason::TRR_NO_GSERVICE);
    aRec->mEffectiveTRRMode = requestMode;
    return;
@ -987,14 +986,14 @@ void nsHostResolver::ComputeEffectiveTRRMode(nsHostRecord* aRec) {
    return;
  }

-  if (gTRRService->IsExcludedFromTRR(aRec->host)) {
+  if (TRRService::Get()->IsExcludedFromTRR(aRec->host)) {
    aRec->RecordReason(TRRSkippedReason::TRR_EXCLUDED);
    aRec->mEffectiveTRRMode = nsIRequest::TRR_DISABLED_MODE;
    return;
  }

  if (StaticPrefs::network_dns_skipTRR_when_parental_control_enabled() &&
-      gTRRService->ParentalControlEnabled()) {
+      TRRService::Get()->ParentalControlEnabled()) {
    aRec->RecordReason(TRRSkippedReason::TRR_PARENTAL_CONTROL);
    aRec->mEffectiveTRRMode = nsIRequest::TRR_DISABLED_MODE;
    return;
@ -1085,14 +1084,13 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec,
    rec->RecordReason(TRRSkippedReason::TRR_DISABLED_FLAG);
  }

+  bool serviceNotReady =
+      !TRRService::Get() || !TRRService::Get()->Enabled(rec->mEffectiveTRRMode);
  if (rec->mEffectiveTRRMode != nsIRequest::TRR_DISABLED_MODE &&
-      !((rec->flags & RES_DISABLE_TRR))) {
+      !((rec->flags & RES_DISABLE_TRR)) && !serviceNotReady) {
    rv = TrrLookup(rec, aLock);
  }

-  bool serviceNotReady =
-      !gTRRService || !gTRRService->Enabled(rec->mEffectiveTRRMode);
-
  if (rec->mEffectiveTRRMode == nsIRequest::TRR_DISABLED_MODE ||
      (rec->mEffectiveTRRMode == nsIRequest::TRR_FIRST_MODE &&
       (rec->flags & RES_DISABLE_TRR || serviceNotReady || NS_FAILED(rv)))) {
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@ -6713,7 +6713,7 @@ nsHttpChannel::OnStartRequest(nsIRequest* request) {
      Telemetry::Accumulate(Telemetry::HTTP_CHANNEL_ONSTART_SUCCESS_ODOH,
                            NS_SUCCEEDED(mStatus));
    }
-  } else if (gTRRService && gTRRService->IsConfirmed()) {
+  } else if (TRRService::Get() && TRRService::Get()->IsConfirmed()) {
    // Note this telemetry probe is not working when DNS resolution is done in
    // the socket process.
    Telemetry::Accumulate(Telemetry::HTTP_CHANNEL_ONSTART_SUCCESS_TRR2,