Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

denysoblohin-okta · 2024-12-13T15:47:42Z

Fixes bug in Mobile Safari 18.0-18.2 for poll requests.

We should wait ~500ms after document become visible before making poll requests.
If fetch still failed with error Load failed, restart fetch request.

lib/authn/util/poll.ts

lesterchoi-okta · 2024-12-17T16:19:50Z

lib/authn/util/poll.ts

+          const isNetworkError = isAuthApiError(err) && err.message === 'Load failed';
+          const isTimeout = isAuthApiError(err) && err.message === 'Load timeout';


Looks brittle. Check that this works cross-browser (at least doesn't cause regression)

This check for Load failed message is intended to work only in Safari

lib/constants.ts

lesterchoi-okta · 2024-12-19T19:27:54Z

lib/http/request.ts

+
+    // Restarts fetch on 'Load failed' error
+    // This error can occur when `fetch` does not respond
+    //  (due to CORS error, non-existing host, or network error)


Is there a way to differentiate between/among these types of errors?

Seems to me isNetworkError will be true in all those cases and retry the fetch, although we don't want to retry on CORS, non-existent host, etc.

LMK if I'm misreading/misunderstanding the behavior

Google search results for "safari types of network error"

Google search results for "chrome devtools types of network error"

Probably good to add some tests to ensure we don't catch and retry CORS errors, etc

You're correct, isNetworkError will be true in all those 3 cases.
There is no way to distinguish programmatically in JS.
(The difference can be seen only in browser console)

That's why I added canRetry option to enable retry logic only for polling requests.
Polling requests are repeatable by design so I think it's fine to retry it due to real network loss.

Since all API requests should be performed to the same host and poll request is never a first request in a list of ones performed by SIW, possible CORS error should be caught before polling is started.

Thanks. Is there any logic to say we should stop trying polling after n failed attempts?

jaredperreault-okta · 2024-12-19T20:19:57Z

lib/http/request.ts

-  return sdk.options.httpRequestClient!(method!, url!, ajaxOptions)
+  var err, res, promise;
+
+  if (isIOS()) {


My initial research/assumptions about browser version detection (I seemed to have confused it with OS version) were wrong. We can actually determine the browser version. This is a library that does the parsing: https://www.npmjs.com/package/ua-parser-js

I think it's fine to use it or use it as a reference. Either way we should gate this by the specific browser versions 18.x

Done
793382c#diff-b640bdcca3985bce5a6f136070067d742a100754b211417c74125f85a9be47b2R100

jaredperreault-okta · 2024-12-19T20:21:44Z

lib/idx/idxState/v1/generateIdxAction.ts

        url: target,
        method: actionDefinition.method,
        headers,
        args: body,
        withCredentials: toPersist?.withCredentials ?? true
-      });
+      };
+      const isPolling = target.endsWith('/poll');


I'm not sure we can guarantee all IDX polling flows utilize paths that end in /poll

changed to

const isPolling = actionDefinition.name === 'poll' || actionDefinition.name?.endsWith('-poll');

https://github.com/okta/okta-signin-widget/blob/2cdaecd7f607f16875223652550cf603e476884b/src/v3/src/hooks/usePolling.ts#L36

jaredperreault-okta · 2024-12-19T20:22:25Z

lib/http/request.ts

@@ -113,7 +128,8 @@ export function httpRequest(sdk: OktaAuthHttpInterface, options: RequestOptions)
      withCredentials = options.withCredentials === true, // default value is false
      storageUtil = sdk.options.storageUtil,
      storage = storageUtil!.storage,
-      httpCache = sdk.storageManager.getHttpCache(sdk.options.cookies);
+      httpCache = sdk.storageManager.getHttpCache(sdk.options.cookies),
+      canRetry = options.canRetry;


Can you add an additional option here to gate this logic? Something like pollingIntent: true/false (defaults to false). Then change L163 to

