Bug 145890 – [OS X] Decoding fonts may return nil when using force click

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

[OS X] Decoding fonts may return nil when using force click

[Myles C. Maxfield]

Created attachment 254739 [details]
Speculative fix

[Myles C. Maxfield]

<rdar://problem/21170487>

[Myles C. Maxfield]

Committed r185475: <http://trac.webkit.org/changeset/185475>

[Myles C. Maxfield]

This is not the best solution to this problem.

[Myles C. Maxfield]

Created attachment 254773 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 254773 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:345
> +            [dictionary setObject:value.get() forKey:key.get()];

Simon suggested that we may not even want to be in the business of claiming to transmit fonts in the first place (in favor of font attributes dictionaries). This would push this recreation failure logic into the user of IPC.

[Myles C. Maxfield]

Comment on attachment 254773 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:431
> +        if (value)

Transmitting an array with smaller size than the one sent seems incorrect.

[Myles C. Maxfield]

Comment on attachment 254773 [details]
Patch

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

>> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:345
>> +            [dictionary setObject:value.get() forKey:key.get()];
> 
> Simon suggested that we may not even want to be in the business of claiming to transmit fonts in the first place (in favor of font attributes dictionaries). This would push this recreation failure logic into the user of IPC.

This approach doesn't really work for attributed strings - forcing clients to recreate an attributed string from components seems needlessly complex. Perhaps instead I could have the attributed string encoding function inspect its argument for NSFonts upon ingestion.

Moving this to an encoding function rather than a decoding function seems cleaner. There may be a way to recognize a web font without trying a full encode / decode cycle.

[Myles C. Maxfield]

Comment on attachment 254773 [details]
Patch

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

>>> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:345
>>> +            [dictionary setObject:value.get() forKey:key.get()];
>> 
>> Simon suggested that we may not even want to be in the business of claiming to transmit fonts in the first place (in favor of font attributes dictionaries). This would push this recreation failure logic into the user of IPC.
> 
> This approach doesn't really work for attributed strings - forcing clients to recreate an attributed string from components seems needlessly complex. Perhaps instead I could have the attributed string encoding function inspect its argument for NSFonts upon ingestion.
> 
> Moving this to an encoding function rather than a decoding function seems cleaner. There may be a way to recognize a web font without trying a full encode / decode cycle.

Another alternative is to make the client of the IPC subsystem check for these non-serializable fonts, and then maintain an invariant that any font inside the IPC subsystem is serializable.

[Darin Adler]

Comment on attachment 254773 [details]
Patch

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

>>>> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:345
>>>> +            [dictionary setObject:value.get() forKey:key.get()];
>>> 
>>> Simon suggested that we may not even want to be in the business of claiming to transmit fonts in the first place (in favor of font attributes dictionaries). This would push this recreation failure logic into the user of IPC.
>> 
>> This approach doesn't really work for attributed strings - forcing clients to recreate an attributed string from components seems needlessly complex. Perhaps instead I could have the attributed string encoding function inspect its argument for NSFonts upon ingestion.
>> 
>> Moving this to an encoding function rather than a decoding function seems cleaner. There may be a way to recognize a web font without trying a full encode / decode cycle.
> 
> Another alternative is to make the client of the IPC subsystem check for these non-serializable fonts, and then maintain an invariant that any font inside the IPC subsystem is serializable.

Dictionary decoding seems too low a level to make a decision like this one, and changing the shape of the dictionary seems wrong to me. At this level the only behaviors that make sense to me are:

1) Failing entirely.
2) Turning an unsuccessfully transmitted value into some kind of object like an NSNull.

And I strongly prefer (1).

Generally speaking the encoders and decoders are too low a level to be dealing with this. Sure, the encoder could detect that this is a font that we can’t recreate by changing it into a font descriptor, and that seems better than having the decoder detect it, but there is no reasonable way for the encoder and decoder to handle that. The idea that we can send an attributed string across the process boundary with a web font as the font seems fundamentally flawed.

*** I think the primary problem here is that we are trying to change the code before we have decided what behavior we are trying to implement. What do we want the UI process to receive in the case of a string that is supposed to be using a font that exists only in the web process? ***

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:368
> -    return result;
> +    return true;

I’m not sure that you will want to roll this out once we make our final decision about behavior. I don’t know that “successfully” decoding a null NSFont is going to end up being a valuable part of the solution.

I don’t think that nil is a good way to represent “failed to transmit the NSFont successfully”. It seems more appropriate to use nil to encode something that was nil on the sending side, and some other way to indicate that we couldn’t transmit an NSFont across the process boundary.

>> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:431
>> +        if (value)
> 
> Transmitting an array with smaller size than the one sent seems incorrect.

I agree; array decoding is too low a level to make a decision like this one and changing the size of the array seems wrong to me. At this level the only behaviors that make sense to me are:

1) Failing entirely.
2) Turning an unsuccessfully transmitted value into some kind of object like an NSNull.

And I strongly prefer (1).

[Darin Adler]

