Bug 208783 – [iOS] Mapping to UTI from tag and tag class should be performed in the UI process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Calling UTTypeCreatePreferredIdentifierForTag to find UTI from tag and tag class should be done in the UI process and not in the WebContent process, since this is using a system service that will be closed.

[Per Arne Vollan]

Created attachment 392954 [details]
Patch

[Per Arne Vollan]

Created attachment 392955 [details]
Patch

[Per Arne Vollan]

Created attachment 392956 [details]
Patch

[Per Arne Vollan]

Created attachment 392962 [details]
Patch

[Per Arne Vollan]

Created attachment 392963 [details]
Patch

[Per Arne Vollan]

Created attachment 392964 [details]
Patch

[Per Arne Vollan]

Created attachment 392966 [details]
Patch

[Per Arne Vollan]

Created attachment 392968 [details]
Patch

[Per Arne Vollan]

Created attachment 392972 [details]
Patch

[Per Arne Vollan]

Created attachment 392978 [details]
Patch

[Per Arne Vollan]

Created attachment 392981 [details]
Patch

[Per Arne Vollan]

Created attachment 392987 [details]
Patch

[Per Arne Vollan]

Created attachment 392989 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/60205026>

[Per Arne Vollan]

Created attachment 392990 [details]
Patch

[Per Arne Vollan]

Created attachment 392992 [details]
Patch

[Brent Fulgham]

Comment on attachment 392992 [details]
Patch

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

Please fix the memory leaks (or explain how I am mistaken!). But r=me with that change.

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:89
> +            WebUTTypeRecord *record = [WebUTTypeRecord alloc];

Should we be using a smart pointer of some kind to avoid leaks/overreleases?

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:107
> +            r.uti = UTTypeCreatePreferredIdentifierForTag(r.tagClass.createCFString().get(), r.tag.createCFString().get(), r.identifier.isNull() ? nullptr : r.identifier.createCFString().get());

I think UTTypeCreatePreferredIdentifierForTag returns a CFStringRef with a +1 count. I think you need something like:

r.uti = adoptCF(UTTypeCreatePreferredIdentifierForTag ....).get();

Or maybe use your new UTIFromTag helper?

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:110
> +            i.uti = UTTypeCreatePreferredIdentifierForTag(i.tagClass.createCFString().get(), i.tag.createCFString().get(), i.identifier.isNull() ? nullptr : i.identifier.createCFString().get());

Ditto.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #17)
> Comment on attachment 392992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392992&action=review
> 
> Please fix the memory leaks (or explain how I am mistaken!). But r=me with
> that change.
> 
> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:89
> > +            WebUTTypeRecord *record = [WebUTTypeRecord alloc];
> 
> Should we be using a smart pointer of some kind to avoid leaks/overreleases?
> 
> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:107
> > +            r.uti = UTTypeCreatePreferredIdentifierForTag(r.tagClass.createCFString().get(), r.tag.createCFString().get(), r.identifier.isNull() ? nullptr : r.identifier.createCFString().get());
> 
> I think UTTypeCreatePreferredIdentifierForTag returns a CFStringRef with a
> +1 count. I think you need something like:
> 
> r.uti = adoptCF(UTTypeCreatePreferredIdentifierForTag ....).get();
> 
> Or maybe use your new UTIFromTag helper?
> 
> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:110
> > +            i.uti = UTTypeCreatePreferredIdentifierForTag(i.tagClass.createCFString().get(), i.tag.createCFString().get(), i.identifier.isNull() ? nullptr : i.identifier.createCFString().get());
> 
> Ditto.

Nice catch! Will fix.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 392997 [details]
Patch

[Per Arne Vollan]

Committed r258120: <https://trac.webkit.org/changeset/258120/webkit>

[Simon Fraser (smfr)]

Comment on attachment 392992 [details]
Patch

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

r- because of leaks

>>> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:89
>>> +            WebUTTypeRecord *record = [WebUTTypeRecord alloc];
>> 
>> Should we be using a smart pointer of some kind to avoid leaks/overreleases?
> 
> Nice catch! Will fix.
> 
> Thanks for reviewing!

Why is this not [[WebUTTypeRecord alloc] init]?

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:94
> +    WTFLogAlways("UTI not found for tag: %s %s %s\n", String(tag).utf8().data(), String(tagClass).utf8().data(), String(identifier).utf8().data());

Does this leak any privacy-sensitive information?

> Source/WebCore/testing/Internals.idl:856
>      DOMString getUTIFromMIMEType(DOMString mimeType);
> +    DOMString getUTIFromTag(DOMString tagClass, DOMString tag, DOMString conformingToUTI);

These seem like a rather odd use of internals. Can these be API tests instead?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:379
> +    if (!WebCore::IOSApplication::isMobileSafari() || _AXSApplicationAccessibilityEnabled()) {

Should accessibility really enable all these services, or should it be more fine-grained?

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #21)
> Comment on attachment 392992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392992&action=review
> 
> r- because of leaks
> 
> >>> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:89
> >>> +            WebUTTypeRecord *record = [WebUTTypeRecord alloc];
> >> 
> >> Should we be using a smart pointer of some kind to avoid leaks/overreleases?
> > 
> > Nice catch! Will fix.
> > 
> > Thanks for reviewing!
> 
> Why is this not [[WebUTTypeRecord alloc] init]?
> 

Good point! I will fix this.

> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:94
> > +    WTFLogAlways("UTI not found for tag: %s %s %s\n", String(tag).utf8().data(), String(tagClass).utf8().data(), String(identifier).utf8().data());
> 
> Does this leak any privacy-sensitive information?
> 

I don't believe so, but we should remove the logging as soon as we have identified the UTI types not covered.

> > Source/WebCore/testing/Internals.idl:856
> >      DOMString getUTIFromMIMEType(DOMString mimeType);
> > +    DOMString getUTIFromTag(DOMString tagClass, DOMString tag, DOMString conformingToUTI);
> 
> These seem like a rather odd use of internals. Can these be API tests
> instead?
> 

They are actually used in API tests to compare the values in the UI process with the values in the WebContent process. But perhaps using internals in API tests is not optimal?

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:379
> > +    if (!WebCore::IOSApplication::isMobileSafari() || _AXSApplicationAccessibilityEnabled()) {
> 
> Should accessibility really enable all these services, or should it be more
> fine-grained?

You are right! This is a speculative fix, and the extensions will be more targeted when we learn who is actually using the services.

Thanks for reviewing, Simon!

[Simon Fraser (smfr)]

Comment on attachment 392997 [details]
Patch

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

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:42
> +@interface WebUTTypeRecord : UTTypeRecord

UTTypeRecord is a tagged pointer type. Override it like this may cause serious problems.

> Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:53
> +    _identifier = i;

How do we know that 'I' stays alive? This should probably own it and -copy.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #23)
> Comment on attachment 392997 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=392997&action=review
> 
> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:42
> > +@interface WebUTTypeRecord : UTTypeRecord
> 
> UTTypeRecord is a tagged pointer type. Override it like this may cause
> serious problems.
> 
> > Source/WebCore/platform/cocoa/UTTypeRecordSwizzler.mm:53
> > +    _identifier = i;
> 
> How do we know that 'I' stays alive? This should probably own it and -copy.

I will work on coming up with a better approach for these issues now.

Thanks for catching this!