REGRESSION (r197149): Missing availability checks when soft-linking DataDetectors.framework

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

The DataDetectors.framework is not available in the Base System on Mac OS X, but the code that soft-linked the framework did not always check to see if the framework was available before trying to use classes, constants and functions from it.

When r197149 landed (as a follow-up fix for Bug 154364), this caused the lack of runtime checking to turn into a release assert because Objective-C classes that were assumed to be available were actually not.

Bug 154364: [Cocoa] Always check the return value of dlopen() and dlsym() in Release builds
<https://bugs.webkit.org/show_bug.cgi?id=154364>
<http://trac.webkit.org/changeset/197149>

[David Kilzer (:ddkilzer)]

Created attachment 273476 [details]
Patch

[David Kilzer (:ddkilzer)]

<rdar://problem/24946426>

[WebKit Commit Bot]

Attachment 273476 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/mac/WebHitTestResultData.mm:82:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/mac/WebHitTestResultData.mm:82:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 273476 [details]
Patch

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

> Source/WebCore/platform/spi/mac/DataDetectorsSPI.h:104
> -SOFT_LINK_CLASS(DataDetectors, DDAction)
> -SOFT_LINK_CLASS(DataDetectors, DDActionContext)
> -SOFT_LINK_CLASS(DataDetectors, DDActionsManager)
> +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDAction)
> +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDActionContext)
> +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDActionsManager)

Not something that needs to be done in this patch, but I wonder if we can make it a compile-time error to non-optionally soft-link things (classes, pointers, functions, etc.) in optionally soft-linked frameworks.

> Source/WebKit2/Shared/mac/WebHitTestResultData.mm:88
> -    RetainPtr<NSKeyedUnarchiver> unarchiver = adoptNS([[NSKeyedUnarchiver alloc] initForReadingWithData:(NSData *)data.get()]);
> -    [unarchiver setRequiresSecureCoding:YES];
> -    @try {
> -        hitTestResultData.detectedDataActionContext = [unarchiver decodeObjectOfClass:getDDActionContextClass() forKey:@"actionContext"];
> -    } @catch (NSException *exception) {
> -        LOG_ERROR("Failed to decode DDActionContext: %@", exception);
> -        return false;
> -    }
> +    if (DataDetectorsLibrary()) {
> +        RetainPtr<NSKeyedUnarchiver> unarchiver = adoptNS([[NSKeyedUnarchiver alloc] initForReadingWithData:(NSData *)data.get()]);
> +        [unarchiver setRequiresSecureCoding:YES];
> +        @try {
> +            hitTestResultData.detectedDataActionContext = [unarchiver decodeObjectOfClass:getDDActionContextClass() forKey:@"actionContext"];
> +        } @catch (NSException *exception) {
> +            LOG_ERROR("Failed to decode DDActionContext: %@", exception);
> +            return false;
> +        }
>      
> -    [unarchiver finishDecoding];
> +        [unarchiver finishDecoding];
> +    }

I think we could do slightly better here. WebHitTestResultData::platformEncode() is going to encode a non-nil value for actionContext unconditionally, so I assume that a non-nil actionContext implies that DataDetectors.framework is present. Given that, you could just ASSERT(DataDetectorsLibrary()) if hasActionContext is true. If my assumption is wrong, then this change is also wrong, because the encoding and decoding will be out of balance.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Attachment 273476 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit2/Shared/mac/WebHitTestResultData.mm:82:  Place brace on
> its own line for function definitions.  [whitespace/braces] [4]
> ERROR: Source/WebKit2/Shared/mac/WebHitTestResultData.mm:82:  Extra space
> before ( in function call  [whitespace/parens] [4]
> Total errors found: 2 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Filed Bug 155273: check-webkit-style: fix false-positive warnings about @try/@catch blocks in Objective-C++ source files
<https://bugs.webkit.org/show_bug.cgi?id=155273>

[David Kilzer (:ddkilzer)]

(In reply to comment #4)
> Comment on attachment 273476 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273476&action=review
> 
> > Source/WebCore/platform/spi/mac/DataDetectorsSPI.h:104
> > -SOFT_LINK_CLASS(DataDetectors, DDAction)
> > -SOFT_LINK_CLASS(DataDetectors, DDActionContext)
> > -SOFT_LINK_CLASS(DataDetectors, DDActionsManager)
> > +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDAction)
> > +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDActionContext)
> > +SOFT_LINK_CLASS_OPTIONAL(DataDetectors, DDActionsManager)
> 
> Not something that needs to be done in this patch, but I wonder if we can
> make it a compile-time error to non-optionally soft-link things (classes,
> pointers, functions, etc.) in optionally soft-linked frameworks.

Yes, we could make the make for the optionally soft-linked frameworks different from the non-optionally soft-linked frameworks, thereby causing a compiler error.  :)

> > Source/WebKit2/Shared/mac/WebHitTestResultData.mm:88
> > -    RetainPtr<NSKeyedUnarchiver> unarchiver = adoptNS([[NSKeyedUnarchiver alloc] initForReadingWithData:(NSData *)data.get()]);
> > -    [unarchiver setRequiresSecureCoding:YES];
> > -    @try {
> > -        hitTestResultData.detectedDataActionContext = [unarchiver decodeObjectOfClass:getDDActionContextClass() forKey:@"actionContext"];
> > -    } @catch (NSException *exception) {
> > -        LOG_ERROR("Failed to decode DDActionContext: %@", exception);
> > -        return false;
> > -    }
> > +    if (DataDetectorsLibrary()) {
> > +        RetainPtr<NSKeyedUnarchiver> unarchiver = adoptNS([[NSKeyedUnarchiver alloc] initForReadingWithData:(NSData *)data.get()]);
> > +        [unarchiver setRequiresSecureCoding:YES];
> > +        @try {
> > +            hitTestResultData.detectedDataActionContext = [unarchiver decodeObjectOfClass:getDDActionContextClass() forKey:@"actionContext"];
> > +        } @catch (NSException *exception) {
> > +            LOG_ERROR("Failed to decode DDActionContext: %@", exception);
> > +            return false;
> > +        }
> >      
> > -    [unarchiver finishDecoding];
> > +        [unarchiver finishDecoding];
> > +    }
> 
> I think we could do slightly better here.
> WebHitTestResultData::platformEncode() is going to encode a non-nil value
> for actionContext unconditionally, so I assume that a non-nil actionContext
> implies that DataDetectors.framework is present. Given that, you could just
> ASSERT(DataDetectorsLibrary()) if hasActionContext is true. If my assumption
> is wrong, then this change is also wrong, because the encoding and decoding
> will be out of balance.

Will remove the if block and add a Debug ASSERT().  Thanks!

[David Kilzer (:ddkilzer)]

Committed r197902: <http://trac.webkit.org/changeset/197902>