Bug 202077 – Safari 13 may launch leftover 32-bit plug-in process from Safari 12’s WebKit, which crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

rdar://problem/55547063

On macOS High Sierra, if Safari 13 was installed after Safari 12 had been installed, Safari may end up launching the 32-bit plug-in service left behind in the staged WebKit framework, and the service will crash immediately (because as of Safari 13, its dependencies don’t have a 32-bit slice). This can happen, for example, if there is still a 32-bit plug-in in /Library/Internet Plug-Ins, and the user chooses to manage website data.

This is because even though the 32-bit plug-in service has been removed from WebKit, the code paths that launch it haven’t been removed.

Patch forthcoming.

Created attachment 379323 [details]
Remove 32-bit plug-in code paths

[Sam Weinig]

Comment on attachment 379323 [details]
Remove 32-bit plug-in code paths

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

> Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:60
>      case ProcessLauncher::ProcessType::Plugin32:

What is the reason for keeping this enum case at all?

(In reply to Sam Weinig from comment #2)
> Comment on attachment 379323 [details]
> Remove 32-bit plug-in code paths
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379323&action=review
> 
> > Source/WebKit/UIProcess/Launcher/mac/ProcessLauncherMac.mm:60
> >      case ProcessLauncher::ProcessType::Plugin32:
> 
> What is the reason for keeping this enum case at all?

ProcessType is a cross-platform type, and I didn’t want to expand the scope of this patch beyond Mac code.

[Sam Weinig]

Gotcha.

[WebKit Commit Bot]

Comment on attachment 379323 [details]
Remove 32-bit plug-in code paths

Clearing flags on attachment: 379323

Committed r250186: <https://trac.webkit.org/changeset/250186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This is causing a lot of crashes on the bots (They exit early):
https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r250186%20(4826)/results.html

[Chris Dumez]

Comment on attachment 379323 [details]
Remove 32-bit plug-in code paths

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

> Source/WebKit/UIProcess/Plugins/mac/PluginProcessProxyMac.mm:63
> +    ASSERT(pluginProcessAttributes.moduleInfo.pluginArchitecture == CPU_TYPE_X86);

Looks like this assertion is hitting.

(In reply to Chris Dumez from comment #8)
> Comment on attachment 379323 [details]
> Remove 32-bit plug-in code paths
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379323&action=review
> 
> > Source/WebKit/UIProcess/Plugins/mac/PluginProcessProxyMac.mm:63
> > +    ASSERT(pluginProcessAttributes.moduleInfo.pluginArchitecture == CPU_TYPE_X86);
> 
> Looks like this assertion is hitting.

I’ll see if I can reproduce this locally.

(In reply to mitz from comment #9)
> (In reply to Chris Dumez from comment #8)
> > Comment on attachment 379323 [details]
> > Remove 32-bit plug-in code paths
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=379323&action=review
> > 
> > > Source/WebKit/UIProcess/Plugins/mac/PluginProcessProxyMac.mm:63
> > > +    ASSERT(pluginProcessAttributes.moduleInfo.pluginArchitecture == CPU_TYPE_X86);

Silly mistake, this should say CPU_TYPE_X86_64. I’ll fix it right away. Thanks for noticing this!

[Chris Dumez]

Comment on attachment 379323 [details]
Remove 32-bit plug-in code paths

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

>>> Source/WebKit/UIProcess/Plugins/mac/PluginProcessProxyMac.mm:63
>>> +    ASSERT(pluginProcessAttributes.moduleInfo.pluginArchitecture == CPU_TYPE_X86);
>> 
>> Looks like this assertion is hitting.
> 
> I’ll see if I can reproduce this locally.

Shouldn’t this be != instead of == ?

[Chris Dumez]

(In reply to mitz from comment #10)
> (In reply to mitz from comment #9)
> > (In reply to Chris Dumez from comment #8)
> > > Comment on attachment 379323 [details]
> > > Remove 32-bit plug-in code paths
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=379323&action=review
> > > 
> > > > Source/WebKit/UIProcess/Plugins/mac/PluginProcessProxyMac.mm:63
> > > > +    ASSERT(pluginProcessAttributes.moduleInfo.pluginArchitecture == CPU_TYPE_X86);
> 
> Silly mistake, this should say CPU_TYPE_X86_64. I’ll fix it right away.
> Thanks for noticing this!

Oh just saw your comment, glad this is an easy fix.

Corrected the assertion in r250197.