Skip to content
This repository was archived by the owner on May 28, 2025. It is now read-only.

Removing an unnecessary call to run UI appliation on the main thread.#1937

Closed
pradipd wants to merge 0 commit into
microsoft:developfrom
pradipd:develop
Closed

Removing an unnecessary call to run UI appliation on the main thread.#1937
pradipd wants to merge 0 commit into
microsoft:developfrom
pradipd:develop

Conversation

@pradipd

@pradipd pradipd commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

Ui application initialize already forces itself to run on the main thread.

This is issue was discovered when testing #1872.
FunctionalTestSetupUIApplication calls
RunSynchronouslyOnMainThread calls

which looks like:
inline void RunSynchronouslyOnMainThread(void (^block)()) {
if ([NSThread isMainThread]) {
block(); //block = UIApplicationInitializeFunctionalTest
} else {
dispatch_sync(dispatch_get_main_queue(), block); //block = UIApplicationInitializeFunctionalTest
}

The problem is isMainThread is not properly initialize till after UIApplicationInitializeFunctionalTest is called.

Up until now, we have just been lucky NSThread isMainThread returned true. Even though, in functional tests (TAEF) we know we are not on the main thread. If you look at isMainThread.

  • (BOOL)isMainThread {
    return [self _threadObjectFromCurrentThread] == [self mainThread];
    }
    this evaluates to nil == nil for functional tests before UIApplicationInitializeFunctionalTest is called.

But, our functional tests have always worked because UIApplicationInitializeFunctionalTest itself already takes care of calling the important pieces on the main thread.

So, the call to RunSynchronouslyOnMainThread in FunctionalTestSetupUIApplication seems redundant since UIApplicationInitializeFunctionalTest already knows how to initialize itself.

And, with #1872, this caused the FT's to hang, because isMainThread returns false (which is correct!), but, now we call libdispatch to get the main queue, but that hasn't been setup yet because UIApplicationInitializeFunctionalTest.

tl;dr
Call UIApplicationInitializeFunctionalTest before relying on any objective-c thread related activities.

Additional Validation:
I also tested this change with #1872 and confirmed the PR worked.

@pradipd

pradipd commented Feb 9, 2017

Copy link
Copy Markdown
Contributor Author

@jaredhms is added to the review. #Closed

@pradipd

pradipd commented Feb 9, 2017

Copy link
Copy Markdown
Contributor Author

@rajsesh-msft is added to the review. #Closed


// Launches the functional test app
bool FunctionalTestSetupUIApplication() {
RunSynchronouslyOnMainThread(^{

@jaredhms jaredhms Feb 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RunSynchronouslyOnMainThread [](start = 4, length = 28)

Maybe we should be using CoreDispatcher APIs rather than NSThread, etc for this? How can you be positive that FunctionalTestSetupUIApplication is getting called from the UI thread? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I guarantee that it is not being called on the UI thread.

UIApplicationInitializeFunctionalTest uses Coredispatcher to call the important parts on the UI thread.


In reply to: 100440302 [](ancestors = 100440302)

UIApplicationInitializeFunctionalTest(nullptr, Strings::NarrowToWide<std::wstring>(NSStringFromClass([AppDelegate class])).c_str());
});

UIApplicationInitializeFunctionalTest(nullptr, Strings::NarrowToWide<std::wstring>(NSStringFromClass([AppDelegate class])).c_str());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

UIApplicationInitializeFunctionalTest(nullptr, St [](start = 0, length = 53)

Ah! in that case, please add a detailed comment so we don't do this again

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is that what those // are for?
Will do.


In reply to: 100440979 [](ancestors = 100440979)

@jaredhms

jaredhms commented Feb 9, 2017

Copy link
Copy Markdown
Contributor

:shipit:

@pradipd pradipd closed this Feb 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants