Always check the return value of dlopen() and dlsym() in Release builds. This would have caught a soft linking error sooner for an internal issue I was debugging.
Created attachment 271601 [details] Patch v1
Comment on attachment 271601 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=271601&action=review > Source/WebCore/platform/mac/SoftLinking.h:37 > + static void* dylib = ^{ \ What is the purpose of adding the blocks?
Comment on attachment 271601 [details] Patch v1 Oops, that's actually pretty clear without explanation.
Comment on attachment 271601 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=271601&action=review > Source/WebCore/platform/mac/SoftLinking.h:324 > ASSERT_WITH_MESSAGE_UNUSED(isOptional, isOptional || frameworkLibrary, "%s", dlerror()); \ Do we still need this assert?
Comment on attachment 271601 [details] Patch v1 Clearing flags on attachment: 271601 Committed r196744: <http://trac.webkit.org/changeset/196744>
All reviewed patches have been landed. Closing bug.
Comment on attachment 271601 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=271601&action=review >> Source/WebCore/platform/mac/SoftLinking.h:37 >> + static void* dylib = ^{ \ > > What is the purpose of adding the blocks? I saw you answered your own question, but for posterity, this is so we can use a RELEASE_ASSERT_WITH_MESSAGE that only fires once instead of each time the method is called. >> Source/WebCore/platform/mac/SoftLinking.h:324 >> ASSERT_WITH_MESSAGE_UNUSED(isOptional, isOptional || frameworkLibrary, "%s", dlerror()); \ > > Do we still need this assert? Oops! No, we can remove this. Will fix in a followup. Good catch!
(In reply to comment #5) > Comment on attachment 271601 [details] > Patch v1 > > Clearing flags on attachment: 271601 > > Committed r196744: <http://trac.webkit.org/changeset/196744> Follow-up fix for Andy's comment: Committed r196763: < http://trac.webkit.org/changeset/196763>
A couple more questions: 1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in SOFT_LINK_CLASS? Seems like that case would also cause some mysterious crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs. 2. Shouldn't we use dispatch_once/std::call_once to initialize these static locals? Since soft-linked symbols are lazy-loaded, and we often use macros that look like the real symbol names, it would be easy to accidentally introduce a race condition. For instance: #import "QuickLookSoftLink.h" NSURL *url = ...; NSURLSession *session = [NSURLSession sharedSession]; [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse *response, NSError *) { // This is called on the session's delegate queue if (response.URL.scheme isEqualToString:QLPreviewScheme]) ... }]; NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal pointer value and function name, but they're really macros that expand to functions that will ultimately both call QuickLookLibrary() if the framework hasn't already been loaded.
This change caused: Bug 154703: REGRESSION (r196744): NetworkExtension.framework and NEFilterSource class are not available on Recovery partition <https://bugs.webkit.org/show_bug.cgi?id=154703>
(In reply to comment #9) > A couple more questions: > > 1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in > SOFT_LINK_CLASS? Seems like that case would also cause some mysterious > crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs. Committed r197149: <http://trac.webkit.org/changeset/197149> > 2. Shouldn't we use dispatch_once/std::call_once to initialize these static > locals? Since soft-linked symbols are lazy-loaded, and we often use macros > that look like the real symbol names, it would be easy to accidentally > introduce a race condition. For instance: > > #import "QuickLookSoftLink.h" > > NSURL *url = ...; > NSURLSession *session = [NSURLSession sharedSession]; > [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse > *response, NSError *) { > // This is called on the session's delegate queue > if (response.URL.scheme isEqualToString:QLPreviewScheme]) > ... > }]; > > NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread > > QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal > pointer value and function name, but they're really macros that expand to > functions that will ultimately both call QuickLookLibrary() if the framework > hasn't already been loaded. QLPreviewScheme and QLPreviewGetSupportedMIMETypes are initialized using dispatch_once in SOFT_LINK_POINTER_FOR_SOURCE() and SOFT_LINK_FUNCTION_FOR_SOURCE() macros. Are you suggesting the non *_FOR_SOURCE() versions of the macros need dispatch_once versions? That would be SOFT_LINK_POINTER() and SOFT_LINK(), as well as: SOFT_LINK_MAY_FAIL() SOFT_LINK_OPTIONAL() SOFT_LINK_CLASS() SOFT_LINK_CLASS_OPTIONAL() SOFT_LINK_POINTER_OPTIONAL() SOFT_LINK_CONSTANT() SOFT_LINK_CONSTANT_MAY_FAIL()
(In reply to comment #11) > (In reply to comment #9) > > A couple more questions: > > > > 1. Should we also RELEASE_ASSERT that objc_getClass() doesn't return nil in > > SOFT_LINK_CLASS? Seems like that case would also cause some mysterious > > crashes (e.g. uncaught NSInvalidArgumentExceptions) and bugs. > > Committed r197149: <http://trac.webkit.org/changeset/197149> Thanks! > > > 2. Shouldn't we use dispatch_once/std::call_once to initialize these static > > locals? Since soft-linked symbols are lazy-loaded, and we often use macros > > that look like the real symbol names, it would be easy to accidentally > > introduce a race condition. For instance: > > > > #import "QuickLookSoftLink.h" > > > > NSURL *url = ...; > > NSURLSession *session = [NSURLSession sharedSession]; > > [session dataTaskWithURL:url completionHandler:^(NSData *, NSURLResponse > > *response, NSError *) { > > // This is called on the session's delegate queue > > if (response.URL.scheme isEqualToString:QLPreviewScheme]) > > ... > > }]; > > > > NSSet *mimeTypes = QLPreviewGetSupportedMIMETypes(); // main thread > > > > QLPreviewScheme and QLPreviewGetSupportedMIMETypes() look like a normal > > pointer value and function name, but they're really macros that expand to > > functions that will ultimately both call QuickLookLibrary() if the framework > > hasn't already been loaded. > > QLPreviewScheme and QLPreviewGetSupportedMIMETypes are initialized using > dispatch_once in SOFT_LINK_POINTER_FOR_SOURCE() and > SOFT_LINK_FUNCTION_FOR_SOURCE() macros. I guess I chose a bad example, then :) > > Are you suggesting the non *_FOR_SOURCE() versions of the macros need > dispatch_once versions? That would be SOFT_LINK_POINTER() and SOFT_LINK(), > as well as: > > SOFT_LINK_MAY_FAIL() > SOFT_LINK_OPTIONAL() > SOFT_LINK_CLASS() > SOFT_LINK_CLASS_OPTIONAL() > SOFT_LINK_POINTER_OPTIONAL() > SOFT_LINK_CONSTANT() > SOFT_LINK_CONSTANT_MAY_FAIL() Yes, the init##...() functions in these macros need dispatch_once to protect the setting of the static function pointer. Also, SOFT_LINK_LIBRARY() and all variants of SOFT_LINK_FRAMEWORK() need to use dispatch_once when initializing their static void*s.