iOS-sim debug: WebCoreNSURLSessionTest.BasicOperation and WebCoreNSURLSessionTest.InvalidateEmpty asserting

[Ryan Haddad]

iOS-sim debug: WebCoreNSURLSessionTest.BasicOperation and WebCoreNSURLSessionTest.InvalidateEmpty asserting

Tests that failed:
  WebCoreNSURLSessionTest.BasicOperation
  WebCoreNSURLSessionTest.InvalidateEmpty

ASSERTION FAILED: s_mainRunLoop
/Volumes/Data/slave/ios-simulator-9-debug/build/Source/WTF/wtf/RunLoop.cpp(72) : static bool WTF::RunLoop::isMain()
1   0x10c6dee40 WTFCrash
2   0x10c72bd52 WTF::RunLoop::isMain()
3   0x1123f571d WebCore::applicationBundleIdentifier()
4   0x1123f5901 WebCore::applicationBundleIsEqualTo(WTF::String const&)
5   0x1123f6355 WebCore::IOSApplication::isTheEconomistOnIphone()
6   0x11c8e8820 +[WebView(WebPrivate) enableWebThread]
7   0x11c8d91f6 WebKitInitialize
8   0x11b30cf61 ___UIApplicationLoadWebKit_block_invoke
9   0x119dcc4a7 _dispatch_client_callout
10  0x119db9c7b dispatch_once_f
11  0x118f54bff _class_initialize
12  0x118f5ad23 lookUpImpOrForward
13  0x118f698bb objc_msgSend
14  0x10b21b608 TestWebKitAPI::WebKit1_AudioSessionCategoryIOS_Test::TestBody()
15  0x10b441a8a testing::Test::Run()
16  0x10b44236d testing::internal::TestInfoImpl::Run()
17  0x10b442f0d testing::TestCase::Run()
18  0x10b448e92 testing::internal::UnitTestImpl::RunAllTests()
19  0x10b448b09 testing::UnitTest::Run()
20  0x10b35f6ee TestWebKitAPI::TestsController::run(int, char**)
21  0x10b3d6065 main
22  0x119dfb92d start

<https://build.webkit.org/builders/Apple%20iOS%209%20Simulator%20Debug%20WK2%20(Tests)/builds/89/steps/run-api-tests/logs/stdio>

[Alexey Proskuryakov]

static String& applicationBundleIdentifier()
{
    ASSERT(RunLoop::isMain());

    static NeverDestroyed<String> identifier([[NSBundle mainBundle] bundleIdentifier]);

    return identifier;
}

Main run loop is never set up by iOS WebKit1, and it doesn't make a lot of sense there.

I think that it is a bug in application check code that it tries to use NSBundle from the web thread (the test crashes in main thread, but I think that this code would run the on web thread normally).

[Chris Dumez]

Oh, I added this assertion recently and did not realize this would be an issue on WK1.

[Chris Dumez]

Very likely a regression from <http://trac.webkit.org/changeset/197628>

[Chris Dumez]

(In reply to comment #3)
> Very likely a regression from <http://trac.webkit.org/changeset/197628>

So before r197628, we were always using [[NSBundle mainBundle] bundleIdentifier] directly on iOS (no matter the thread). So I propose the restore this behavior as follows:
diff --git a/Source/WebCore/platform/RuntimeApplicationChecks.mm b/Source/WebCore/platform/RuntimeApplicationChecks.mm
index b76bb2e..b2583bd 100644
--- a/Source/WebCore/platform/RuntimeApplicationChecks.mm
+++ b/Source/WebCore/platform/RuntimeApplicationChecks.mm
@@ -28,7 +28,7 @@
 
 #import <Foundation/NSBundle.h>
 #import <wtf/NeverDestroyed.h>
-#import <wtf/RunLoop.h>
+#import <wtf/MainThread.h>
 #import <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -39,7 +39,7 @@ namespace WebCore {
 // the WK2 UIProcess.
 static String& applicationBundleIdentifier()
 {
-    ASSERT(RunLoop::isMain());
+    ASSERT(isMainThread());
 
     static NeverDestroyed<String> identifier([[NSBundle mainBundle] bundleIdentifier]);
 
@@ -53,6 +53,10 @@ void setApplicationBundleIdentifier(const String& bundleIdentifier)
 
 static bool applicationBundleIsEqualTo(const String& bundleIdentifierString)
 {
+#if USE(WEB_THREAD)
+    if (isWebThread())
+        return [[NSBundle mainBundle] bundleIdentifier] == bundleIdentifierString;
+#endif
     return applicationBundleIdentifier() == bundleIdentifierString;
 }

I will test before uploading but let me know if you have any thoughts.

If we are in the WebThread, then we are using WK1 and therefore we don't care about the applicationBundleIdentifier(), main bundle identifier does the right thing.

For other cases (WK2 or WK1 main thread), we keep using the static applicationBundleIdentifier.

[Alexey Proskuryakov]

*** Bug 155275 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Created attachment 273674 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 273674 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273674&action=review

> Source/WebCore/platform/RuntimeApplicationChecks.mm:42
> -    ASSERT(RunLoop::isMain());
> +    ASSERT(isMainThread());

I'm not sure if this change is right. This assertion use to say that we are only using NSBundle for the main thread, which is the right thing to require given that NSBundle is not thread safe.

isMainThread() is a lot more complicated beast, and I don't really see how it verifies anything useful.

[Alexey Proskuryakov]

And if we are concerned about thread safety of WebCore code here (as opposed to NSBundle), then this function can use dispatch_once. I actually don't know if we enable thread-safe statics.

[Alexey Proskuryakov]

We compile with -fno-threadsafe-statics. Also, as Chris pointed out in person, String is not thread safe anyway.

[Chris Dumez]

Comment on attachment 273674 [details]
Patch

After discussing this with Alexey, I have decided to take another stab at this.

[Chris Dumez]

Created attachment 273741 [details]
WIP Patch

[Chris Dumez]

Created attachment 273742 [details]
WIP Patch

[Chris Dumez]

I have a better / simpler idea. I will upload another patch shortly.

[Chris Dumez]

Created attachment 273747 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 273747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273747&action=review

> Source/WebCore/platform/RuntimeApplicationChecks.mm:37
> +static bool wasApplicationBundleIdentifierOverrideQueried = false;

Nitpick: I'd name this applicationBundleIdentifierOverrideWasQueried, so that "if (applicationBundleIdentifierOverrideWasQueried)" reads like a sentence.

> Source/WebCore/platform/RuntimeApplicationChecks.mm:54
> +    const auto& identifier = applicationBundleIdentifierOverride();

Will this be true?

ASSERT(identifier.isNull() || RunLoop::isMain());

[Chris Dumez]

Created attachment 273749 [details]
Patch

[Chris Dumez]

Comment on attachment 273747 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273747&action=review

>> Source/WebCore/platform/RuntimeApplicationChecks.mm:37
>> +static bool wasApplicationBundleIdentifierOverrideQueried = false;
> 
> Nitpick: I'd name this applicationBundleIdentifierOverrideWasQueried, so that "if (applicationBundleIdentifierOverrideWasQueried)" reads like a sentence.

Done.

>> Source/WebCore/platform/RuntimeApplicationChecks.mm:54
>> +    const auto& identifier = applicationBundleIdentifierOverride();
> 
> Will this be true?
> 
> ASSERT(identifier.isNull() || RunLoop::isMain());

It should. I added it.

[WebKit Commit Bot]

Comment on attachment 273749 [details]
Patch

Clearing flags on attachment: 273749

Committed r198040: <http://trac.webkit.org/changeset/198040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.