Bug 213347 – [macOS]: A HEIF image, selected from the OpenPanel, should be converted to an accepted MIME type

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

If a page has an <input> file element which accepts an image like one of these:

<input type="file" accept="image/png, image/jpeg">

And the user selects an image of unsupported format, HEIF for example, in the file picker the image should be converted to one of the formats in the "accept" attribute if possible.

[Radar WebKit Bug Importer]

<rdar://problem/64500775>

[Said Abou-Hallawa]

<rdar://problem/57258464>

[Said Abou-Hallawa]

Created attachment 402433 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402437 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402442 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402452 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402453 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402456 [details]
Patch for review

[Said Abou-Hallawa]

Created attachment 402510 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402514 [details]
Patch

[Said Abou-Hallawa]

Created attachment 402537 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403030 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403031 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403036 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403092 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403099 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403135 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403161 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 403161 [details]
Patch

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

> Source/WebKit/Platform/cg/CGUtilities.cpp:78
> +    auto components = FileSystem::pathGetFileName(path).split('.');
> +    auto extension = WebCore::MIMETypeRegistry::getPreferredExtensionForMIMEType(mimeType);
> +    auto prefix = makeString(components.first(), "_"_s);
> +    auto suffix = makeString("."_s, extension);

A directory name can have a period in it, and a file can have multiple extensions, so this doesn't seem safe.

[Darin Adler]

Comment on attachment 403161 [details]
Patch

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

> Source/WebKit/Platform/cg/CGUtilities.cpp:74
> +    // Compute the temporary file name prefix and file extension.

Since there can be file name length limits, in the past I have found that temporary file names needed to *not* be constructed from user-provided or website-provided file names, of arbitrary length. Not sure what the practical considerations here, but I’m skeptical that this is the best strategy. Typically the extension has to be correct, but the file name can be an arbitrary random string.

> Source/WebKit/Platform/cg/CGUtilities.cpp:77
> +    auto prefix = makeString(components.first(), "_"_s);

If we do end up with code like this, note that makeString accepts characters as well as strings, so you can use '_' and '.' rather than "_"_s and "."_s.

[Simon Fraser (smfr)]

(In reply to Darin Adler from comment #20)
> Comment on attachment 403161 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403161&action=review
> 
> > Source/WebKit/Platform/cg/CGUtilities.cpp:74
> > +    // Compute the temporary file name prefix and file extension.
> 
> Since there can be file name length limits, in the past I have found that
> temporary file names needed to *not* be constructed from user-provided or
> website-provided file names, of arbitrary length. Not sure what the
> practical considerations here, but I’m skeptical that this is the best
> strategy. Typically the extension has to be correct, but the file name can
> be an arbitrary random string.

Said, does the name of the coverted file show up in the <input type="file"> post-conversion?

[Said Abou-Hallawa]

Comment on attachment 403161 [details]
Patch

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

>>> Source/WebKit/Platform/cg/CGUtilities.cpp:74
>>> +    // Compute the temporary file name prefix and file extension.
>> 
>> Since there can be file name length limits, in the past I have found that temporary file names needed to *not* be constructed from user-provided or website-provided file names, of arbitrary length. Not sure what the practical considerations here, but I’m skeptical that this is the best strategy. Typically the extension has to be correct, but the file name can be an arbitrary random string.
> 
> Said, does the name of the coverted file show up in the <input type="file"> post-conversion?

Yes, the converted file name will be shown in the <input type="file">. For example if the page has <input type="file" accept=".jpg,.png,.jpeg">, and the user selects the HEIF image IMG_1311.HEIC, the name IMG_1311_ljd4i1.jpg will be shown in the <input> element.

The length of the converted file name = the length of the original file name + 6. And the <input> element truncates the file name of the file if it exceed a certain threshold. For example, if I insert many '0' letters at the end of the HEIF file name before the period, the name IMG_13110000...0_f798EG.jpg for this long name in the <input> element.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:77
>> +    auto prefix = makeString(components.first(), "_"_s);
> 
> If we do end up with code like this, note that makeString accepts characters as well as strings, so you can use '_' and '.' rather than "_"_s and "."_s.

Fixed.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:78
>> +    auto suffix = makeString("."_s, extension);
> 
> A directory name can have a period in it, and a file can have multiple extensions, so this doesn't seem safe.

This code constructs the temporary file prefix and suffix from the original file and the destination extension. For example   

path ="/Users/local/Downloads/IMG_1311.HEIC"
FileSystem::pathGetFileName(path) = "IMG_1311.HEIC"
components = FileSystem::pathGetFileName(path).split('.') = { "IMG_1311", "HEIC" }
prefix = IMG_1311_
suffix = ".jpg"

So having a period in the directory name should be handled by FileSystem::pathGetFileName(). And I do not think there a problem if the file name has multiple extensions because FileSystem::openTemporaryFil() will add six unique letters to any file name we pass.

But actually I found a real problem which is not handled by this code. If the input has the following markup: <input type="file" accept=".jpg,.png,.jpeg" webkitdirectory>. This does not work correctly especially if the file name has an image extension like "images.gif" for example. If I chose a directory with this name, the assertion ASSERT_NOT_REACHED_WITH_MESSAGE("Replacement image can't be created.") below will fire.

[Darin Adler]

Comment on attachment 403161 [details]
Patch

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

>>>> Source/WebKit/Platform/cg/CGUtilities.cpp:74
>>>> +    // Compute the temporary file name prefix and file extension.
>>> 
>>> Since there can be file name length limits, in the past I have found that temporary file names needed to *not* be constructed from user-provided or website-provided file names, of arbitrary length. Not sure what the practical considerations here, but I’m skeptical that this is the best strategy. Typically the extension has to be correct, but the file name can be an arbitrary random string.
>> 
>> Said, does the name of the coverted file show up in the <input type="file"> post-conversion?
> 
> Yes, the converted file name will be shown in the <input type="file">. For example if the page has <input type="file" accept=".jpg,.png,.jpeg">, and the user selects the HEIF image IMG_1311.HEIC, the name IMG_1311_ljd4i1.jpg will be shown in the <input> element.
> 
> The length of the converted file name = the length of the original file name + 6. And the <input> element truncates the file name of the file if it exceed a certain threshold. For example, if I insert many '0' letters at the end of the HEIF file name before the period, the name IMG_13110000...0_f798EG.jpg for this long name in the <input> element.

So if the original file name has the maximum length supported by the file system, the converted file name will be too big. That’s my point, not how it looks on the webpage.

[Said Abou-Hallawa]

Created attachment 403584 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403585 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403586 [details]
Patch

[Darin Adler]

Comment on attachment 403586 [details]
Patch

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

> Source/WebKit/Platform/cg/CGUtilities.cpp:75
> +    // Compute the extension and the suffix of the temporary file.

This comment in particular says the same thing the code does. We want our comments to say something different from what the code below does, not just say the same thing in English. I suggest removing some of these comments or rewriting them to answer questions not answered by the code, like "why".

> Source/WebKit/Platform/cg/CGUtilities.cpp:76
> +    auto extension = WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType);

Not sure we need this local variable; could combine with the next line.

> Source/WebKit/Platform/cg/CGUtilities.cpp:77
> +    auto suffix = makeString("."_s, extension);

'.' works here and is more efficient than "."_s

> Source/WebKit/Platform/cg/CGUtilities.cpp:79
> +    // Create the temporary file.

Same thing about this comment.

> Source/WebKit/Platform/cg/CGUtilities.cpp:83
> +        ASSERT_NOT_REACHED_WITH_MESSAGE("Replacement image could not be created.");

ASSERT is not appropriate for a failed file system call. It’s OK to log, but not assert, in a case like this.

> Source/WebKit/Platform/cg/CGUtilities.cpp:87
> +    // Create the data consumer.

Same thing about this comment.

