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

938 Fixed Device UA being empty on First Bid Request #957

Merged

Conversation

jsligh
Copy link
Collaborator

@jsligh jsligh commented Feb 23, 2024

Fixes #938

Removes the race condition and uses UIWebView instead of WKWebView. UIWebView does javascript calls synchronously while WKWebView does javascript asynchronously which is why the race condition is occurring. While synchronous execution might take an extra second, it prevents a lot of headaches and dispatch_once makes sure this only happens once. The UserAgent is also cached in the shared PBMUserAgentService.

@jsligh jsligh self-assigned this Feb 23, 2024
@jsligh
Copy link
Collaborator Author

jsligh commented Mar 5, 2024

Issue:

I am actually quite stumped on this because of the following:

The current implementation MUST include a completion handler in WKWebView.evaluateJavascript but we can't wait for it to complete because synchronously waiting on the main thread will block the completion handler from ever executing.

self.userAgent = [self.webView valueForKey:@"userAgent"]; while synchronous, is an undocumented API which we cannot guarantee will always work/be correct.

We cannot use ASYNC AWAIT or a similar concurrent Swift function as it is too new (iOS 15+). This will not work as the minimum target version for GMA SDK is iOS 12.0.

Proposed Solutions:

  1. Convert PBMUserAgentService to Swift and use ASYNC AWAIT for clients > iOS 15.0 and keep the current method for clients on older versions.
  • Pushback: doesn't solve problem for users on anything previous to iOS 15.0 (release date 9/20/2021).
  1. Rewrite implementation and put in some kind of wait for all clients.
  • Pushback: could make the code unnecessarily complicated and cause a slowdown on the initialization of PBMUserAgentService.

@@ -68,6 +70,13 @@ - (instancetype)init {
#pragma mark - Public Methods

- (nonnull NSString *)getFullUserAgent {
if (self.userAgent == nil) {
self.uaSemaphore = dispatch_semaphore_create(0);
Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI Apr 4, 2024

Choose a reason for hiding this comment

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

If two ad units call this method at the same time before ua is obtained, the semaphore will be recreated, with unpredictable consequences

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about using semaphore here. It is a tool dedicated to multithreading access to resources. But we need just to lock the process for any resource consumer. So, for me, using NSLock will be much more convenient here. It will let us avoid using the while loop for waiting for the resource. But we still need to be sure of proper unlocking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved the instantiation of the semaphore. The larger issue is that there are multiple threads at work here:

  1. The thread that asks for the userAgent (that has to be the main thread because only the main thread can reference webView.evaluateJavaScript)
  2. The thread where self.webView.evaluateJavaScript runs which is run on a background thread
  3. The evaluateJavaScript completionHandler which is executed on the main thread.

So we cannot use NSLock here because we can't lock the main thread as the completionHandler has to execute.

Copy link
Contributor

Choose a reason for hiding this comment

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

NSLock doesn't lock anything itself. But yes, the requirement that lock() and unlock() should be called on the same thread makes it hardly usable in this situation.

The current implementation looks good. However, this loop looks potentially endless. In the case when the semaphore will be actually incremented before the start of the loop, the wait will always be finished with a timeout. Does it make sense to add an extra check for userAgent inside the loop or limit the number of possible loops?

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

LGTM

@jsligh jsligh merged commit 36ea594 into master Apr 17, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

device.ua always empty in the first prebid bid request
2 participants