Bug 202077 - Safari 13 may launch leftover 32-bit plug-in process from Safari 12’s WebKit, which crashes
Summary: Safari 13 may launch leftover 32-bit plug-in process from Safari 12’s WebKit,...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-21 14:16 PDT by mitz
Modified: 2019-09-21 23:13 PDT (History)
5 users (show)

See Also:


Attachments
Remove 32-bit plug-in code paths (3.60 KB, patch)
2019-09-21 14:32 PDT, mitz
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2019-09-21 14:16:39 PDT
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.
Comment 1 mitz 2019-09-21 14:32:06 PDT
Created attachment 379323 [details]
Remove 32-bit plug-in code paths
Comment 2 Sam Weinig 2019-09-21 14:42:46 PDT
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?
Comment 3 mitz 2019-09-21 14:48:37 PDT
(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.
Comment 4 Sam Weinig 2019-09-21 14:58:14 PDT
Gotcha.
Comment 5 WebKit Commit Bot 2019-09-21 15:57:53 PDT
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>
Comment 6 WebKit Commit Bot 2019-09-21 15:57:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Chris Dumez 2019-09-21 23:05:48 PDT
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
Comment 8 Chris Dumez 2019-09-21 23:06:53 PDT
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.
Comment 9 mitz 2019-09-21 23:07:27 PDT
(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.
Comment 10 mitz 2019-09-21 23:10:33 PDT
(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!
Comment 11 Chris Dumez 2019-09-21 23:11:14 PDT
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 == ?
Comment 12 Chris Dumez 2019-09-21 23:13:19 PDT
(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.
Comment 13 mitz 2019-09-21 23:13:29 PDT
Corrected the assertion in r250197.