[webkit-reviews] review denied: [Bug 213347] [macOS]: A HEIF image, selected from the OpenPanel, should be converted to an accepted MIME type : [Attachment 404048] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 213347: [macOS]: A HEIF image, selected from the OpenPanel, should be
converted to an accepted MIME type
https://bugs.webkit.org/show_bug.cgi?id=213347

Attachment 404048: Patch

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




--- Comment #47 from Darin Adler <darin at apple.com> ---
Comment on attachment 404048
  --> https://bugs.webkit.org/attachment.cgi?id=404048
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.