Skip to content

Commit

Permalink
NetworkPortalDetectorImpl: Use PortalStateChanged
Browse files Browse the repository at this point in the history 
This modifies NetworkPortalDetectorImpl to observe PortalStateChanged
instead of observing DefaultNetworkChanged and performing its own state
tracking.

This also updates and improves tests to ensure that the behavior remains
as expected.

BUG=b:207069182
TEST=unit_tests, chromeos_unittests, browser_tests

Change-Id: I1948189ee794ff7aba0db4dc302fd8b8f464f4f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3810587
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1035169}
  • Loading branch information
@stevenjb
stevenjb authored and Chromium LUCI CQ committed Aug 15, 2022
1 parent ce9f884 commit 15778f3
Show file tree
Hide file tree
Showing 5 changed files with 273 additions and 224 deletions.
187 changes: 94 additions & 93 deletions chrome/browser/ash/net/network_portal_detector_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,14 +143,13 @@ void NetworkPortalDetectorImpl::Enable() {
if (enabled_)
return;

NET_LOG(EVENT) << "NetworkPortalDetector Enabled.";
DCHECK(is_idle());
enabled_ = true;

const NetworkState* network = DefaultNetwork();
if (!network)
return;
NET_LOG(EVENT) << "Starting detection attempt:"
<< " id=" << NetworkId(network);
SetNetworkPortalState(network, NetworkState::PortalState::kUnknown);
StartDetection();
}
Expand All @@ -164,8 +163,12 @@ NetworkPortalDetectorImpl::GetCaptivePortalStatus() {
void NetworkPortalDetectorImpl::StartPortalDetection() {
if (!is_idle())
return;
const NetworkState* network = DefaultNetwork();
if (!network) {
NET_LOG(ERROR) << "StartPortalDetection called with no default network.";
return;
}
StartDetection();
return;
}

void NetworkPortalDetectorImpl::SetStrategy(
Expand All @@ -175,75 +178,66 @@ void NetworkPortalDetectorImpl::SetStrategy(
strategy_ = PortalDetectorStrategy::CreateById(id, this);
if (!is_idle())
StopDetection();
StartPortalDetection();
StartDetection();
}

void NetworkPortalDetectorImpl::DefaultNetworkChanged(
const NetworkState* default_network) {
void NetworkPortalDetectorImpl::PortalStateChanged(
const NetworkState* default_network,
NetworkState::PortalState portal_state) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

if (!default_network) {
NET_LOG(EVENT) << "Default network changed: None";

if (!default_network || !default_network->IsConnectedState()) {
NET_LOG(EVENT)
<< "No connected default network, stopping portal detection.";
default_network_id_ = std::string();
default_proxy_config_ = base::Value();

StopDetection();

DetectionCompleted(nullptr, CAPTIVE_PORTAL_STATUS_OFFLINE, -1);
DetectionCompleted(nullptr, CAPTIVE_PORTAL_STATUS_OFFLINE);
return;
}

bool network_changed = (default_network_id_ != default_network->guid());
if (network_changed)
default_network_id_ = default_network->guid();

bool connection_state_changed =
(default_connection_state_ != default_network->connection_state());
default_connection_state_ = default_network->connection_state();

bool proxy_config_changed = false;
const base::Value& default_network_proxy_config =
default_network->proxy_config();
if (default_network_proxy_config.is_none()) {
if (!default_proxy_config_.is_none()) {
proxy_config_changed = true;
default_proxy_config_ = base::Value();
}
} else if (network_changed ||
default_proxy_config_ != default_network_proxy_config) {
proxy_config_changed = true;
default_proxy_config_ = default_network_proxy_config.Clone();
}

NET_LOG(EVENT) << "Default network changed:"
<< " id=" << NetworkGuidId(default_network_id_)
<< " state=" << default_connection_state_
<< " changed=" << network_changed
<< " proxy_config_changed=" << proxy_config_changed
<< " state_changed=" << connection_state_changed;

if (network_changed || connection_state_changed || proxy_config_changed)
StopDetection();

if (!NetworkState::StateIsConnected(default_connection_state_))
return;

if (proxy_config_changed) {
ScheduleAttempt(base::Seconds(kProxyChangeDelaySec));
return;
}

if (is_idle()) {
// Initiate Captive Portal detection if network's captive
// portal state is unknown (e.g. for freshly created networks),
// offline or if network connection state was changed.
CaptivePortalStatus status = GetCaptivePortalStatus();
if (status == CAPTIVE_PORTAL_STATUS_UNKNOWN ||
status == CAPTIVE_PORTAL_STATUS_OFFLINE ||
(!network_changed && connection_state_changed)) {
ScheduleAttempt(base::TimeDelta());
}
default_network_id_ = default_network->guid();
bool has_proxy = !default_network->proxy_config().is_none();
NET_LOG(EVENT) << "PortalStateChanged, id="
<< NetworkGuidId(default_network_id_)
<< " state=" << default_network->connection_state()
<< " portal_state=" << portal_state
<< " has_proxy=" << has_proxy;

switch (portal_state) {
case NetworkState::PortalState::kUnknown:
// Not expected. Shill detection failed or unexpected results, use Chrome
// portal detection.
NET_LOG(ERROR) << "Unknown PortalState, scheduling Chrome detection.";
ScheduleAttempt();
return;
case NetworkState::PortalState::kOnline:
// If a proxy is configured, use captive_portal_detector_ to detect a
// proxy auth required (407) response.
if (has_proxy)
ScheduleAttempt();
else
DetectionCompleted(default_network, CAPTIVE_PORTAL_STATUS_ONLINE);
return;
case NetworkState::PortalState::kPortalSuspected:
// Shill result was inconclusive.
ScheduleAttempt();
return;
case NetworkState::PortalState::kPortal:
DetectionCompleted(default_network, CAPTIVE_PORTAL_STATUS_PORTAL);
return;
case NetworkState::PortalState::kNoInternet:
// If a proxy is configured it may be interfering with Shill portal
// detection
if (has_proxy)
ScheduleAttempt();
else
DetectionCompleted(default_network, CAPTIVE_PORTAL_STATUS_ONLINE);
return;
case NetworkState::PortalState::kProxyAuthRequired:
// This may happen if a global proxy is applied. Run Chrome detection
// to verify.
ScheduleAttempt();
return;
}
}

Expand All @@ -269,15 +263,18 @@ base::TimeTicks NetworkPortalDetectorImpl::NowTicks() const {
// NetworkPortalDetectorImpl, private:

void NetworkPortalDetectorImpl::StartDetection() {
DCHECK(is_idle());
NET_LOG(EVENT) << "StartDetection";

ResetStrategyAndCounters();
default_portal_status_ = CAPTIVE_PORTAL_STATUS_UNKNOWN;
detection_start_time_ = NowTicks();
ScheduleAttempt(base::TimeDelta());
ScheduleAttempt();
}

void NetworkPortalDetectorImpl::StopDetection() {
if (is_idle())
return;
NET_LOG(EVENT) << "StopDetection";
attempt_task_.Cancel();
attempt_timeout_.Cancel();
captive_portal_detector_->Cancel();
Expand Down Expand Up @@ -363,15 +360,6 @@ void NetworkPortalDetectorImpl::OnAttemptCompleted(
}
}

// If using a fake profile client, also fake being behind a captive portal
// if the default network is in portal state.
if (result != captive_portal::RESULT_NO_RESPONSE &&
ShillProfileClient::Get()->GetTestInterface() &&
shill_is_captive_portal) {
result = captive_portal::RESULT_BEHIND_CAPTIVE_PORTAL;
response_code = 200;
}

state_ = STATE_IDLE;
attempt_timeout_.Cancel();

Expand Down Expand Up @@ -439,25 +427,25 @@ void NetworkPortalDetectorImpl::OnAttemptCompleted(
else
no_response_result_count_ = 0;

bool detection_completed = false;
if (status == CAPTIVE_PORTAL_STATUS_ONLINE ||
status == CAPTIVE_PORTAL_STATUS_PORTAL ||
status == CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED) {
// Chrome positively identified an online, portal or proxy auth state.
// No need to continue detection.
DetectionCompleted(network, status, response_code);
return;
}

if (same_detection_result_count_ >= kMaxOfflineResultsBeforeReport) {
detection_completed = true;
} else if (same_detection_result_count_ >= kMaxOfflineResultsBeforeReport) {
NET_LOG(EVENT) << "Max identical portal detection results reached: "
<< same_detection_result_count_ << " Status: " << status;
DetectionCompleted(network, status, response_code);
return;
detection_completed = true;
}

// Observers (via DetectionCompleted) may already schedule a new attempt.
if (is_idle())
if (detection_completed) {
response_code_for_testing_ = response_code;
DetectionCompleted(network, status);
} else if (is_idle()) {
ScheduleAttempt(results.retry_after_delta);
}
}

void NetworkPortalDetectorImpl::Observe(
Expand All @@ -475,21 +463,34 @@ void NetworkPortalDetectorImpl::Observe(

void NetworkPortalDetectorImpl::DetectionCompleted(
const NetworkState* network,
const CaptivePortalStatus& status,
int response_code) {
const CaptivePortalStatus& status) {
NET_LOG(EVENT) << "NetworkPortalDetector: DetectionCompleted: id="
<< (network ? NetworkGuidId(network->guid()) : "<none>")
<< ", status=" << status
<< ", response_code=" << response_code;
<< ", status=" << status;

default_portal_status_ = status;
response_code_for_testing_ = response_code;
if (network) {
// TODO(b/207069182): Set online and proxy_auth_required also.
auto portal_state =
status == NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_PORTAL
? NetworkState::PortalState::kPortal
: NetworkState::PortalState::kUnknown;
NetworkState::PortalState portal_state;
switch (status) {
case CAPTIVE_PORTAL_STATUS_UNKNOWN:
case CAPTIVE_PORTAL_STATUS_COUNT:
case CAPTIVE_PORTAL_STATUS_OFFLINE:
portal_state = NetworkState::PortalState::kUnknown;
break;
case CAPTIVE_PORTAL_STATUS_ONLINE:
// TODO(b/207069182): This should state PortalState::kOnline.
portal_state = NetworkState::PortalState::kUnknown;
break;
case CAPTIVE_PORTAL_STATUS_PORTAL:
portal_state = NetworkState::PortalState::kPortal;
break;
case CAPTIVE_PORTAL_STATUS_PROXY_AUTH_REQUIRED:
// TODO(b/207069182): This should state PortalState::kProxyAuthRequired.
portal_state = NetworkState::PortalState::kUnknown;
break;
}
// Note: setting an unknown portal state will ignore the Chrome result and
// fall back to the Shill result.
SetNetworkPortalState(network, portal_state);
}
for (auto& observer : observers_)
Expand Down
20 changes: 4 additions & 16 deletions chrome/browser/ash/net/network_portal_detector_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,6 @@
#include "content/public/browser/notification_registrar.h"
#include "url/gurl.h"

namespace base {
class Value;
}

namespace network {
class SharedURLLoaderFactory;
namespace mojom {
Expand Down Expand Up @@ -83,7 +79,7 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
void RetryDetection();

// Initiates Captive Portal detection attempt after |delay|.
void ScheduleAttempt(const base::TimeDelta& delay);
void ScheduleAttempt(const base::TimeDelta& delay = base::TimeDelta());

// Starts detection attempt.
void StartAttempt();
Expand All @@ -107,8 +103,9 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
void SetStrategy(PortalDetectorStrategy::StrategyId id) override;

// NetworkStateHandlerObserver implementation:
void DefaultNetworkChanged(const NetworkState* network) override;
void OnShuttingDown() override;
void PortalStateChanged(const NetworkState* default_network,
NetworkState::PortalState portal_state) override;

// PortalDetectorStrategy::Delegate implementation:
int NoResponseResultCount() override;
Expand All @@ -120,11 +117,8 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
const content::NotificationSource& source,
const content::NotificationDetails& details) override;

// Called synchronously from OnAttemptCompleted with the current default
// network. Stores the captive portal status and notifies observers.
void DetectionCompleted(const NetworkState* network,
const CaptivePortalStatus& results,
int response_code);
const CaptivePortalStatus& results);

State state() const { return state_; }

Expand Down Expand Up @@ -174,12 +168,6 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
// Unique identifier of the default network.
std::string default_network_id_;

// Connection state of the default network.
std::string default_connection_state_;

// Proxy configuration of the default network.
base::Value default_proxy_config_;

CaptivePortalStatus default_portal_status_ = CAPTIVE_PORTAL_STATUS_UNKNOWN;
int response_code_for_testing_ = -1;

Expand Down

0 comments on commit 15778f3

Please sign in to comment.