Bug 208783

Summary: [iOS] Mapping to UTI from tag and tag class should be performed in the UI process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, simon.fraser, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review+
Patch none

Description Per Arne Vollan 2020-03-08 08:56:28 PDT
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.
Comment 1 Per Arne Vollan 2020-03-08 09:06:35 PDT
Created attachment 392954 [details]
Patch
Comment 2 Per Arne Vollan 2020-03-08 09:15:12 PDT
Created attachment 392955 [details]
Patch
Comment 3 Per Arne Vollan 2020-03-08 09:25:39 PDT
Created attachment 392956 [details]
Patch
Comment 4 Per Arne Vollan 2020-03-08 11:15:44 PDT
Created attachment 392962 [details]
Patch
Comment 5 Per Arne Vollan 2020-03-08 11:22:16 PDT
Created attachment 392963 [details]
Patch
Comment 6 Per Arne Vollan 2020-03-08 11:49:26 PDT
Created attachment 392964 [details]
Patch
Comment 7 Per Arne Vollan 2020-03-08 12:00:07 PDT
Created attachment 392966 [details]
Patch
Comment 8 Per Arne Vollan 2020-03-08 12:07:32 PDT
Created attachment 392968 [details]
Patch
Comment 9 Per Arne Vollan 2020-03-08 12:29:30 PDT
Created attachment 392972 [details]
Patch
Comment 10 Per Arne Vollan 2020-03-08 13:11:14 PDT
Created attachment 392978 [details]
Patch
Comment 11 Per Arne Vollan 2020-03-08 13:45:12 PDT
Created attachment 392981 [details]
Patch
Comment 12 Per Arne Vollan 2020-03-08 14:25:39 PDT
Created attachment 392987 [details]
Patch
Comment 13 Per Arne Vollan 2020-03-08 15:00:49 PDT
Created attachment 392989 [details]
Patch
Comment 14 Radar WebKit Bug Importer 2020-03-08 15:01:22 PDT
<rdar://problem/60205026>
Comment 15 Per Arne Vollan 2020-03-08 15:04:01 PDT
Created attachment 392990 [details]
Patch
Comment 16 Per Arne Vollan 2020-03-08 15:13:06 PDT
Created attachment 392992 [details]
Patch
Comment 17 Brent Fulgham 2020-03-08 15:54:58 PDT
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.
Comment 18 Per Arne Vollan 2020-03-08 16:03:01 PDT
(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!
Comment 19 Per Arne Vollan 2020-03-08 16:20:38 PDT
Created attachment 392997 [details]
Patch
Comment 20 Per Arne Vollan 2020-03-08 16:53:29 PDT
Committed r258120: <https://trac.webkit.org/changeset/258120/webkit>
Comment 21 Simon Fraser (smfr) 2020-03-08 21:06:03 PDT
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?
Comment 22 Per Arne Vollan 2020-03-08 21:27:20 PDT
(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!
Comment 23 Simon Fraser (smfr) 2020-03-08 21:51:04 PDT
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.
Comment 24 Per Arne Vollan 2020-03-09 09:16:43 PDT
(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!