Bug 215718 – PlatformPasteboard::getPathnamesForType() assumes the pasteboard type property list is an NSArray of NSStrings

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Kate Cheney]

This is not always the case, so we should probably handle other cases

[Kate Cheney]

<rdar://problem/67505327>

[Kate Cheney]

Created attachment 406979 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/67906582>

[Kate Cheney]

Created attachment 408262 [details]
Patch

[Darin Adler]

Comment on attachment 408262 [details]
Patch

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

> Source/WebCore/platform/mac/PlatformPasteboardMac.mm:113
> +        if ([[paths objectAtIndex:0] isKindOfClass:[NSArray class]]) {
> +            [paths enumerateObjectsUsingBlock:^(NSArray *plistEntry, NSUInteger, BOOL *) {
> +                pathnames.appendVector(makeVector<String>(plistEntry));
> +            }];
> +            return;
> +        }

This is very interesting. What is the semantics of this array of arrays? Why is it correct to flatten out this array of arrays into a single vector of strings? Are all the strings in all these arrays pathnames? Why is this structured that way?

(Separately, sadly appendVector(makeVector) is not particularly efficient, and we might want to improve that later.)

[Kate Cheney]

(In reply to Darin Adler from comment #5)
> Comment on attachment 408262 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408262&action=review
> 
> > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:113
> > +        if ([[paths objectAtIndex:0] isKindOfClass:[NSArray class]]) {
> > +            [paths enumerateObjectsUsingBlock:^(NSArray *plistEntry, NSUInteger, BOOL *) {
> > +                pathnames.appendVector(makeVector<String>(plistEntry));
> > +            }];
> > +            return;
> > +        }
> 
> This is very interesting. What is the semantics of this array of arrays? Why
> is it correct to flatten out this array of arrays into a single vector of
> strings? Are all the strings in all these arrays pathnames? Why is this
> structured that way?

I noticed this while investigating another bug, but haven't actually been able to reproduce any user-facing impact (trying now). I thought it was strange that WebKit itself sets property lists of NSArrays of NSArrays but here assumes the type is an NSArray of NSStrings. Maybe we don't care about that case in PlatformPasteboard::getPathnamesForType().

> 
> (Separately, sadly appendVector(makeVector) is not particularly efficient,
> and we might want to improve that later.)

Noted, I did not know it was inefficient.

[Kate Cheney]

(In reply to katherine_cheney from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 408262 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=408262&action=review
> > 
> > > Source/WebCore/platform/mac/PlatformPasteboardMac.mm:113
> > > +        if ([[paths objectAtIndex:0] isKindOfClass:[NSArray class]]) {
> > > +            [paths enumerateObjectsUsingBlock:^(NSArray *plistEntry, NSUInteger, BOOL *) {
> > > +                pathnames.appendVector(makeVector<String>(plistEntry));
> > > +            }];
> > > +            return;
> > > +        }
> > 
> > This is very interesting. What is the semantics of this array of arrays? Why
> > is it correct to flatten out this array of arrays into a single vector of
> > strings? Are all the strings in all these arrays pathnames? Why is this
> > structured that way?
> 
> I noticed this while investigating another bug, but haven't actually been
> able to reproduce any user-facing impact (trying now). I thought it was
> strange that WebKit itself sets property lists of NSArrays of NSArrays but
> here assumes the type is an NSArray of NSStrings. Maybe we don't care about
> that case in PlatformPasteboard::getPathnamesForType().
> 

Actually, looking at other parts of the code, you're right. PlatformPasteboard::getPathnamesForType() should only need to handle the case of NSArray of NSStrings, because the NSArray of NSArray case is not pathname-related. Closing this bug.