New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable NSPredicate evaluation with RLMArray as subquery collection #4770

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@0xced

0xced commented Mar 24, 2017

When evaluating an NSPredicate on an RLMObject, an exception is thrown if the subquery's collection is an RLMArray instance although it should work just fine.

-[NSPredicate evaluateWithObject:] eventually calls -[NSSubqueryExpression expressionValueWithObject:context:] when
the predicate is a subquery. The problem is that this method is incorrectly implemented, it goes roughly like this:

id collection = [self.collection expressionValueWithObject:object context:context];
if (![collection isNSArray__] || ![collection isNSSet__] || ![collection isNSOrderedSet__])
{
    NSString *reason = @"Can't perform collection evaluate with non-collection object."
    @throw [NSException exceptionWithName:NSInternalInconsistencyException reason:reason userInfo:nil];
}

for (id element in collection)
{
    // goes on with evaluation
}

So, instead of testing if the collection conforms to the NSFastEnumeration protocol (which RLMArray does) the
collection is tested against three known Foundation collections.

Pretending that RLMArray is an NSArray by implementing the isNSArray__ method enables NSPredicate evaluation with RLMArray as subquery collection.

Since the isNSArray__ method is not public, some care was taken to construct the selector dynamically.

@jpsim jpsim requested review from bdash and jpsim Mar 24, 2017

@jpsim jpsim added the S:Review label Mar 24, 2017

@jpsim

This comment has been minimized.

Contributor

jpsim commented Mar 24, 2017

Thanks for filing this @0xced. Have you filed a radar about this? If not, I'm happy to file one myself.

@bdash can you confirm that if we do this, we also ought to do it with our RLMLinkingObjects too?

@bdash

This comment has been minimized.

Contributor

bdash commented Mar 24, 2017

I think we'd need to do it for all of our root-level collection types (RLMArray, RLMResults). RLMLinkingObjects would inherit the implementation from RLMResults. I suspect we also need to do the same for our Swift collection types (Results, List, LinkingObjects), since they could conceivably be used in the same way.

@0xced

This comment has been minimized.

0xced commented Mar 24, 2017

Have you filed a radar about this?

No, I haven't.

If not, I'm happy to file one myself.

Great, please go ahead.

@bdash

Thanks for catching this issue and submitting a PR, @0xced. Would you mind confirming that you've completed our Contributor License Agreement?

@@ -46,6 +46,14 @@ @implementation RLMArray {
NSMutableArray *_backingArray;
}
+ (void)load

This comment has been minimized.

@bdash

bdash Mar 24, 2017

Contributor

+initialize would be preferable to +load since it's sufficient to run this the first time an instance is constructed, rather than as soon as the image is loaded.

