Closed Bug 1699998 Opened 4 years ago Closed 4 years ago

Extract the Enabled() check in TRRService::IsDomainBlocked into TRR::MaybeBlockRequest

Categories

(Core :: Networking: DNS, defect, P2)

defect

Tracking

()

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

Enabled() is an API that essentially tells consumers whether or not to use TRR for a request, given the current confirmation state (prefs + computed state). It accepts the request's TRR mode as a parameter in order to do its logic.

Currently IsDomainBlocked() is calling Enabled() without any argument, as a way to return (true) early if we shouldn't use TRR anyway.

The fallout of this is that when the global TRR mode is 0, for requests that have TRR_FIRST_MODE explicitly set, the first call to Enabled along with the request's TRR mode returns true, but later in the flow when we check whether the domain is blocked, the second call to Enable() from IsDomainBlocked returns false, so IsDomainBlocked returns true and we end up falling back to Do53 for the request.

I would argue that the correct behavior here is that we should ensure that Enabled() is called at the original time of deciding whether to use TRR for a request, and IsDomainBlocked should assume that the caller is acting responsibly and has already gotten the green light from Enabled() by the time blocklisting checks are reached.

By this logic, we should just remove the early return in IsDomainBlocked.

Blocks: 1694521
Summary: Don't call Enabled() from IsDomainBlocked in TRRService. → Don't call Enabled() withou from IsDomainBlocked in TRRService.

Currently, we are checking Enabled() without passing the request mode in IsDomainBlocked.
This isn't the right place to do this, it's cleaner if IsDomainBlocked trusts the caller
to have checked Enabled already. We should call Enabled, with the request mode, in
TRR::MaybeBlockDomain() and return early if it's false.

Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Pushed by nhnt11@gmail.com: https://hg.mozilla.org/integration/autoland/rev/74f32f852769 Extract the Enabled() check in TRRService::IsDomainBlocked into TRR::MaybeBlockRequest. r=necko-reviewers,valentin
Summary: Don't call Enabled() withou from IsDomainBlocked in TRRService. → Don't call Enabled() from IsDomainBlocked in TRRService.
Summary: Don't call Enabled() from IsDomainBlocked in TRRService. → Extract the Enabled() check in TRRService::IsDomainBlocked into TRR::MaybeBlockRequest
Severity: -- → S3
Priority: -- → P2
Whiteboard: [necko-triaged]
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: