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.
Created attachment 392954 [details] Patch
Created attachment 392955 [details] Patch
Created attachment 392956 [details] Patch
Created attachment 392962 [details] Patch
Created attachment 392963 [details] Patch
Created attachment 392964 [details] Patch
Created attachment 392966 [details] Patch
Created attachment 392968 [details] Patch
Created attachment 392972 [details] Patch
Created attachment 392978 [details] Patch
Created attachment 392981 [details] Patch
Created attachment 392987 [details] Patch
Created attachment 392989 [details] Patch
<rdar://problem/60205026>
Created attachment 392990 [details] Patch
Created attachment 392992 [details] Patch
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.
(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!
Created attachment 392997 [details] Patch
Committed r258120: <https://trac.webkit.org/changeset/258120/webkit>
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?
(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 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.
(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!