Summary: | iOS-sim debug: WebCoreNSURLSessionTest.BasicOperation and WebCoreNSURLSessionTest.InvalidateEmpty asserting | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryan Haddad <ryanhaddad> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | achristensen, andersca, ap, cdumez, commit-queue, darin, dbates, ddkilzer, jer.noble | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Bug Depends on: | |||||||||||||||
Bug Blocks: | 155035 | ||||||||||||||
Attachments: |
|
Description
Ryan Haddad
2016-03-09 13:38:09 PST
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). Oh, I added this assertion recently and did not realize this would be an issue on WK1. Very likely a regression from <http://trac.webkit.org/changeset/197628> (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. *** Bug 155275 has been marked as a duplicate of this bug. *** Created attachment 273674 [details]
Patch
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. 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. We compile with -fno-threadsafe-statics. Also, as Chris pointed out in person, String is not thread safe anyway. Comment on attachment 273674 [details]
Patch
After discussing this with Alexey, I have decided to take another stab at this.
Created attachment 273741 [details]
WIP Patch
Created attachment 273742 [details]
WIP Patch
I have a better / simpler idea. I will upload another patch shortly. Created attachment 273747 [details]
Patch
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()); Created attachment 273749 [details]
Patch
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. Comment on attachment 273749 [details] Patch Clearing flags on attachment: 273749 Committed r198040: <http://trac.webkit.org/changeset/198040> All reviewed patches have been landed. Closing bug. |