Bug 185611 – [GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

• Component: New Bugs
• Status: RESOLVED FIXED

[Thibault Saunier]

Without that, I am getting the following segfault when running tests:

[Thibault Saunier]

Created attachment 340313 [details]
[GTK][WPE]: Avoid trying to getenv with an unset varname

Otherwise we might get segfault

[EWS Watchlist]

Comment on attachment 340313 [details]
[GTK][WPE]: Avoid trying to getenv with an unset varname

Attachment 340313 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7677915

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 340321 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Thibault Saunier]

(In reply to Build Bot from comment #2)
> Comment on attachment 340313 [details]
> [GTK][WPE]: Avoid trying to getenv with an unset varname
> 
> Attachment 340313 [details] did not pass win-ews (win):
> Output: http://webkit-queues.webkit.org/results/7677915
> 
> New failing tests:
> http/tests/preload/onload_event.html

Unrelated?

[Zan Dobersek]

Comment on attachment 340313 [details]
[GTK][WPE]: Avoid trying to getenv with an unset varname

The early return would fit better inside the switch statement, under the default label. OTOH all the possible values of the ProcessType enum are handled there, so that's a soft guarantee that the early return isn't required at all.

Is this suppressing any real-world problem?

[Xabier Rodríguez Calvar]

Comment on attachment 340313 [details]
[GTK][WPE]: Avoid trying to getenv with an unset varname

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

> Source/WebKit/ChangeLog:8
> +        Otherwise we might segfault

Missing a period at the end.

[Michael Catanzaro]

Comment on attachment 340313 [details]
[GTK][WPE]: Avoid trying to getenv with an unset varname

Good catch, Zan. Indeed, something else must be wrong here for launchOptions.processType to be invalid. The switch would benefit from an ASSERT_NOT_REACHED() in the default case.

[Thibault Saunier]

(In reply to Zan Dobersek from comment #5)
> Comment on attachment 340313 [details]
> [GTK][WPE]: Avoid trying to getenv with an unset varname
> 
> The early return would fit better inside the switch statement, under the
> default label. OTOH all the possible values of the ProcessType enum are
> handled there, so that's a soft guarantee that the early return isn't
> required at all.
> 
> Is this suppressing any real-world problem?

Yes, I am getting a segfault running the layout tests:

#0  0x00007f9f16830621 in getenv () from /usr/lib/libc.so.6
#1  0x00007f9f2e47999b in WebKit::ChildProcessProxy::getLaunchOptions (this=0x7f9f0d1b0000, launchOptions=...) at ../../Source/WebKit/UIProcess/ChildProcessProxy.cpp:79
#2  0x00007f9f2e6c487d in WebKit::PluginProcessProxy::getLaunchOptions (this=0x7f9f0d1b0000, launchOptions=...) at ../../Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:94
#3  0x00007f9f2e479a8a in WebKit::ChildProcessProxy::connect (this=0x7f9f0d1b0000) at ../../Source/WebKit/UIProcess/ChildProcessProxy.cpp:89
#4  0x00007f9f2e6c461a in WebKit::PluginProcessProxy::PluginProcessProxy (this=0x7f9f0d1b0000, PluginProcessManager=0x7f9f36b8b4c0 <WebKit::PluginProcessManager::singleton()::pluginProcessManager>, pluginProcessAttributes=..., pluginProcessToken=1468176759767222162) at ../../Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:78
#5  0x00007f9f2e6c44bc in WebKit::PluginProcessProxy::create (PluginProcessManager=0x7f9f36b8b4c0 <WebKit::PluginProcessManager::singleton()::pluginProcessManager>, pluginProcessAttributes=..., pluginProcessToken=1468176759767222162) at ../../Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:64
#6  0x00007f9f2e6c24ef in WebKit::PluginProcessManager::getOrCreatePluginProcess (this=0x7f9f36b8b4c0 <WebKit::PluginProcessManager::singleton()::pluginProcessManager>, pluginProcessToken=1468176759767222162) at ../../Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:141
#7  0x00007f9f2e6c2214 in WebKit::PluginProcessManager::fetchWebsiteData(WebKit::PluginModuleInfo const&, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::Function<void (WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul>)>&&) (this=0x7f9f36b8b4c0 <WebKit::PluginProcessManager::singleton()::pluginProcessManager>, plugin=..., fetchOptions=..., completionHandler=...) at ../../Source/WebKit/UIProcess/Plugins/PluginProcessManager.cpp:103
#8  0x00007f9f2e6eba85 in WebKit::WebsiteDataStore::State::fetchWebsiteDataForNextPlugin (this=0x55ff51c35550) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:545
#9  0x00007f9f2e6eb7bd in WebKit::WebsiteDataStore::State::State(WTF::Ref<WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue>&&, WTF::Function<void(WTF::Vector<WebKit::WebsiteDataRecord>)>&&)::CallbackAggregator, WTF::DumbPtrTraits<WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue>&&, WTF::Function<void(WTF::Vector<WebKit::WebsiteDataRecord>)>&&)::CallbackAggregator> > &&, WTF::Vector<WebKit::PluginModuleInfo, 0, WTF::CrashOnOverflow, 16> &&) (this=0x55ff51c35550, callbackAggregator=..., plugins=...) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:524
#10 0x00007f9f2e6eb734 in WebKit::WebsiteDataStore::State::fetchData(WTF::Ref<WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue>&&, WTF::Function<void(WTF::Vector<WebKit::WebsiteDataRecord>)>&&)::CallbackAggregator, WTF::DumbPtrTraits<WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue>&&, WTF::Function<void(WTF::Vector<WebKit::WebsiteDataRecord>)>&&)::CallbackAggregator> > &&, WTF::Vector<WebKit::PluginModuleInfo, 0, WTF::CrashOnOverflow, 16> &&) (callbackAggregator=..., plugins=...) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:514
#11 0x00007f9f2e6ec8de in WebKit::WebsiteDataStore::fetchDataAndApply(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::RefPtr<WTF::WorkQueue, WTF::DumbPtrTraits<WTF::WorkQueue> >&&, WTF::Function<void (WTF::Vector<WebKit::WebsiteDataRecord, 0ul, WTF::CrashOnOverflow, 16ul>)>&&) (this=0x7f9f0d1e4000, dataTypes=..., fetchOptions=..., queue=..., apply=...) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:557
#12 0x00007f9f2e6e9517 in WebKit::WebsiteDataStore::fetchData(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::Function<void (WTF::Vector<WebKit::WebsiteDataRecord, 0ul, WTF::CrashOnOverflow, 16ul>)>&&) (this=0x7f9f0d1e4000, dataTypes=..., fetchOptions=..., completionHandler=...) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:225
#13 0x00007f9f2e6ecb1f in WebKit::WebsiteDataStore::topPrivatelyControlledDomainsWithWebsiteData(WTF::OptionSet<WebKit::WebsiteDataType>, WTF::OptionSet<WebKit::WebsiteDataFetchOption>, WTF::Function<void (WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> >&&)>&&) (this=0x7f9f0d1e4000, dataTypes=..., fetchOptions=..., completionHandler=...) at ../../Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:588
#14 0x00007f9f2e5a6b9a in WebKit::WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(WTF::OptionSet<WebKit::WebsiteDataType>, bool, WTF::Function<void (WTF::HashSet<WTF::String, WTF::StringHash, WTF::HashTraits<WTF::String> >&&)>&&) (dataTypes=..., shouldNotifyPage=false, completionHandler=...) at ../../Source/WebKit/UIProcess/WebProcessProxy.cpp:353
#15 0x00007f9f2eb66cab in WebKit::WebResourceLoadStatisticsStore::<lambda()>::operator()(void) (__closure=0x7f9f0d1d0008) at ../../Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:492
#16 0x00007f9f2eb97010 in WTF::Function<void()>::CallableWrapper<WebKit::WebResourceLoadStatisticsStore::grandfatherExistingWebsiteData(WTF::CompletionHandler<void()>&&)::<lambda()> >::call(void) (this=0x7f9f0d1d0000) at DerivedSources/ForwardingHeaders/wtf/Function.h:101
#17 0x000055ff50e7557c in WTF::Function<void ()>::operator()() const (this=0x7ffee374eb60) at ../../Source/WTF/wtf/Function.h:56
#18 0x000055ff50e889f2 in WTF::RunLoop::performWork (this=0x7f9f0d1f7000) at ../../Source/WTF/wtf/RunLoop.cpp:106
#19 0x000055ff50edab40 in WTF::RunLoop::<lambda(gpointer)>::operator()(gpointer) const (__closure=0x0, userData=0x7f9f0d1f7000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:68
#20 0x000055ff50edab64 in WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:70
#21 0x000055ff50edaaf2 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x55ff51ade160, callback=0x55ff50edab47 <WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer)>, userData=0x7f9f0d1f7000) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45
#22 0x000055ff50edab22 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:46
#23 0x00007f9f23d6a8f8 in g_main_dispatch () at /home/thiblahute/devel/Webkit/webkit-mediastream/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3148
#24 g_main_context_dispatch () at /home/thiblahute/devel/Webkit/webkit-mediastream/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3813
#25 0x00007f9f23d6ace8 in g_main_context_iterate () at /home/thiblahute/devel/Webkit/webkit-mediastream/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3886
#26 0x00007f9f23d6ad7c in g_main_context_iteration () at /home/thiblahute/devel/Webkit/webkit-mediastream/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gmain.c:3947
#27 0x00007f9f253c7a75 in gtk_main_iteration () at /home/thiblahute/devel/Webkit/webkit-mediastream/WebKitBuild/DependenciesGTK/Source/gtk+-3.22.11/gtk/gtkmain.c:1413
#28 0x000055ff50e5310d in WTR::PlatformWebView::PlatformWebView (this=0x55ff51adf2c0, configuration=0x7f9f0d1ce000, options=...) at ../../Tools/WebKitTestRunner/gtk/PlatformWebViewGtk.cpp:54
#29 0x000055ff50e20bdb in std::make_unique<WTR::PlatformWebView, OpaqueWKPageConfiguration const*&, WTR::TestOptions const&> (__args#0=@0x7ffee374ee30: 0x7f9f0d1ce000, __args#1=...) at /usr/include/c++/8.1.0/bits/unique_ptr.h:831
#30 0x000055ff50e1840c in WTR::TestController::platformCreateWebView (this=0x7ffee374fd40, configuration=0x7f9f0d1ce000, options=...) at ../../Tools/WebKitTestRunner/TestController.cpp:2408
#31 0x000055ff50e0ff26 in WTR::TestController::createWebViewWithOptions (this=0x7ffee374fd40, options=...) at ../../Tools/WebKitTestRunner/TestController.cpp:520
#32 0x000055ff50e10464 in WTR::TestController::ensureViewSupportsOptionsForTest (this=0x7ffee374fd40, test=...) at ../../Tools/WebKitTestRunner/TestController.cpp:668
#33 0x000055ff50e1271b in WTR::TestController::configureViewForTest (this=0x7ffee374fd40, test=...) at ../../Tools/WebKitTestRunner/TestController.cpp:1156
#34 0x000055ff50e2c0b9 in WTR::TestInvocation::invoke (this=0x55ff51adf4a0) at ../../Tools/WebKitTestRunner/TestInvocation.cpp:152
#35 0x000055ff50e1309d in WTR::TestController::runTest (this=0x7ffee374fd40, inputLine=0x7ffee374f4a0 "/home/thiblahute/devel/Webkit/webkit-mediastream/LayoutTests/fast/mediastream/MediaStreamConstructor.html'--timeout'30000") at ../../Tools/WebKitTestRunner/TestController.cpp:1265
#36 0x000055ff50e131ae in WTR::TestController::runTestingServerLoop (this=0x7ffee374fd40) at ../../Tools/WebKitTestRunner/TestController.cpp:1282
#37 0x000055ff50e131fc in WTR::TestController::run (this=0x7ffee374fd40) at ../../Tools/WebKitTestRunner/TestController.cpp:1290
#38 0x000055ff50e0e405 in WTR::TestController::TestController (this=0x7ffee374fd40, argc=2, argv=0x7ffee3750018) at ../../Tools/WebKitTestRunner/TestController.cpp:129
#39 0x000055ff50e545e0 in main (argc=2, argv=0x7ffee3750018) at ../../Tools/WebKitTestRunner/gtk/main.cpp:45


> Good catch, Zan. Indeed, something else must be wrong here for launchOptions.processType to be invalid. The switch would benefit from an ASSERT_NOT_REACHED() in the default case.


Let me dig a bit more :-)

[Thibault Saunier]

Looking at the previously pasted stacktrace it looks like we are passing an unset `launchOptions)` to `getLaunchOptions` in `void ChildProcessProxy::connect()`:

```
void ChildProcessProxy::connect()
{
    ASSERT(!m_processLauncher);
    ProcessLauncher::LaunchOptions launchOptions;
    getLaunchOptions(launchOptions);
```

I have no idea what should be set as a `launchOptions.processType` there, any idea?

[Michael Catanzaro]

(In reply to Thibault Saunier from comment #9)
> I have no idea what should be set as a `launchOptions.processType` there,
> any idea?

It's supposed to be set by the child class override of ChildProcessProxy::getLaunchOptions.

Look here:

void PluginProcessProxy::getLaunchOptions(ProcessLauncher::LaunchOptions& launchOptions)
{
    ChildProcessProxy::getLaunchOptions(launchOptions);
    platformGetLaunchOptions(launchOptions, m_pluginProcessAttributes);
}

That looks like the bug: the process type is set inside PluginProcessProxy::platformGetLaunchOptions, but that's too late because ChildProcessProxy expects it to have been set already.

I *think* it should be safe to just swap the order of those function calls, because it *looks* like nothing inside PluginProcessProxy::platformGetLaunchOptions will break for either Mac or GTK.

To ensure this is more robust, we really need a default ASSERT_NOT_REACHED() case inside ChildProcessProxy::getLaunchOptions.

[Thibault Saunier]

Created attachment 340429 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

Otherwise we might get segfault

[Thibault Saunier]

(In reply to Michael Catanzaro from comment #10)
> I *think* it should be safe to just swap the order of those function calls,
> because it *looks* like nothing inside
> PluginProcessProxy::platformGetLaunchOptions will break for either Mac or
> GTK.

Done that and it seem to fix the issue indeed.
 
> To ensure this is more robust, we really need a default ASSERT_NOT_REACHED()
> case inside ChildProcessProxy::getLaunchOptions.

Also added the assertion.

[Michael Catanzaro]

Comment on attachment 340429 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

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

> Source/WebKit/ChangeLog:12
> +        https://bugs.webkit.org/show_bug.cgi?id=185611

It's redundant as you already have it up above.

> Source/WebKit/UIProcess/ChildProcessProxy.cpp:80
> +        return;

Always assume the assertion never fails! So remove the return, since it's supposed to be unreachable. Just let it crash on the next line if the assertion fails.

[Michael Catanzaro]

Comment on attachment 340429 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

This actually needs to be reviewed by an owner. Alex, could you check it please?

[Thibault Saunier]

Comment on attachment 340429 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

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

>> Source/WebKit/UIProcess/ChildProcessProxy.cpp:80
>> +        return;
> 
> Always assume the assertion never fails! So remove the return, since it's supposed to be unreachable. Just let it crash on the next line if the assertion fails.

I don't call that being resilient :-). but well... as you whish.

[Thibault Saunier]

Created attachment 340488 [details]
Address comments

[Carlos Garcia Campos]

Comment on attachment 340488 [details]
Address comments

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

> Source/WebKit/UIProcess/ChildProcessProxy.cpp:79
> +    default:
> +        ASSERT_NOT_REACHED();

I don't think this is the right fix, this should never happen, we are handling all possible values in the switch.

> Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:95
> -    ChildProcessProxy::getLaunchOptions(launchOptions);
>      platformGetLaunchOptions(launchOptions, m_pluginProcessAttributes);
> +    ChildProcessProxy::getLaunchOptions(launchOptions);

This doesn't look correct either. I think the problem is that most of the child processes are calling connect(), which is virtual, in the constructor. The right fix might be to call connect from the create() methods after the object is constructed like WebProcessProxy does.

[Carlos Garcia Campos]

Comment on attachment 340488 [details]
Address comments

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

>> Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:95
>> +    ChildProcessProxy::getLaunchOptions(launchOptions);
> 
> This doesn't look correct either. I think the problem is that most of the child processes are calling connect(), which is virtual, in the constructor. The right fix might be to call connect from the create() methods after the object is constructed like WebProcessProxy does.

I'm sorry, this is indeed correct. platform impl is the one setting the processType, so it should go first. I still think it's weird to add a default to a switch where all options are handled.

[Thibault Saunier]

Created attachment 341270 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

Otherwise we might get segfault

[Carlos Garcia Campos]

Comment on attachment 341270 [details]
[GTK][WPE]: Avoid using uninitialized launchOptions in getLaunchOptions

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

LGTM, but needs to be approved by a WebKit2 owner.

> Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:61
> +        None,

I would move this at the beginning.

[Thibault Saunier]

(In reply to Carlos Garcia Campos from comment #20)
> > Source/WebKit/UIProcess/Launcher/ProcessLauncher.h:61
> > +        None,
> 
> I would move this at the beginning.

Yeah, I add a doubt, I guess it would be fine as it is not API.

[Carlos Garcia Campos]

Note that I think we should still remove the calls to connect() from constructors, unless I'm wring and calling virtual methods in constructors is a good idea. But it can be done in a separate patch.

[Michael Catanzaro]

All the Apple EWS bots are red.

[Thibault Saunier]

Created attachment 341289 [details]
Update to avoid `enumeration value 'None' not handled in switch` issues in Apple's code.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #20)
> LGTM, but needs to be approved by a WebKit2 owner.

Ditto

[Carlos Garcia Campos]

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

[Michael Catanzaro]

Ping WK2 owners

[Chris Dumez]

Comment on attachment 341289 [details]
Update to avoid `enumeration value 'None' not handled in switch` issues in Apple's code.

Looks to me that the problem is that GTK's implementation of PluginProcessProxy::platformGetLaunchOptions() fails to set launchOptions.processType.

[Carlos Bentzen]

(In reply to Chris Dumez from comment #28)
> Comment on attachment 341289 [details]
> Update to avoid `enumeration value 'None' not handled in switch` issues in
> Apple's code.
> 
> Looks to me that the problem is that GTK's implementation of
> PluginProcessProxy::platformGetLaunchOptions() fails to set
> launchOptions.processType.

It does set launchOptions.processType correctly:

// in PluginProcessProxyUnix.cpp
void PluginProcessProxy::platformGetLaunchOptions(ProcessLauncher::LaunchOptions& launchOptions, const PluginProcessAttributes& pluginProcessAttributes)
{
    launchOptions.processType = ProcessLauncher::ProcessType::Plugin64;
    ...
}

But the call to PluginProcessProxy::platformGetLaunchOptions() goes after ChildProcessProxy::getLaunchOptions() (where the crash happens) in PluginProcessProxy::getLaunchOptions(). This patch fixes exactly that by changing the order of the calls.

The crash does not happen in Apple ports because the getenv call is inside #if ENABLE(DEVELOPER_MODE) && (PLATFORM(GTK) || PLATFORM(WPE)).

I believe launchOptions.processType will arrive uninitialized at ChildProcessProxy::getLaunchOptions() in Apple ports as well, but it just isn't used at that point yet.

[Chris Dumez]

Ok, I may have misread. I will take another look shortly, when I am back at a computer.

[Chris Dumez]

Comment on attachment 341289 [details]
Update to avoid `enumeration value 'None' not handled in switch` issues in Apple's code.

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

> Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:92
> +    ChildProcessProxy::getLaunchOptions(launchOptions);

Yes, we definitely need to swap the 2 instructions here. Why isn't this enough? Why do we need a new ProcessType::None enum value?

[Chris Dumez]

Comment on attachment 341289 [details]
Update to avoid `enumeration value 'None' not handled in switch` issues in Apple's code.

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

>> Source/WebKit/UIProcess/Plugins/PluginProcessProxy.cpp:92
>> +    ChildProcessProxy::getLaunchOptions(launchOptions);
> 
> Yes, we definitely need to swap the 2 instructions here. Why isn't this enough? Why do we need a new ProcessType::None enum value?

None is not a valid process Type and we should always have a valid process type I believe.

I think we should either:
- Not add new enum value and just swap the instructions here.
or (if we want to provide some validation that the processType gets set):
- Rename None to Unset (or Invalid) & ASSERT in ChildProcessProxy::getLaunchOptions() that processType is not Unset/Invalid.

[Thibault Saunier]

Created attachment 342812 [details]
Patch.

(In reply to Chris Dumez from comment #32)
> I think we should either:
> - Not add new enum value and just swap the instructions here.
> or (if we want to provide some validation that the processType gets set):
> - Rename None to Unset (or Invalid) & ASSERT in
> ChildProcessProxy::getLaunchOptions() that processType is not Unset/Invalid.

I decided to keep it simple and just swap the lines.

[Chris Dumez]

Comment on attachment 342812 [details]
Patch.

r=me

[WebKit Commit Bot]

Comment on attachment 342812 [details]
Patch.

Clearing flags on attachment: 342812

Committed r232872: <https://trac.webkit.org/changeset/232872>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41161091>