{
SEL isNSArray = NSSelectorFromString([@[@"is", @"NSArray", @"_", @"_"] componentsJoinedByString:@""]);
IMP yes = imp_implementationWithBlock(^(__unused id array) { return YES; });
Method isConcurrent = class_getInstanceMethod(NSOperation.class, @selector(isConcurrent));

This comment has been minimized.

@bdash

bdash Mar 24, 2017

Contributor

It'd be preferable to use one of our own classes / selectors here, since it may cause people to wonder why NSOperation is referenced. -[RLMArray isInvalidated] looks to have the expected type signature.

@jpsim

This comment has been minimized.

Contributor

jpsim commented Mar 24, 2017

Radar filed: rdar://31252694.

@0xced

This comment has been minimized.

0xced commented Mar 24, 2017

Too bad I can’t see why the tests are failing on Jenkins because of course it works on my machine.

@bdash

This comment has been minimized.

Contributor

bdash commented Mar 24, 2017

The failures of Cocoapods tests are because they don't work with PRs originating from forks of the repository, only with branches on the realm copy of the repository.

@0xced

This comment has been minimized.

0xced commented Mar 24, 2017

@bdash I just completed the CLA. I hope you are fine with the changes I made since I opened this pull request.

Method isInvalidated = class_getInstanceMethod(RLMArray.class, @selector(isInvalidated));
const char *typeEncoding = method_getTypeEncoding(isInvalidated);
class_addMethod(RLMArray.class, isNSArray, yes, typeEncoding);
class_addMethod(RLMResults.class, isNSArray, yes, typeEncoding);

This comment has been minimized.

@bdash

bdash Mar 25, 2017

Contributor

To cover the RealmSwift classes I think we'd need to do the same on RLMListBase, RealmSwift.Results and RealmSwift.LinkingObjects. RLMListBase is defined by Realm.farmework and so should always be available, while the other two are only available if RealmSwift is loaded and would need to be looked up via NSClassFromString.

This comment has been minimized.

@0xced

0xced Mar 25, 2017

I can easily add these 3 but I don’t know how to write the corresponding Swift unit tests.

This comment has been minimized.

@bdash

bdash Mar 25, 2017

Contributor

I can take care of adding them, and their tests, if you'd like.

This comment has been minimized.

@0xced

0xced Mar 25, 2017

Yes, please go ahead.

This comment has been minimized.

@bdash

bdash Mar 25, 2017

Contributor

Done!

@bdash

This comment has been minimized.

Contributor

bdash commented Mar 25, 2017

The non-Cocoapods failures appear to just be CI flakiness.

@bdash

This comment has been minimized.

Contributor

bdash commented Mar 25, 2017

My only remaining concern is what other purposes Foundation uses the isNSArray__ method for. I see a few other calls to it within other predicate evaluation code, which will likely result in improved behavior, but there are also calls within user defaults and property list serialization code, amongst other locations. I'll need to look over these call sites to see what impact this change would have on them.

@0xced

This comment has been minimized.

0xced commented Mar 25, 2017

I just addressed your concern in 2ff9c8b.

@0xced 0xced force-pushed the 0xced:Subquery-RLMArray branch 2 times, most recently from c1852cd to 33de074 Mar 27, 2017

@0xced

This comment has been minimized.

0xced commented Mar 28, 2017

Freshly rebased on master.

@jpsim

This comment has been minimized.

Contributor

jpsim commented Mar 29, 2017

The iOS and tvOS device test failures appear legitimate. They're all failing with the "Can't perform collection evaluate" exception.

Failing tests:
	-[AsyncQueryTests testSubqueryPredicateEvaluationWithRLMArray]
	-[AsyncQueryTests testSubqueryPredicateEvaluationWithRLMLinkingObjects]
	-[AsyncQueryTests testSubqueryPredicateEvaluationWithRLMResults]
	-[NullQueryTests testSubqueryPredicateEvaluationWithRLMArray]
	-[NullQueryTests testSubqueryPredicateEvaluationWithRLMLinkingObjects]
	-[NullQueryTests testSubqueryPredicateEvaluationWithRLMResults]
	-[QueryTests testSubqueryPredicateEvaluationWithRLMArray]
	-[QueryTests testSubqueryPredicateEvaluationWithRLMLinkingObjects]
	-[QueryTests testSubqueryPredicateEvaluationWithRLMResults]
	-[QueryWithReversedColumnOrderTests testSubqueryPredicateEvaluationWithRLMArray]
	-[QueryWithReversedColumnOrderTests testSubqueryPredicateEvaluationWithRLMLinkingObjects]
	-[QueryWithReversedColumnOrderTests testSubqueryPredicateEvaluationWithRLMResults]

That's on iOS 8.3 and tvOS 9.1. My guess is that -[NSPredicate evaluateWithObject:] doesn't always go through -[NSSubqueryExpression expressionValueWithObject:context:], which would explain why this passed before 33de074.

@jpsim

This comment has been minimized.

Contributor

jpsim commented Mar 29, 2017

Yeah, 33de074 is definitely the culprit as the device tests passed with 91babf7: https://ci.realm.io/job/objc_pr/4898/

@0xced

This comment has been minimized.

0xced commented Mar 29, 2017

Haha, I did not even notice the failure because all previous tests were also half failing (as previously explained by @bdash). Spotting 5 failing and 35 successful checks vs 3 failing and 37 successful checks is not easy. 😕

Unfortunately, I don’t have a device with iOS 8.2 to see what’s going on.

Also, I can't see the Jenkins builds, I get the following error:

Access Denied
0xced is missing the Overall/Read permission

@jpsim

This comment has been minimized.

Contributor

jpsim commented Mar 29, 2017

Our Jenkins instance is only accessible within the Realm organization, so I apologize for that inconvenience. Have you tested this on a physical device at all, @0xced? Sometimes iOS Simulator SDK implementations differ than the ARM implementations, so you might be able to reproduce this even on devices running more recent OS versions.

We could also take some time to investigate this on our end as well, if you prefer.

@0xced

This comment has been minimized.

0xced commented Mar 29, 2017

I can reproduce the issue on iOS 10.2.1, the call stack symbols are unusable on a real device unfortunately.

(lldb) po callStackSymbols
<_NSCallStackArray 0x170449ff0>(
0   Realm                               0x000000010110387c ___Z26RLMWorkaroundRadar31252694v_block_invoke + 68,
1   Foundation                          0x00000001903d4fcc <redacted> + 140,
2   Foundation                          0x00000001902b8bc0 <redacted> + 176,
3   Foundation                          0x00000001902b8a3c <redacted> + 232,
4   Tests                               0x0000000100895778 -[QueryTests testSubqueryPredicateEvaluationWithRLMArray] + 636,
5   CoreFoundation                      0x000000018f867150 <redacted> + 144,
6   CoreFoundation                      0x000000018f759eac <redacted> + 284,
7   XCTest                              0x000000010025a0a8 __24-[XCTestCase invokeTest]_block_invoke_2 + 388,
8   XCTest                              0x000000010028ec98 -[XCTestContext performInScope:] + 208,
9   XCTest                              0x0000000100259f0c -[XCTestCase invokeTest] + 268,
10  Tests                               0x0000000100a3b19c -[RLMTestCase invokeTest] + 320,
11  XCTest                              0x000000010025a5e0 -[XCTestCase performTest:] + 460,
12  XCTest                              0x0000000100257a5c -[XCTestSuite performTest:] + 428,
13  XCTest                              0x0000000100257a5c -[XCTestSuite performTest:] + 428,
14  XCTest                              0x0000000100257a5c -[XCTestSuite performTest:] + 428,
15  XCTest                              0x0000000100243740 __25-[XCTestDriver _runSuite]_block_invoke + 56,
16  XCTest                              0x0000000100264260 -[XCTestObservationCenter _observeTestExecutionForBlock:] + 528,
17  XCTest                              0x00000001002435d8 -[XCTestDriver _runSuite] + 460,
18  XCTest                              0x00000001002443b4 -[XCTestDriver _checkForTestManager] + 296,
19  XCTest                              0x0000000100290164 _XCTestMain + 628,
20  CoreFoundation                      0x000000018f80ea44 <redacted> + 20,
21  CoreFoundation                      0x000000018f80e240 <redacted> + 288,
22  CoreFoundation                      0x000000018f80c094 <redacted> + 788,
23  CoreFoundation                      0x000000018f73a2b8 CFRunLoopRunSpecific + 444,
24  GraphicsServices                    0x00000001911ee198 GSEventRunModal + 180,
25  UIKit                               0x00000001957817fc <redacted> + 684,
26  UIKit                               0x000000019577c534 UIApplicationMain + 208,
27  TestHost                            0x0000000100100ae0 main + 124,
28  libdyld.dylib                       0x000000018e71d5b8 <redacted> + 4
)

But I have a solution, stay tuned!

@0xced 0xced force-pushed the 0xced:Subquery-RLMArray branch from 33de074 to 0d2e7f9 Mar 29, 2017

0xced added some commits Mar 24, 2017

Enable NSPredicate evaluation with RLMArray as subquery collection
When evaluating an NSPredicate on an RLMObject, an exception is thrown if the subquery's collection is an RLMArray instance although it should work just fine.

`-[NSPredicate evaluateWithObject:]` eventually calls `-[NSSubqueryExpression expressionValueWithObject:context:]` when
the predicate is a subquery. The problem is that this method is incorrectly implemented, it goes roughly like this:

```objc
id collection = [self.collection expressionValueWithObject:object context:context];
if (![collection isNSArray__] || ![collection isNSSet__] || ![collection isNSOrderedSet__])
{
    NSString *reason = @"Can't perform collection evaluate with non-collection object."
    @throw [NSException exceptionWithName:NSInternalInconsistencyException reason:reason userInfo:nil];
}

for (id element in collection)
{
    // goes on with evaluation
}
```

So, instead of testing if the collection conforms to the `NSFastEnumeration` protocol (which RLMArray does) the
collection is tested against three known Foundation collections.

Pretending that RLMArray is an NSArray by implementing the `isNSArray__` method enables NSPredicate evaluation with RLMArray as subquery collection.

Since the `isNSArray__` method is not public, some care was taken to construct the selector dynamically.

0xced and others added some commits Mar 24, 2017

Apply the same isNSArray__ workaround to our Swift collection types.
This required introducing a non-generic base class for `Results` to allow
it to be looked up using `NSClassFromString`.
Safer way to check who is the caller
+[NSThread callStackSymbols] is not usable on real devices because names are redacted. But we know we want the caller to be -[NSSubqueryExpression expressionValueWithObject:context:] and we can get this information with `dladdr`.

@0xced 0xced force-pushed the 0xced:Subquery-RLMArray branch from 0d2e7f9 to 2656fb9 Apr 7, 2017

@0xced

This comment has been minimized.

0xced commented Apr 7, 2017

Any chance to have this merged for the 2.5.1 release?

@jpsim

This comment has been minimized.

Contributor

jpsim commented Apr 7, 2017

Hi @0xced, thanks for following up. I apologize for the delay in getting back to you as we were weighing what we should do.

I discussed this with the rest of the team, and despite this being the best solution to this problem we can think of, it's unfortunately still too risky to include in our official releases.

We can't risk deliberately invoking private APIs (despite being obfuscated) because the risk isn't only on us, but also on our user's who might have their App Store submissions rejected.

We'll be adding an entry to the "Current Limitations" section of our documentation describing this issue and pointing users to your solution if they need to work around it in their apps and they're comfortable with the risks involved.

That being said, we were really impressed with the quality of your contribution and sincerely hope you'll contribute again in the future.

@0xced

This comment has been minimized.

0xced commented Apr 7, 2017

I totally understand your position.

Would you still include it if was opt-in? Say, by exposing RLMWorkaroundRadar31252694 in a public header or by protecting the call with some environment variable? For example:

if (NSProcessInfo.processInfo.environment[@"REALM_WORKAROUND_RADAR_31252694"].boolValue) {
    RLMWorkaroundRadar31252694();
}

Or are you just worried to include the obfuscated code?

@jpsim

This comment has been minimized.

Contributor

jpsim commented Apr 14, 2017

Yes, I'm not quite comfortable even opt-in, just shipping this in the binary. I hope you understand. We updated the docs last week though to refer to this PR as a workaround for rdar://31252694 however 😄

Thanks again, we'll be closing this now.

@jpsim jpsim closed this Apr 14, 2017

@jpsim jpsim removed the S:Review label Apr 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment