Bug 47700 – Make mime type lookup in File::create(path) thread-safe

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Kinuko Yasuda]

Make mime type lookup in File::create(path) thread-safe.  This is dangerous when this code runs on workers.

Currently when we create a File object (WebCore/fileapi/File) from a file path, it calls MIMETypeRegistry::getMIMETypeForExtension() to initialize File.type based on the given path.
But this is not thread-safe on many platforms.

[Dai Mikurube]

Created attachment 79894 [details]
Patch

[Dai Mikurube]

Hi,

The patch is not to be reviewed (not tested!), but what do you think about it?

[Kinuko Yasuda]

Comment on attachment 79894 [details]
Patch

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

Thanks for uploading this.  You'd want to wait for David's (or someone who knows very well) comments, but I put a few minor ones:

> Source/WebCore/ChangeLog:20
> +        * platform/MIMETypeRegistry.h:

It is generally encouraged to put short change summary (ideally) for each file, maybe the first two files may deserve short summary in this patch?

> Source/WebCore/platform/android/TemporaryLinkStubs.cpp:310
> +    ASSERT_NOT_REACHED();

This shouldn't compile if this happens right?

How about changing this to more generic check like:
ASSERT(isMainThread());
without guards, or only with ENABLE(WORKERS).

[Dai Mikurube]

Created attachment 80040 [details]
Patch

[Dai Mikurube]

Comment on attachment 79894 [details]
Patch

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

Hi Kinuko-san,

Thank you for your comments.

>> Source/WebCore/ChangeLog:20
>> +        * platform/MIMETypeRegistry.h:
> 
> It is generally encouraged to put short change summary (ideally) for each file, maybe the first two files may deserve short summary in this patch?

Exactly. I've added summary.

>> Source/WebCore/platform/android/TemporaryLinkStubs.cpp:310
>> +    ASSERT_NOT_REACHED();
> 
> This shouldn't compile if this happens right?
> 
> How about changing this to more generic check like:
> ASSERT(isMainThread());
> without guards, or only with ENABLE(WORKERS).

Changed as your advice. Thanks.

[Kinuko Yasuda]

lgtm; now you'll need a real reviewer.

[Adam Barth]

Comment on attachment 80040 [details]
Patch

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

> Source/WebCore/platform/mac/MIMETypeRegistryMac.mm:39
> +#if ENABLE(WORKERS)
> +    ASSERT(isMainThread());
> +#endif

Can we make these asserts even if we haven't enabled workers?

[Dai Mikurube]

Created attachment 80710 [details]
Patch

[Dai Mikurube]

(In reply to comment #7)
HI Adam,

Thank you for your review. I've removed #if ENABLED(WORKERS).

[David Levin]

Comment on attachment 80710 [details]
Patch

This looks good. One last thing to fix up wherever you added ASSERT(isMainThread()); please make sure that the file has the following includes:

#include <wtf/Assertions.h>
#include <wtf/MainThread.h>

I spot checked MIMETypeRegistryWinCE.cpp and MimeTypeRegistryWx.cpp and noticed they didn't have these so those builds may get broken by this change.

[Dai Mikurube]

Created attachment 80898 [details]
Patch

[Dai Mikurube]

(In reply to comment #10)
Hi Levin,

Thank you for your review. Added the header files!

[WebKit Commit Bot]

Comment on attachment 80898 [details]
Patch

Clearing flags on attachment: 80898

Committed r77368: <http://trac.webkit.org/changeset/77368>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

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