> Source/WebKit/Platform/cg/CGUtilities.cpp:88
> +    CGDataConsumerCallbacks callbacks = { [](void *info, const void* buffer, size_t count) -> size_t {

"void* info", not "void *info"

> Source/WebKit/Platform/cg/CGUtilities.cpp:89
> +        auto platformFileHandle = *static_cast<FileSystem::PlatformFileHandle*>(info);

Could name this local just "handle".

> Source/WebKit/Platform/cg/CGUtilities.cpp:93
> +    // Create the image replacement.

Same thing about this comment.

> Source/WebKit/Platform/cg/CGUtilities.cpp:97
> +    // Write the source image to the replacement image.

And this comment.

> Source/WebKit/Platform/cg/CGUtilities.cpp:113
> +    if (mimeTypeForEncoding.isNull())
> +        return { };

Duplicate code, please deelete it.

> Source/WebKit/Platform/cg/CGUtilities.cpp:119
> +        auto mimeType = WebCore::MIMETypeRegistry::mimeTypeForPath(path);

Don’t need this local variable. Could just put this into the expression in the line below.

> Source/WebKit/Platform/cg/CGUtilities.cpp:120
> +        if (allowedMIMETypes.contains(mimeType))

This is a case-sensitive check. This means tat allowedMIMETypes have to be all in lowercase. That’s tricky. Since MIME types aren’t case sensitive, often we try to avoid requiring that they be in the correct case in APIs. I am worried that this is not the case with what we have added here.

> Source/WebKit/Platform/cg/CGUtilities.h:31
> +Vector<String> transcodeImages(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);

The implementation here is specific to CoreGraphics, but the concept isn’t; I could see us implementing this cross-platform later. I’m not sure this should be in a Core-Graphics-specific header.

[Darin Adler]

Would have set review+ but that patch didn’t have review? on it.

[Said Abou-Hallawa]

Created attachment 403613 [details]
Patch

[Said Abou-Hallawa]

*** Bug 213828 has been marked as a duplicate of this bug. ***

[Said Abou-Hallawa]

Comment on attachment 403586 [details]
Patch

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

>> Source/WebKit/Platform/cg/CGUtilities.cpp:75
>> +    // Compute the extension and the suffix of the temporary file.
> 
> This comment in particular says the same thing the code does. We want our comments to say something different from what the code below does, not just say the same thing in English. I suggest removing some of these comments or rewriting them to answer questions not answered by the code, like "why".

Fixed.  I was adding these comments to state the steps of the conversion in plain English.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:76
>> +    auto extension = WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType);
> 
> Not sure we need this local variable; could combine with the next line.

Local variable was removed.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:77
>> +    auto suffix = makeString("."_s, extension);
> 
> '.' works here and is more efficient than "."_s

The character literal was used instead of the string literal.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:83
>> +        ASSERT_NOT_REACHED_WITH_MESSAGE("Replacement image could not be created.");
> 
> ASSERT is not appropriate for a failed file system call. It’s OK to log, but not assert, in a case like this.

ASSERT was replaced with LOG(Images, ...).

>> Source/WebKit/Platform/cg/CGUtilities.cpp:88
>> +    CGDataConsumerCallbacks callbacks = { [](void *info, const void* buffer, size_t count) -> size_t {
> 
> "void* info", not "void *info"

Fixed.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:89
>> +        auto platformFileHandle = *static_cast<FileSystem::PlatformFileHandle*>(info);
> 
> Could name this local just "handle".

Name was changed.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:113
>> +        return { };
> 
> Duplicate code, please deelete it.

Duplicate code was removed.

>> Source/WebKit/Platform/cg/CGUtilities.cpp:119
>> +        auto mimeType = WebCore::MIMETypeRegistry::mimeTypeForPath(path);
> 
> Don’t need this local variable. Could just put this into the expression in the line below.

Local variable was removed.

>> Source/WebKit/Platform/cg/CGUtilities.h:31
>> +Vector<String> transcodeImages(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);
> 
> The implementation here is specific to CoreGraphics, but the concept isn’t; I could see us implementing this cross-platform later. I’m not sure this should be in a Core-Graphics-specific header.

1. paintImage() was moved to NativeImage.cpp in WebCore and was renamed to drawNativeImage().
2. This file was moved to Source/WebKit/Platform/ and was renamed ImageUtilities.h.
3. All the references to paintImage() was repacked with drawNativeImage().

[Said Abou-Hallawa]

Created attachment 403629 [details]
Patch

[Said Abou-Hallawa]

Created attachment 403709 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 403586 [details]
Patch

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

>> Source/WebKit/Platform/cg/CGUtilities.cpp:120
>> +        if (allowedMIMETypes.contains(mimeType))
> 
> This is a case-sensitive check. This means tat allowedMIMETypes have to be all in lowercase. That’s tricky. Since MIME types aren’t case sensitive, often we try to avoid requiring that they be in the correct case in APIs. I am worried that this is not the case with what we have added here.

I made sure MIMETypeRegistry::allowedMIMETypes() returns lowercase MIME types. I also changed one of the layout tests in this patch to include the following attribute: accept="IMAGE/HEIC"

[Simon Fraser (smfr)]

Comment on attachment 403709 [details]
Patch

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

> Source/WebKit/Platform/ImageUtilities.h:30
> +Vector<String> transcodeImages(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);

It's not really clear from the signature that allowedMIMETypes is the set of possible destination MIME types, and that the return value is a set of new paths. If one of the input paths is not transcodable, how does the return value relate to the input paths?

I feel like this needs to be a more expressive API, maybe even a simple class, that makes it easier to use without error.

Also are these absolute paths or URLs? The call site makes them look like URLs.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:59
> +    auto suffix = makeString('.', WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType));

What if preferredExtensionForMIMEType() is empty?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:65
> +        return nullString();

This error case returns the same result as the "no translation needed" case, so the caller can't tell them apart.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:91
> +    String mimeTypeForEncoding = WebCore::MIMETypeRegistry::preferredImageMIMETypeForEncoding(allowedMIMETypes, { });
> +    if (mimeTypeForEncoding.isNull())
> +        return { };

Oh, all the images get the same destination MIME type? That's not what I expected from the signature. I thought it would be one entry in the vector per image.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:98
> +            replacementPaths.uncheckedAppend(nullString());

so nullString in the result means "no transcoding"?

> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
> +    auto replacementURLs = transcodeImages(fileURLs, allowedMIMETypes);

You're calling these URLs but they are urlStrings; not URL objects.

[Said Abou-Hallawa]

Comment on attachment 403709 [details]
Patch

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

>> Source/WebKit/Platform/ImageUtilities.h:30
>> +Vector<String> transcodeImages(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);
> 
> It's not really clear from the signature that allowedMIMETypes is the set of possible destination MIME types, and that the return value is a set of new paths. If one of the input paths is not transcodable, how does the return value relate to the input paths?
> 
> I feel like this needs to be a more expressive API, maybe even a simple class, that makes it easier to use without error.
> 
> Also are these absolute paths or URLs? The call site makes them look like URLs.

Suppose the you have a markup like this: 

    <input type='file' accept="image/png,.jpeg,.gif">. 

In this case:
    acceptedFileExtensions = { ".jpeg", ".gif" }
    acceptedMIMETypes = { "image/png" }
    allowedFileExtensions = { "png", "jpeg", "gif", "heif", "heic" }
    allowedMimeTypes = { "image/png", "image/jpeg", "image/gif" }

Suppose the user selects the following images 

    paths = { image1.png, image2.gif, image3.jpeg, image4.heic, image5.heif }

then the result of this function can be something like this:

    transcodeImages() = { nullString(), nullString(), nullString(), tempImage123456.png, tempImageabcdef.png }

So WebCore knows that the first three images were not transcoded. So it creates three File objects with the original images. And it knows the last two images were transcoded, so it create two File objects with the replacement images.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:59
>> +    auto suffix = makeString('.', WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType));
> 
> What if preferredExtensionForMIMEType() is empty?

I think it should not.

CG supports a small subset of image formats for encoding. I expect they are the very well known image formats. And I do not expect any of them will have empty string for the preferred extensions. I can add assertions like these 

ASSERT(WebCore::MIMETypeRegistry::isSupportedImageMIMETypeForEncoding(mimeType));
ASSERT(!WebCore::MIMETypeRegistry::preferredExtensionForMIMEType(mimeType).isNull());

But I do not think I need to provide any fallback for the failure cases.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:65
>> +        return nullString();
> 
> This error case returns the same result as the "no translation needed" case, so the caller can't tell them apart.

I think this is not bad. If we fail to create the temporary file for the transcoded image, WebProcess will use the original image when creating the File object. This image file may not be displayable by WebKit and may not be accepted by the server. But we can't do anything better than this.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:91
>> +        return { };
> 
> Oh, all the images get the same destination MIME type? That's not what I expected from the signature. I thought it would be one entry in the vector per image.

Yes all the images whose MIME types are not in the list allowedMIMETypes will be transcoded for the first one in allowedMIMETypes which CG supports encoding to. Currently only HEIF images are the only ones that can fall in this category.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:98
>> +            replacementPaths.uncheckedAppend(nullString());
> 
> so nullString in the result means "no transcoding"?

Yes. This is correct.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
>> +    auto replacementURLs = transcodeImages(fileURLs, allowedMIMETypes);
> 
> You're calling these URLs but they are urlStrings; not URL objects.

I was just following the naming convention in this function.

Here are the signatures of functions that are involved in this scenario:

void WebOpenPanelResultListenerProxy::chooseFiles(const Vector<String>& filenames, const Vector<String>& allowedMIMETypes)
void WebPageProxy::didChooseFilesForOpenPanel(const Vector<String>& fileURLs, const Vector<String>& allowedMIMETypes)
void WebPage::didChooseFilesForOpenPanel(const Vector<String>& files, const Vector<String>& replacementFiles)
void WebOpenPanelResultListener::didChooseFiles(const Vector<String>& files, const Vector<String>& replacementFiles)
void FileChooser::chooseFiles(const Vector<String>& filenames, const Vector<String>& replacementNames)
void FileInputType::filesChosen(const Vector<FileChooserFileInfo>& paths, const String& displayString, Icon* icon)
Ref<File> File::create(const String& path, const String& replacementPath, const String& nameOverride)

The used names are: filenames, fileURLs, files, paths.

I really do not know which one I should use. I wished I can unify all the names and I prefer filenames or paths. If you want I can do this in the same patch or a separate patch before or after this one.

[Said Abou-Hallawa]

Comment on attachment 403709 [details]
Patch

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

>>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:65
>>> +        return nullString();
>> 
>> This error case returns the same result as the "no translation needed" case, so the caller can't tell them apart.
> 
> I think this is not bad. If we fail to create the temporary file for the transcoded image, WebProcess will use the original image when creating the File object. This image file may not be displayable by WebKit and may not be accepted by the server. But we can't do anything better than this.

Also see Darin's comment above where he suggested replacing 

    ASSERT_NOT_REACHED_WITH_MESSAGE("Replacement image could not be created.");

with a log message.

[Said Abou-Hallawa]

Created attachment 403849 [details]
Patch

[Said Abou-Hallawa]

I moved transcoding the images to a work queue to unblock the UI process main thread and allow the OpenPanel to be dismissed quickly. The Web process will be blocked till the message WebPage::DidChooseFilesForOpenPanel() is received from the transcoding work queue in the UI process.

[Said Abou-Hallawa]

Created attachment 403850 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 403850 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:6691
> +    auto transcodingUTI = WebCore::UTIFromMIMEType(transcodingMIMEType);

This call has to be made in the main thread because UTIFromMIMEType() has the following assertion:
ASSERT(isMainThread());

[Said Abou-Hallawa]

Comment on attachment 403850 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:6694
> +    m_transcodingQueue->dispatch([this, protectedThis = makeRef(*this), fileURLs = fileURLs, transcodingURLs = WTFMove(transcodingURLs), transcodingUTI = transcodingUTI, transcodingExtension = transcodingExtension]() mutable {

mutable is not needed. I will remove it.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
> +        RunLoop::main().dispatch([this, fileURLs = fileURLs, transcodedURLs = WTFMove(transcodedURLs)]() mutable {

Ditto.

[Darin Adler]

Comment on attachment 403850 [details]
Patch

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

> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:118
> +    CString suffixUtf8 = suffix.utf8();

I don’t understand why this function is using "Utf8" capitalization instead of "UTF8".

> Source/WTF/wtf/glib/FileSystemGlib.cpp:329
> -String openTemporaryFile(const String& prefix, PlatformFileHandle& handle)
> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle, const String&)

Seems bad that this platform simply ignores the suffix. Only OK because the feature isn’t used? Maybe:

    // FIXME: Suffix is not supported, but OK for now since the code using it is macOS-port-only.
    ASSERT_UNUSED(suffix, suffix.isEmpty());

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:445
> -String openTemporaryFile(const String& prefix, PlatformFileHandle& handle)
> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle, const String&)

Ditto.

> Source/WTF/wtf/win/FileSystemWin.cpp:407
> -String openTemporaryFile(const String&, PlatformFileHandle& handle)
> +String openTemporaryFile(const String&, PlatformFileHandle& handle, const String&)

Ditto.

> Source/WebKit/Platform/ImageUtilities.h:30
> +Vector<String> findImagesForTranscoding(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);

Probably need a comment explaining what the return value is.

> Source/WebKit/Platform/ImageUtilities.h:31
> +Vector<String> transcodeImages(const Vector<String>& paths, const String& uti, const String& extension);

Probably need a comment explaining what the return value is.

Maybe "destinationUTI" rather than "uti" and "destinationExtension" rather than "extension".

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:41
> +    // This function transcodes files only.
> +    if (FileSystem::fileIsDirectory(path, FileSystem::ShouldFollowSymbolicLinks::Yes))
> +        return nullString();

Why do we need this code? Won’t CGImageSourceCreateWithURL fail without this check?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:43
> +    // Ensure the 'path' is actually an image file.

This doesn’t seem like an accurate comment. This doesn’t "ensure it’s an image file". I don’t think the comment is needed.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:49
> +    // Ensure the format of the image source is different from 'mimeType'.

Comment says nothing different from what the code below does. Why is it a valuable to fail in this case? Do we have test coverage for this case? I suggest omitting this code.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:54
> +    // The file extension is important because it reveals the MIME type of the temporary file.

This is imprecise. What does "reveals" mean here? Do you mean that it helps software select the appropriate MIME type? What software are we trying to help here? Comment could cover that.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:67
> +            return FileSystem::writeToFile(handle, static_cast<const char*>(buffer), count);

Does this code do the correct thing when there is an error?

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:75
> +    // Write the source image to the destination image.

Should remove this comment.

> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:78
> +    FileSystem::closeFile(destinationFileHandle);

Does this code do the correct thing when there is an error?

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6694
>> +    m_transcodingQueue->dispatch([this, protectedThis = makeRef(*this), fileURLs = fileURLs, transcodingURLs = WTFMove(transcodingURLs), transcodingUTI = transcodingUTI, transcodingExtension = transcodingExtension]() mutable {
> 
> mutable is not needed. I will remove it.

It’s not safe to pass strings across threads. Their reference counts are not thread-safe. We need to pass isolated copies across threads.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
>> +        RunLoop::main().dispatch([this, fileURLs = fileURLs, transcodedURLs = WTFMove(transcodedURLs)]() mutable {
> 
> Ditto.

Ditto.

[Said Abou-Hallawa]

Comment on attachment 403850 [details]
Patch

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

>> Source/WTF/wtf/cocoa/FileSystemCocoa.mm:118
>> +    CString suffixUtf8 = suffix.utf8();
> 
> I don’t understand why this function is using "Utf8" capitalization instead of "UTF8".

Fixed.

>> Source/WTF/wtf/glib/FileSystemGlib.cpp:329
>> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle, const String&)
> 
> Seems bad that this platform simply ignores the suffix. Only OK because the feature isn’t used? Maybe:
> 
>     // FIXME: Suffix is not supported, but OK for now since the code using it is macOS-port-only.
>     ASSERT_UNUSED(suffix, suffix.isEmpty());

Done.

>> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:445
>> +String openTemporaryFile(const String& prefix, PlatformFileHandle& handle, const String&)
> 
> Ditto.

Done.

>> Source/WTF/wtf/win/FileSystemWin.cpp:407
>> +String openTemporaryFile(const String&, PlatformFileHandle& handle, const String&)
> 
> Ditto.

Done.

>> Source/WebKit/Platform/ImageUtilities.h:30
>> +Vector<String> findImagesForTranscoding(const Vector<String>& paths, const Vector<String>& allowedMIMETypes);
> 
> Probably need a comment explaining what the return value is.

A comment is added.

>> Source/WebKit/Platform/ImageUtilities.h:31
>> +Vector<String> transcodeImages(const Vector<String>& paths, const String& uti, const String& extension);
> 
> Probably need a comment explaining what the return value is.
> 
> Maybe "destinationUTI" rather than "uti" and "destinationExtension" rather than "extension".

A comment is added and the arguments' names are changed.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:41
>> +        return nullString();
> 
> Why do we need this code? Won’t CGImageSourceCreateWithURL fail without this check?

Yes this is correct. Code is removed.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:43
>> +    // Ensure the 'path' is actually an image file.
> 
> This doesn’t seem like an accurate comment. This doesn’t "ensure it’s an image file". I don’t think the comment is needed.

The comment is removed.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:49
>> +    // Ensure the format of the image source is different from 'mimeType'.
> 
> Comment says nothing different from what the code below does. Why is it a valuable to fail in this case? Do we have test coverage for this case? I suggest omitting this code.

We can fall to this case if path = "image.heic" and destinationUTI = "public.png" but image.heic is actually a PNG image with the wrong extension.

I kept the code but I removed the comment.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:54
>> +    // The file extension is important because it reveals the MIME type of the temporary file.
> 
> This is imprecise. What does "reveals" mean here? Do you mean that it helps software select the appropriate MIME type? What software are we trying to help here? Comment could cover that.

I added the following comment:

    // It is important to add the appropriate file extension to the temporary file path.
    // The File object depends solely on the extension to know the MIME type of the file.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:67
>> +            return FileSystem::writeToFile(handle, static_cast<const char*>(buffer), count);
> 
> Does this code do the correct thing when there is an error?

According to the header file CGDataConsumer.h, it should handle the case were the return value is 0 or the return value is less than count.

 * `putBytes' copies `count' bytes from `buffer' to the consumer, and
 *   returns the number of bytes copied. It should return 0 if no more data
 *   can be written to the consumer.

So if the image bytes could not be written by the consumer, CGImageDestinationFinalize() will return false.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:75
>> +    // Write the source image to the destination image.
> 
> Should remove this comment.

The comment is removed.

>> Source/WebKit/Platform/cg/ImageUtilitiesCG.cpp:78
>> +    FileSystem::closeFile(destinationFileHandle);
> 
> Does this code do the correct thing when there is an error?

I realized that CGImageDestinationFinalize() returns a bool indicating whether it succeeds or fails. If it fails we should delete the temporary file and return a null string.

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:6694
>>> +    m_transcodingQueue->dispatch([this, protectedThis = makeRef(*this), fileURLs = fileURLs, transcodingURLs = WTFMove(transcodingURLs), transcodingUTI = transcodingUTI, transcodingExtension = transcodingExtension]() mutable {
>> 
>> mutable is not needed. I will remove it.
> 
> It’s not safe to pass strings across threads. Their reference counts are not thread-safe. We need to pass isolated copies across threads.

I used isolatedCopy() for fileURLs and WTFMove for the rest of the arguments.

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
>>> +        RunLoop::main().dispatch([this, fileURLs = fileURLs, transcodedURLs = WTFMove(transcodedURLs)]() mutable {
>> 
>> Ditto.
> 
> Ditto.

I used isolatedCopy() for fileURLs and WTFMove for the rest of the arguments.

[Said Abou-Hallawa]

Created attachment 404048 [details]
Patch

[Darin Adler]

Comment on attachment 403850 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
>>>> +        RunLoop::main().dispatch([this, fileURLs = fileURLs, transcodedURLs = WTFMove(transcodedURLs)]() mutable {
>>> 
>>> Ditto.
>> 
>> Ditto.
> 
> I used isolatedCopy() for fileURLs and WTFMove for the rest of the arguments.

WTFMove may not be sufficient for arguments if they are reference counted and haven’t already been isolated. We have to go through each one carefully.

[Darin Adler]

Comment on attachment 404048 [details]
Patch

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

Would like to say review+ but I think the threading is still not quite right.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6694
> +    m_transcodingQueue->dispatch([this, protectedThis = makeRef(*this), fileURLs = fileURLs.isolatedCopy(), transcodingURLs = WTFMove(transcodingURLs), transcodingUTI = WTFMove(transcodingUTI), transcodingExtension = WTFMove(transcodingExtension)]() {

We need isolated copies for the last three captured things. There are many ways of making strings that do not yield an isolated string ready to be safely passed to another thread. Assuming that because we created the string that we are the sole owner is not necessarily correct. And also, strings that come from something like the MIME type registry are definitely not going to be isolated.

I also think that getting the string isolation wrong typically leads to really hard to reproduce bugs, no obvious easily noticeable failures.

> Source/WebKit/UIProcess/WebPageProxy.cpp:6700
> +        RunLoop::main().dispatch([this, fileURLs = fileURLs.isolatedCopy(), transcodedURLs = WTFMove(transcodedURLs)]() {

We need to capture protectedThis here or allocate a new one.

Also, here *maybe* we can get away with WTFMove for transcodedURLs, but I am not sure and I think isolatedCopy may be required. Those same issues as above likely apply.

[Said Abou-Hallawa]

Created attachment 404084 [details]
Patch

[EWS]

Committed r264286: <https://trac.webkit.org/changeset/264286>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404084 [details].