431475 - KeyEquivDBItem shouldn't use NSMutableIndexSet

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Stack trace

It appears that the value of an index in NSMutableIndexSet is limited to NSNotFound - 1 (where NSNotFound is 0x7fffffff according to http://developer.apple.com/documentation/Cocoa/Reference/Foundation/Miscellaneous/Foundation_Constants/Reference/reference.html).

This means that pointers like mine (0xf92e0fd0) will throw an exception and kill the app. See the attached stack trace.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Obj-C really isn't my strongest language, but I think this might work.

Comment 2

[Josh Aas]

Comment on attachment 318549 [details] [diff] [review]
Patch, v1

+  return [mTables containsObject:[NSValue valueWithPointer:aTable]];

Are you sure this operation uses an equality test instead of the specific pointer you pass in? If not, you're always going to get NO here. You might want to be using the "member:" method of NSSet, which explicitly uses an equality test.

+    [mTables removeObject:[NSValue valueWithPointer:aTable]];

Are you sure this operation uses an equality test instead of the specific pointer you pass in? If not, you're never going to remove objects.

+    ItemValue *itemValue = (ItemValue *)[NSValue valueWithPointer:aItem];

Will this cast work correctly when the object is not actually of type ItemValue? If you called "isKindOfClass:[ItemValue class]" on the object you got back it would be false.

Since you're really using the isEqual method of ItemValue like a utility function, maybe you should just make it a utility function and use that when you need that type of comparison.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> It appears that the value of an index in NSMutableIndexSet is
> limited to NSNotFound - 1

This is appalling!  And Apple doesn't own up to it anywhere (as far as
I can tell).

But you can argue that Apple's docs imply it, and it does appear to be
true (from the evidence of your crash log).

Good catch!

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> It appears that the value of an index in NSMutableIndexSet is
> limited to NSNotFound - 1

On Leopard NSNotFound is defined as NSIntegerMax, which in turn is defined as LONG_MAX.  I did all my compiling and tests on Leopard, which is probably why I didn't see this problem myself.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> On Leopard NSNotFound is defined as NSIntegerMax, which in turn is
> defined as LONG_MAX.  I did all my compiling and tests on Leopard,
> which is probably why I didn't see this problem myself.

And I was compiling on a 64-bit system ... though I wasn't compiling
64-bit binaries.  (On 32-bit systems LONG_MAX _is_ 0x7fffffff.)

Comment 6

[Steven Michaud [:smichaud] (Retired)]

>> It appears that the value of an index in NSMutableIndexSet is
>> limited to NSNotFound - 1
>
> This is appalling!  And Apple doesn't own up to it anywhere (as far
> as I can tell).

What's really appalling is that everywhere in Apple's docs on
NSIndexSet and NSMutableIndexSet, the indexes are claimed to be
unsigned integers (NSUInteger).  But this (apparently) isn't true --
they're really signed integers (NSInteger).

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

The docs don't say anything about how it determines equality so I assume that it just uses pointer comparisons :(

This patch does everything manually.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v3

Tweaked the way KeyEquivDBItem::removeTable method worked and fixed a style nit I knew you were going to comment on :)

Comment 9

[Josh Aas]

Comment on attachment 318661 [details] [diff] [review]
Patch, v3

We don't want the exception handling macros there because that code runs inserted into Apple's framework code and we don't want to interfere with their own exceptions.

Maybe we could leave the exception handling there if it didn't wrap the swizzled methods, just what we're doing ourselves.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

By the way, I'm still not confident the membership comparisons will be done correctly.  As far as I know, the only way to be sure is to override the isEqual and (probably also) the hash methods of the object to be stored.  (I forgot about overriding the hash method in my original code.)

Comment 11

[Josh Aas]

"member:" uses "isEqual:", which is not the same as "isEqualToValue:". We might not be able to do that unless we override "isEqual:" to be the same as "isEqualToValue:".

Comment 12

[Steven Michaud [:smichaud] (Retired)]

I think we should do something like the following:

@interface ItemValue : NSValue
- (NSUInteger)hash;
- (BOOL)isEqual:(id)anObject;
@end
 
@implementation ItemValue

- (NSUInteger)hash
{
  return (NSUInteger) [self pointerValue];
}
 
// Ensure that hastable comparisons use an NSValue object's value -- not its
// address in memory.
- (BOOL)isEqual:(id)anObject
{
  if ([anObject isKindOfClass:[NSValue class]])
    return ([self pointerValue] == [(NSValue*)anObject pointerValue]);
  return [super isEqual:anObject];
}

@end

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Or actually this'd be better:

// Ensure that hastable comparisons use an NSValue object's value -- not its
// address in memory.
- (BOOL)isEqual:(id)anObject
{
  if ([anObject isKindOfClass:[ItemValue class]])
    return ([self pointerValue] == [(ItemValue*)anObject pointerValue]);
  return [super isEqual:anObject];
}

Comment 14

[Stuart Morgan]

Summarizing from irc: NSValue, like most Foundation objects, has isEqual: behavior that does what you would hope rather than straight address comparison, so there's no need for the override.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v4

Ok, I verified that smorgan is correct, NSValue's isEqual does the right thing. This fixes the exception guards to ignore the swizzled methods.

Comment 16

[Josh Aas]

Comment on attachment 318746 [details] [diff] [review]
Patch, v4

+
+  NS_OBJC_END_TRY_ABORT_BLOCK;
+

Extra newline here at the end of the method, plz get rid of it on checkin.

Comment 17

[Steven Michaud [:smichaud] (Retired)]

More quibbles:

If smorgan is right about NSValue's isEqual and hash methods always
"doing the right thing" (and he seems to be), you don't really need
the GetShadowDBKeyForPointer() method.  Instead you can just call

  NSValue *key = [NSValue valueWithPointer:aItem];

This should save a few CPU cycles.

I notice that the Objective-C exception macros are still there, even
though this code is quite isolated from "our" code (it never calls
"our" code, and is never called by it (at least directly)).  With them
present you risk getting crashes on non-fatal errors.

Also, I've had bad luck putting these macros anywhere but at the very
beginning and the very end of a method.  In fact I once resolved a
crash by moving one of these macros to the top of a method (from a
position one line below the top).  Yes, this is pretty vague ... but I
thought I should mention it.

Comment 18

[Josh Aas]

I don't want to be doing anything unexpected in methods we swizzle from the OS, including throwing exceptions. There shouldn't be anything wrong with using those macros in places other than the top and bottom of methods.

Comment 19

[Steven Michaud [:smichaud] (Retired)]

So ... are you going to get rid of GetShadowDBKeyForPointer()?

I don't think that's essential, but it could be a factor in performance.

I'm fine with the Objective-C exception macros, by the way.

Comment 20

[Josh Aas]

Attachment: Patch, v5

Comment 21

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 318902 [details] [diff] [review]
Patch, v5

Looks fine to me.

Comment 22

[Mike Connor [:mconnor]]

Comment on attachment 318902 [details] [diff] [review]
Patch, v5

a=mconnor on behalf of 1.9 drivers

Comment 23

[Reed Loden [:reed]]

I'll let josh or bent land this since it's their patch.

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, I'll get this in as soon as the tree is green. Josh, thanks for grabbing this yesterday, I was swamped.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Just to be safe, I've tested the current patch (attachment 318902 [details] [diff] [review])
against the two STRs from bug 422827 (in bug 422827 comment #8 and bug
422827 comment #29).  Neither caused a crash.

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed!