Bug 206478 – [Cocoa] Media mime types map should be created in the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Creating this map in the WebContent process will access the launch services daemon, which will be blocked.

[Radar WebKit Bug Importer]

<rdar://problem/58714891>

[Per Arne Vollan]

Created attachment 388163 [details]
Patch

[Per Arne Vollan]

Created attachment 388164 [details]
Patch

[Per Arne Vollan]

Created attachment 388165 [details]
Patch

[Per Arne Vollan]

Created attachment 388166 [details]
Patch

[Per Arne Vollan]

Created attachment 388167 [details]
Patch

[Sam Weinig]

What happens if the mapping of mime types reported by launch services changes during the lifetime of the process?

[Darin Adler]

Comment on attachment 388167 [details]
Patch

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

Not sure what the Windows failure on EWS is about.

> Source/WebCore/platform/MIMETypeRegistry.cpp:285
> +    // A table of common media MIME types and file extentions used when a platform's

Spelling error here. It’s extensions, not "extentions".

> Source/WebCore/platform/MIMETypeRegistry.cpp:287
> +    static NeverDestroyed<std::initializer_list<TypeExtensionPair>> commonMediaTypes;

Should be able to use constexpr for this whole thing rather than needing code to initialize it.

> Source/WebCore/platform/MIMETypeRegistry.cpp:290
> +    static std::once_flag onceFlag;
> +    std::call_once(

Surprised we need this. Is this called from multiple threads? If not then we should just be able to use "= [] {" instead of all the call_once stuff as is done in many other places in WebKit such as canAtttachAuthorShadowRoot, hasEffectiveDisplayNoneForDisplayContents, etc.

> Source/WebCore/platform/MIMETypeRegistry.cpp:381
> +    static std::once_flag onceFlag;
> +    std::call_once(

Ditto.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #7)
> What happens if the mapping of mime types reported by launch services
> changes during the lifetime of the process?

Good point! There should be no behavior change with this patch. The only difference is that we create the cached map in the UI process instead of in the WebContent process.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Darin Adler from comment #8)
> Comment on attachment 388167 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388167&action=review
> 
> Not sure what the Windows failure on EWS is about.
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:285
> > +    // A table of common media MIME types and file extentions used when a platform's
> 
> Spelling error here. It’s extensions, not "extentions".
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:287
> > +    static NeverDestroyed<std::initializer_list<TypeExtensionPair>> commonMediaTypes;
> 
> Should be able to use constexpr for this whole thing rather than needing
> code to initialize it.
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:290
> > +    static std::once_flag onceFlag;
> > +    std::call_once(
> 
> Surprised we need this. Is this called from multiple threads? If not then we
> should just be able to use "= [] {" instead of all the call_once stuff as is
> done in many other places in WebKit such as canAtttachAuthorShadowRoot,
> hasEffectiveDisplayNoneForDisplayContents, etc.
> 
> > Source/WebCore/platform/MIMETypeRegistry.cpp:381
> > +    static std::once_flag onceFlag;
> > +    std::call_once(
> 
> Ditto.

You're right. I don't believe we need this, since the function did not use call_once originally. I will update the patch, possibly asserting that this code is being run on the main thread.

Thanks for reviewing!

[Sam Weinig]

(In reply to Per Arne Vollan from comment #9)
> (In reply to Sam Weinig from comment #7)
> > What happens if the mapping of mime types reported by launch services
> > changes during the lifetime of the process?
> 
> Good point! There should be no behavior change with this patch. The only
> difference is that we create the cached map in the UI process instead of in
> the WebContent process.
> 

Sounds like there would be a subtle behavior change then, since the cache will now not be invalidated for the life of the UIProcess, whereas, on each WebProcess creation we used to get a fresh copy.

[Per Arne Vollan]

(In reply to Sam Weinig from comment #11)
> (In reply to Per Arne Vollan from comment #9)
> > (In reply to Sam Weinig from comment #7)
> > > What happens if the mapping of mime types reported by launch services
> > > changes during the lifetime of the process?
> > 
> > Good point! There should be no behavior change with this patch. The only
> > difference is that we create the cached map in the UI process instead of in
> > the WebContent process.
> > 
> 
> Sounds like there would be a subtle behavior change then, since the cache
> will now not be invalidated for the life of the UIProcess, whereas, on each
> WebProcess creation we used to get a fresh copy.

Ah, yes, that is a good point! We could possibly recreate the map every time we create a new WebContent process, but that would be less performant than just using the previous one.

[Per Arne Vollan]

Created attachment 388595 [details]
Patch

[Per Arne Vollan]

Created attachment 388601 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 388601 [details]
Patch

Clearing flags on attachment: 388601

Committed r255050: <https://trac.webkit.org/changeset/255050>

[Sam Weinig]

Probably worth investigating whether one already exists / filing a radar for a callback from launch services that tells you if it changes.a

[Per Arne Vollan]

Reverted r255050 for reason:

Introduced crashes on bots

Committed r255081: <https://trac.webkit.org/changeset/255081>

[Per Arne Vollan]

Created attachment 388714 [details]
Patch

[Per Arne Vollan]

(In reply to Sam Weinig from comment #16)
> Probably worth investigating whether one already exists / filing a radar for
> a callback from launch services that tells you if it changes.a

Thanks! I am looking into this now.

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #19)
> (In reply to Sam Weinig from comment #16)
> > Probably worth investigating whether one already exists / filing a radar for
> > a callback from launch services that tells you if it changes.a
> 
> Thanks! I am looking into this now.

There appears to be an existing mechanism for this.

[WebKit Commit Bot]

Comment on attachment 388714 [details]
Patch

Clearing flags on attachment: 388714

Committed r255119: <https://trac.webkit.org/changeset/255119>