Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Network: fix after "Listen to default network changes" #23313

Merged
merged 1 commit into from
May 23, 2023

Conversation

thexai
Copy link
Member

@thexai thexai commented May 21, 2023

Description

Network: fix after "Listen to default network changes"

Motivation and context

On Shield, at least, #23059 has broken the network because m_defaultInterface can be nullptr in some cases:

Callbacks only are received after network changes (lost, recovered, new) but not when app starts. This PR enables fallback to old enumeration method when m_defaultInterface is not yet initialized.

How has this been tested?

Runtime tested Shield 2019

What is the effect on users?

Fix network not usable. The first symptom is that the web server for remote control fails to initialize when Kodi starts up.

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@thexai thexai added Type: Fix non-breaking change which fixes an issue Platform: Android Component: Network v21 Omega labels May 21, 2023
@thexai thexai added this to the Omega 21.0 Alpha 2 milestone May 21, 2023
@joseluismarti
Copy link
Contributor

It's strange, the constructor registers the CNetworkAndroid class and immediately a notification is received (onAvailable callback) initializing m_defaultInterface.

Later, the first call to GetFirstConnectedInterface() is received, it should not give any issues, let's do more tests.

@fritsch
Copy link
Member

fritsch commented May 21, 2023

This "immediately" is not atomic. Therefore a timespan exists where this ptrs is null.

@joseluismarti
Copy link
Contributor

joseluismarti commented May 21, 2023

The problem is that there are calls to CNetworkAndroid::GetInterfaceList() and m_interfaces is not initialized (as it would not be necessary in Android).

With the code proposed by @thexai it's fixed, later on we will have to see these calls and if it makes sense to get all the interfaces.

@thexai
Copy link
Member Author

thexai commented May 21, 2023

There are at least two problems:

  • CNetworkBase::GetInterfaceList is core API (common to all platforms) and should be available also in Android.
  • This code is NOT called on Shield when Kodi starts up:
    void CNetworkAndroid::onAvailable(const CJNINetwork n)
    {
    CLog::Log(LOGDEBUG, "CNetworkAndroid::onAvailable The default network is now: {}", n.toString());
    CJNIConnectivityManager connman{CXBMCApp::getSystemService(CJNIContext::CONNECTIVITY_SERVICE)};
    CJNILinkProperties lp = connman.getLinkProperties(n);
    if (lp)
    {
    CJNINetworkInterface intf = CJNINetworkInterface::getByName(lp.getInterfaceName());
    if (intf)
    m_defaultInterface = std::make_unique<CNetworkInterfaceAndroid>(n, lp, intf);
    }
    }

Only is called when network changes, then m_defaultInterface never is initialized in normal conditions (no network changes on wired permanent Ethernet).

@@ -279,7 +279,7 @@ CNetworkInterface* CNetworkAndroid::GetFirstConnectedInterface()
{
std::unique_lock<CCriticalSection> lock(m_refreshMutex);

if (CJNIBase::GetSDKVersion() >= 24)
if (CJNIBase::GetSDKVersion() >= 24 && m_defaultInterface)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition can be simplified to:

if (m_defaultInterface)

because in SDK < 24 guarantees that m_defaultInterface never is initialized then is redundant check SDK here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, more simple

@joseluismarti
Copy link
Contributor

  • This code is NOT called on Shield when Kodi starts up:

Startup log in Android Emulator and the same in FireTV:

D Kodi    : 2023-05-21 19:29:45.130 T:30110   debug <general>: virtual void CXBMCApp::onGainFocus(): 
D Kodi    : 2023-05-21 19:29:45.254 T:30111   debug <general>: CSettings: loaded settings definition from special://xbmc/system/settings/settings.xml
D Kodi    : 2023-05-21 19:29:45.495 T:30111   debug <general>: CSettings: loaded settings definition from special://xbmc/system/settings/android.xml
D Kodi    : 2023-05-21 19:29:45.533 T:30115   debug <general>: Thread Announce start, auto delete: false
D Kodi    : 2023-05-21 19:29:45.562 T:30116   debug <general>: CNetworkAndroid::onAvailable The default network is now: 134
I Kodi    : 2023-05-21 19:29:45.563 T:30111    info <general>: -----------------------------------------------------------------------
I Kodi    : 2023-05-21 19:29:45.566 T:30111    info <general>: Starting Kodi (21.0-ALPHA1 (20.90.101) Git:20230519-aefcb837ab-dirty). Platform: Android ARM 32-bit
...

@thexai
Copy link
Member Author

thexai commented May 21, 2023

Is not only in Shield, this is in Google Pixel 5 with latest nightly (kodi-20230520-2f793e7f-master-arm64-v8a.apk):

Screenshot_20230521-204910

@joseluismarti
Copy link
Contributor

This error message appears when we activate the Web server and occurs because m_interfaces (which contains all the available interfaces) is not initialized, it's the bug and is solved by adding RetrieveInterfaces().

Regardless of this, the device should have network connectivity (see System Info > Network) and m_defaultInterface should be initialized.

I'm fine with the fix, go ahead 👍

@joseluismarti joseluismarti mentioned this pull request May 22, 2023
6 tasks
@thexai thexai linked an issue May 22, 2023 that may be closed by this pull request
6 tasks
@fuzzard fuzzard merged commit 9f0a7b7 into xbmc:master May 23, 2023
2 checks passed
@thexai thexai deleted the network-fallback branch May 23, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

http web server won't start
4 participants