Skip to content

Commit 348f53c

Browse files
jkmasselclaude
jkmassel
and
claude
committed
Fix ANR risk and error handling in WP.com OAuth login
- Replace runBlocking with async coroutine scope to avoid blocking the main thread during token exchange (Critical #1) - Use dedicated CoroutineScope instead of Dispatchers.IO so dispose() only cancels this helper's coroutines (Critical #2) - Propagate login errors to the caller via Consumer<Exception> instead of swallowing them - Show error state on the loading screen with a retry button instead of silently failing - Cancel coroutine scope in LoginActivity.onDestroy() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 61edbf9 commit 348f53c

File tree

3 files changed

+145
-36
lines changed

3 files changed

+145
-36
lines changed

‎WordPress/src/main/java/org/wordpress/android/ui/accounts/LoginActivity.java‎

Lines changed: 62 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,10 @@
66
import android.content.Intent;
77
import android.net.Uri;
88
import android.os.Bundle;
9+
import android.view.View;
910
import android.view.ViewGroup;
1011
import android.widget.FrameLayout;
12+
import android.widget.TextView;
1113

1214
import androidx.annotation.NonNull;
1315
import androidx.core.view.WindowCompat;
@@ -125,15 +127,16 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {
125127
// Enable edge-to-edge display
126128
WindowCompat.setDecorFitsSystemWindows(getWindow(), false);
127129

128-
// Attempt Login if this activity was created in response to a user confirming login, and if
129-
// successful clear the intent so we don't reuse the OAuth code if the activity is recreated
130-
boolean loginProcessed = mLoginHelper.tryLoginWithDataString(getIntent().getDataString());
131-
132-
if (loginProcessed) {
130+
// Attempt Login if this activity was created in response to a user confirming login
131+
String dataString = getIntent().getDataString();
132+
if (mLoginHelper.hasOAuthCallback(dataString)) {
133133
getIntent().setData(null);
134-
// OAuth login successful - show loading UI and finish the login flow
135134
setContentView(R.layout.login_loading);
136-
this.loggedInAndFinish(new ArrayList<Integer>(), true);
135+
mLoginHelper.tryLoginWithDataString(
136+
dataString,
137+
() -> loggedInAndFinish(new ArrayList<>(), true),
138+
error -> showLoginError(error)
139+
);
137140
return;
138141
} else {
139142
// Not an OAuth callback - clear any pending login mode from a previous flow
@@ -222,11 +225,15 @@ protected void onNewIntent(@NonNull Intent intent) {
222225
setIntent(intent);
223226

224227
// Handle OAuth callback when activity is reused (singleTop)
225-
boolean loginProcessed = mLoginHelper.tryLoginWithDataString(intent.getDataString());
226-
if (loginProcessed) {
228+
String dataString = intent.getDataString();
229+
if (mLoginHelper.hasOAuthCallback(dataString)) {
227230
intent.setData(null);
228231
setContentView(R.layout.login_loading);
229-
this.loggedInAndFinish(new ArrayList<Integer>(), true);
232+
mLoginHelper.tryLoginWithDataString(
233+
dataString,
234+
() -> loggedInAndFinish(new ArrayList<>(), true),
235+
error -> showLoginError(error)
236+
);
230237
}
231238
}
232239

@@ -242,6 +249,12 @@ protected void onStop() {
242249
mDispatcher.unregister(this);
243250
}
244251

252+
@Override
253+
protected void onDestroy() {
254+
super.onDestroy();
255+
mLoginHelper.dispose();
256+
}
257+
245258
@Override
246259
protected void onResume() {
247260
super.onResume();
@@ -302,6 +315,45 @@ private void navigateToMainActivityOrFinish() {
302315
finish();
303316
}
304317

318+
private void showPrologueScreen() {
319+
LoginFlowThemeHelper.injectMissingCustomAttributes(getTheme());
320+
FrameLayout fragmentContainer = new FrameLayout(this);
321+
mFragmentContainerId = R.id.fragment_container;
322+
fragmentContainer.setId(mFragmentContainerId);
323+
fragmentContainer.setFitsSystemWindows(false);
324+
fragmentContainer.setLayoutParams(new ViewGroup.LayoutParams(
325+
ViewGroup.LayoutParams.MATCH_PARENT,
326+
ViewGroup.LayoutParams.MATCH_PARENT
327+
));
328+
setContentView(fragmentContainer);
329+
showFragment(
330+
new LoginPrologueRevampedFragment(),
331+
LoginPrologueRevampedFragment.TAG
332+
);
333+
}
334+
335+
private void showLoginError(@NonNull Exception error) {
336+
AppLog.e(T.MAIN, "OAuth login failed", error);
337+
338+
View progressBar = findViewById(R.id.progress_bar);
339+
View loadingText = findViewById(R.id.loading_text);
340+
TextView errorText = findViewById(R.id.error_text);
341+
View retryButton = findViewById(R.id.retry_button);
342+
343+
if (progressBar != null) progressBar.setVisibility(View.GONE);
344+
if (loadingText != null) loadingText.setVisibility(View.GONE);
345+
346+
if (errorText != null) {
347+
errorText.setText(getString(R.string.error_generic_network));
348+
errorText.setVisibility(View.VISIBLE);
349+
}
350+
351+
if (retryButton != null) {
352+
retryButton.setVisibility(View.VISIBLE);
353+
retryButton.setOnClickListener(v -> showPrologueScreen());
354+
}
355+
}
356+
305357
private void showFragment(@NonNull Fragment fragment, @NonNull String tag) {
306358
FragmentTransaction fragmentTransaction = getSupportFragmentManager().beginTransaction();
307359
fragmentTransaction.replace(mFragmentContainerId, fragment, tag);

‎WordPress/src/main/java/org/wordpress/android/ui/accounts/login/WPcomLoginHelper.kt‎

Lines changed: 57 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,56 +3,75 @@ package org.wordpress.android.ui.accounts.login
33
import android.content.ComponentName
44
import android.content.Context
55
import android.net.Uri
6-
import android.util.Log
76
import androidx.browser.customtabs.CustomTabsCallback
87
import androidx.browser.customtabs.CustomTabsClient
98
import androidx.browser.customtabs.CustomTabsServiceConnection
109
import androidx.browser.customtabs.CustomTabsSession
1110
import androidx.core.net.toUri
11+
import kotlinx.coroutines.CoroutineScope
1212
import kotlinx.coroutines.Dispatchers
13+
import kotlinx.coroutines.SupervisorJob
1314
import kotlinx.coroutines.cancel
14-
import kotlinx.coroutines.runBlocking
15+
import kotlinx.coroutines.launch
16+
import kotlinx.coroutines.withContext
1517
import org.wordpress.android.fluxc.network.rest.wpapi.WPcomLoginClient
1618
import org.wordpress.android.fluxc.network.rest.wpcom.auth.AppSecrets
1719
import org.wordpress.android.fluxc.store.AccountStore
20+
import org.wordpress.android.util.AppLog
21+
import org.wordpress.android.util.AppLog.T
1822
import javax.inject.Inject
19-
import kotlin.coroutines.CoroutineContext
2023

2124
class WPcomLoginHelper @Inject constructor(
2225
private val loginClient: WPcomLoginClient,
2326
private val accountStore: AccountStore,
2427
appSecrets: AppSecrets,
2528
) {
26-
private val context: CoroutineContext = Dispatchers.IO
29+
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO)
2730

2831
val wpcomLoginUri = loginClient.loginUri(appSecrets.redirectUri)
2932
private val customTabsServiceConnection = ServiceConnection(wpcomLoginUri)
30-
private var processedAuthData: String? = null
3133

32-
@Suppress("ReturnCount")
33-
fun tryLoginWithDataString(data: String?): Boolean {
34-
if (data == null || data == processedAuthData) {
35-
return false
36-
}
37-
38-
val code = data.toUri().getQueryParameter("code") ?: return false
34+
/**
35+
* Returns true if the data string contains an OAuth callback code.
36+
*/
37+
fun hasOAuthCallback(data: String?): Boolean {
38+
if (data == null) return false
39+
return data.toUri().getQueryParameter("code") != null
40+
}
3941

40-
runBlocking {
41-
val tokenResult = loginClient.exchangeAuthCodeForToken(code)
42-
accountStore.updateAccessToken(tokenResult.getOrThrow())
43-
Log.i("WPCOM_LOGIN", "Login Successful")
42+
/**
43+
* Asynchronously exchanges the OAuth code in the data string for
44+
* an access token. Calls [onSuccess] on the main thread if the
45+
* exchange succeeds, or [onFailure] with the exception if it
46+
* fails.
47+
*/
48+
fun tryLoginWithDataString(
49+
data: String,
50+
onSuccess: Runnable,
51+
onFailure: java.util.function.Consumer<Exception>
52+
) {
53+
val code = data.toUri().getQueryParameter("code") ?: return
54+
55+
scope.launch {
56+
try {
57+
val tokenResult = loginClient
58+
.exchangeAuthCodeForToken(code)
59+
accountStore.updateAccessToken(
60+
tokenResult.getOrThrow()
61+
)
62+
withContext(Dispatchers.Main) { onSuccess.run() }
63+
} catch (e: Exception) {
64+
withContext(Dispatchers.Main) { onFailure.accept(e) }
65+
}
4466
}
45-
46-
processedAuthData = data
47-
return true
4867
}
4968

5069
fun isLoggedIn(): Boolean {
5170
return accountStore.hasAccessToken()
5271
}
5372

5473
fun dispose() {
55-
context.cancel()
74+
scope.cancel()
5675
}
5776

5877
fun bindCustomTabsService(context: Context) {
@@ -62,17 +81,24 @@ class WPcomLoginHelper @Inject constructor(
6281

6382
class ServiceConnection(
6483
var uri: Uri
65-
): CustomTabsServiceConnection() {
84+
) : CustomTabsServiceConnection() {
6685
private var client: CustomTabsClient? = null
6786
private var session: CustomTabsSession? = null
6887

69-
override fun onCustomTabsServiceConnected(name: ComponentName, client: CustomTabsClient) {
88+
override fun onCustomTabsServiceConnected(
89+
name: ComponentName,
90+
client: CustomTabsClient
91+
) {
7092
client.warmup(0)
7193
this.client = client
7294

7395
val session = client.newSession(CustomTabsCallback())
7496
session?.mayLaunchUrl(uri, null, null)
75-
session?.mayLaunchUrl("https://wordpress.com/log-in/".toUri(), null, null)
97+
session?.mayLaunchUrl(
98+
"https://wordpress.com/log-in/".toUri(),
99+
null,
100+
null
101+
)
76102

77103
this.session = session
78104
}
@@ -90,8 +116,13 @@ class ServiceConnection(
90116

91117
// Get the default browser package name, this will be null if
92118
// the default browser does not provide a CustomTabsService
93-
val packageName = CustomTabsClient.getPackageName(context, null) ?: return
94-
95-
CustomTabsClient.bindCustomTabsService(context, packageName, this)
119+
val packageName =
120+
CustomTabsClient.getPackageName(context, null) ?: return
121+
122+
CustomTabsClient.bindCustomTabsService(
123+
context,
124+
packageName,
125+
this
126+
)
96127
}
97128
}

‎WordPress/src/main/res/layout/login_loading.xml‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,30 @@
2626
app:layout_constraintStart_toStartOf="parent"
2727
app:layout_constraintTop_toBottomOf="@id/progress_bar" />
2828

29+
<TextView
30+
android:id="@+id/error_text"
31+
android:layout_width="0dp"
32+
android:layout_height="wrap_content"
33+
android:layout_marginHorizontal="@dimen/margin_extra_large"
34+
android:gravity="center"
35+
android:textAppearance="?attr/textAppearanceBody1"
36+
android:visibility="gone"
37+
app:layout_constraintBottom_toTopOf="@id/retry_button"
38+
app:layout_constraintEnd_toEndOf="parent"
39+
app:layout_constraintStart_toStartOf="parent"
40+
app:layout_constraintTop_toTopOf="parent"
41+
app:layout_constraintVertical_chainStyle="packed" />
42+
43+
<com.google.android.material.button.MaterialButton
44+
android:id="@+id/retry_button"
45+
android:layout_width="wrap_content"
46+
android:layout_height="wrap_content"
47+
android:layout_marginTop="@dimen/margin_large"
48+
android:text="@string/retry"
49+
android:visibility="gone"
50+
app:layout_constraintBottom_toBottomOf="parent"
51+
app:layout_constraintEnd_toEndOf="parent"
52+
app:layout_constraintStart_toStartOf="parent"
53+
app:layout_constraintTop_toBottomOf="@id/error_text" />
54+
2955
</androidx.constraintlayout.widget.ConstraintLayout>

0 commit comments

Comments
 (0)
⚡