if (pollingIntent && isIOS() { ... }

Both the poll function of authn and the SIW polling methods would need to pass this flag

.

awaken clean

.

lesterchoi-okta · 2024-12-20T21:41:50Z

lib/features.ts

+  if (isBrowser() && typeof navigator !== 'undefined' && typeof navigator.userAgent !== 'undefined') {
+    // eslint-disable-next-line new-cap
+    const { browser, os } = UAParser(navigator.userAgent);


Does UAParser not have these kinds of safety checks? If you wrote it this way because to navigator.userAgent "might be undefined", let UAParser find the user-agent string? The new will also avoid the need for the eslint annotation

const { browser, os } = new UAParser().getResult();

I think your implementation/invocation is useful when you're running it in a node context or analyzing data.

lesterchoi-okta · 2024-12-20T21:44:23Z

lib/http/request.ts

+
+    // Restarts fetch on 'Load failed' error
+    // This error can occur when `fetch` does not respond
+    //  (due to CORS error, non-existing host, or network error)


Thanks. Is there any logic to say we should stop trying polling after n failed attempts?

jaredperreault-okta reviewed Dec 13, 2024

View reviewed changes

lib/authn/util/poll.ts Outdated Show resolved Hide resolved

lesterchoi-okta reviewed Dec 17, 2024

View reviewed changes

denysoblohin-okta force-pushed the od-fix-polling-in-bg branch 2 times, most recently from c8d0649 to bd83de1 Compare December 19, 2024 15:09

lesterchoi-okta reviewed Dec 19, 2024

View reviewed changes

jaredperreault-okta reviewed Dec 19, 2024

View reviewed changes

denysoblohin-okta added 21 commits December 20, 2024 12:34

Retry on network error

0cac2d7

Add timeout

ee1d17e

Use same logic for authn and idx

000c5cb

revert

b089ffc

.

wip

da33d79

awaken clean

clean

264d775

comments

2f186ee

clean

753c410

fix test

6fdf33a

lint fix

4071797

fix tests

edd5bb5

fix test

cda254e

add test

96bedc3

fix test server

040bf62

fix lint

bd82c58

tris canRetry is excess

6bed4f6

pollingIntent

8d9c2b2

isPolling

1a04106

not necessary isIOS check

f3d0b08

check pollingIntent for all new logic

0277135

test fix

6f39061

denysoblohin-okta force-pushed the od-fix-polling-in-bg branch from 0f4536c to 6f39061 Compare December 20, 2024 10:35

denysoblohin-okta added 2 commits December 20, 2024 13:15

add test

54a6964

isMobileSafari18

793382c

.

denysoblohin-okta changed the title ~~Fix: restart poll requests that fail to fetch in background on iOS~~ Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken Dec 20, 2024

denysoblohin-okta added 2 commits December 20, 2024 16:06

lint fix

b85ea1e

lint fix

d17ad79

denysoblohin-okta requested review from jaredperreault-okta and lesterchoi-okta December 20, 2024 14:28

lesterchoi-okta reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

denysoblohin-okta commented Dec 13, 2024 •

edited

Loading

lesterchoi-okta Dec 17, 2024

denysoblohin-okta Dec 20, 2024

lesterchoi-okta Dec 19, 2024

lesterchoi-okta Dec 19, 2024

jaredperreault-okta Dec 19, 2024

denysoblohin-okta Dec 20, 2024 •

edited

Loading

lesterchoi-okta Dec 20, 2024

jaredperreault-okta Dec 19, 2024

denysoblohin-okta Dec 20, 2024

jaredperreault-okta Dec 19, 2024

denysoblohin-okta Dec 20, 2024 •

edited

Loading

jaredperreault-okta Dec 19, 2024 •

edited

Loading

jaredperreault-okta Dec 19, 2024

denysoblohin-okta Dec 20, 2024

lesterchoi-okta Dec 20, 2024

lesterchoi-okta Dec 20, 2024

		const isNetworkError = isAuthApiError(err) && err.message === 'Load failed';
		const isTimeout = isAuthApiError(err) && err.message === 'Load timeout';

Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

Are you sure you want to change the base?

Fix bug in Mobile Safari 18.x: start poll request when document is visible and awaken #1552

Conversation

denysoblohin-okta commented Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denysoblohin-okta Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denysoblohin-okta Dec 20, 2024 • edited Loading

Choose a reason for hiding this comment

jaredperreault-okta Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denysoblohin-okta commented Dec 13, 2024 •

edited

Loading

denysoblohin-okta Dec 20, 2024 •

edited

Loading

denysoblohin-okta Dec 20, 2024 •

edited

Loading

jaredperreault-okta Dec 19, 2024 •

edited

Loading