For example, do we want to substitute a different font? If so, which font?

Do we want to transmit the entire font to the UI process (seems bad for sandboxing)? Generally speaking I don’t think it’s safe for the UI process to try to render a web font.

I am pretty sure that this work should be done on the web process side in a separate step *before* trying to transmit the attributed string rather than as part of the encoding/decoding process.

[Darin Adler]

I think while we try to figure that out we should make this fail cleanly and get that into the build, if the previous patch was insufficient for that.

[Myles C. Maxfield]

<rdar://problem/21390877>

[Myles C. Maxfield]

Created attachment 254988 [details]
Patch

[Myles C. Maxfield]

Created attachment 254989 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 254989 [details]
Patch

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

> Source/WebKit2/ChangeLog:5
> +        <rdar://problem/21170487>

Wrong radar number

[Darin Adler]

Comment on attachment 254989 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm:579
> +            if (fontCanSurviveSerialization(font))
> +                [scaledAttributes setObject:font forKey:NSFontAttributeName];

This doesn’t remove the font from scaledAttributes if it can’t survive serialization. Need an else that calls removeObjectForKey: and you also need to do some testing to verify this does what you want it to do.

[Myles C. Maxfield]

Created attachment 255160 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 255160 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:215
> +    return [NSFontDescriptor fontDescriptorWithFontAttributes:[[font fontDescriptor] fontAttributes]];

Turns out this actually returns success if its run in the same process as the web font :(

[Myles C. Maxfield]

Created attachment 255165 [details]
Patch

[Myles C. Maxfield]

It looks like our lookup tests don't actually perform any IPC serialization

[Darin Adler]

Comment on attachment 255165 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:218
> +static inline bool fontIsSerializable(NSFont *font)
> +{
> +    return adoptCF(CTFontCopyAttribute((CTFontRef)font, kCTFontURLAttribute));
> +}

All three call sites that use this are using it the same way. They want a function that distinguishes non-serializable fonts from all other objects. Given the function name, filterUnserializableValues, below, I suggest adding this function:

    static inline bool isSerializableValue(id value)
    {
        return ![value isKindOfClass:[NSFont class]] || fontIsSerializable(value);
    }

You could use that in all three call sites.

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:222
> +    auto result = adoptNS([NSMutableDictionary new]);

We almost always use alloc/init instead of new; new is smaller code and slightly slower. I don’t think this function is a good one to be the first that does this!

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:223
> +    for (NSString *key in dictionary) {

I think it would be more efficient to do this with enumerateKeysAndObjectsUsingBlock: rather than a for loop.

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:224
> +        ASSERT([key isKindOfClass:[NSString class]]);

Doesn’t seem important to assert this.

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:225
> +        id object = dictionary[key];

This won’t compile on older platforms we still support. Needs to be:

    id object = [dictionary objectForKey:key];

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:226
> +        if (object && (![object isKindOfClass:[NSFont class]] || fontIsSerializable(object)))

Looks like this is compiling for iOS. To state the obvious, need to do an #if, since NSFont is not present on iOS. Or possibly use typedef to make the equivalent code work with UIFont?

No need to do if (object) here. That can be an assertion. NSDictionary doesn’t support having a key with an object of nil.

[Darin Adler]

Comment on attachment 255165 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:220
> +static inline RetainPtr<NSDictionary> filterUnserializableValues(NSDictionary *dictionary)

Kind of annoying that this always copies the entire dictionary even though the common case is that we don’t have to filter anything. Consider optimizing the case where we didn’t have to filter anything to discard the mutable dictionary and just return the passed-in dictionary? Or even use a two pass algorithm. The first pass checks for the normal case to make it faster. Or keep it simple since copying the dictionary is no big deal, I guess.

[Myles C. Maxfield]

Comment on attachment 255165 [details]
Patch

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

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:217
> +    return adoptCF(CTFontCopyAttribute((CTFontRef)font, kCTFontURLAttribute));

This is the wrong kind of cast

[Myles C. Maxfield]

Created attachment 255243 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:246:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


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

[Tim Horton]

Comment on attachment 255243 [details]
Patch

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

> Source/WebKit2/ChangeLog:9
> +        Upon consulting with Anders, Thorton, and Beth, we realized that the best place to stop the serialization

no need for this naming of names

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:30
> +#if !USE(APPKIT)

PLATFORM(IOS)?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:246
> +    [dictionary enumerateKeysAndObjectsUsingBlock:^(id __nonnull key, id __nonnull object, BOOL * __nonnull stop) {

Do you really need all these __nonnulls here?

> Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:248
> +            [result.get() setObject:object forKey:key];

No need for the .get()

[Darin Adler]

Comment on attachment 255243 [details]
Patch

All of Tim’s comments make sense to me. Not sure why he didn’t say review+, though.

[Myles C. Maxfield]

Created attachment 255245 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:236:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/mac/ArgumentCodersMac.mm:246:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 2 files


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

[Myles C. Maxfield]

Committed r185775: <http://trac.webkit.org/changeset/185775>