Bug 186536 - WTF's internal std::optional implementation should abort() on bad optional access
Summary: WTF's internal std::optional implementation should abort() on bad optional ac...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on: 187217 187364
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-11 12:38 PDT by Michael Catanzaro
Modified: 2018-07-13 23:35 PDT (History)
17 users (show)

See Also:


Attachments
Patch (3.15 KB, patch)
2018-06-11 15:58 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-sierra (1.03 MB, application/zip)
2018-06-11 17:18 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.77 MB, application/zip)
2018-06-11 20:01 PDT, EWS Watchlist
no flags Details
Patch (3.15 KB, patch)
2018-06-27 09:16 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (3.44 MB, application/zip)
2018-06-27 15:50 PDT, EWS Watchlist
no flags Details
Patch (defines a new ASSERT macro without asm statement) (4.28 KB, patch)
2018-06-28 09:36 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (defines a new ASSERT macro without asm statement) (4.29 KB, patch)
2018-06-28 09:40 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews114 for mac-sierra (3.41 MB, application/zip)
2018-06-28 13:12 PDT, EWS Watchlist
no flags Details
Patch (9.05 KB, patch)
2018-06-29 02:04 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch for landing (7.40 KB, patch)
2018-06-29 23:03 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (7.71 KB, patch)
2018-06-30 01:27 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-sierra-wk2 (2.79 MB, application/zip)
2018-06-30 02:58 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.28 MB, application/zip)
2018-06-30 03:32 PDT, EWS Watchlist
no flags Details
Patch for landing (7.78 KB, patch)
2018-07-02 00:17 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (6.33 KB, patch)
2018-07-06 02:30 PDT, Frédéric Wang (:fredw)
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (13.96 MB, application/zip)
2018-07-06 04:37 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-06-11 12:38:37 PDT
Currently GTK/WPE when built with GCC 7 and higher are suffering from issues like bug #186535 and bug #186189. It's impossible for Apple developers to notice the problems because Apple is still using WTF's internal std::optional, which allows accessing the value of std::optional when it is unset (std::nullopt), which is illegal and causes the program to abort using the standard std::optional.

i.e., we need to fix these FIXMEs in wtf/Optional.h:


  OPTIONAL_MUTABLE_CONSTEXPR T& operator *() & {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // CONSTEXPR_ASSERT(initialized());
    return contained_val();
  }

  OPTIONAL_MUTABLE_CONSTEXPR T&& operator *() && {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // CONSTEXPR_ASSERT(initialized());
    return detail_::constexpr_move(contained_val());
  }

  constexpr T const& value() const& {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
    return contained_val();
  }

  OPTIONAL_MUTABLE_CONSTEXPR T& value() & {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
    return contained_val();
  }

  OPTIONAL_MUTABLE_CONSTEXPR T&& value() && {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // if (!initialized()) __THROW_EXCEPTION(bad_optional_access("bad optional access"));
    return std::move(contained_val());
  }

  constexpr T& value() const {
    // FIXME: We need to offer special assert function that can be used under the contexpr context.
    // return ref ? *ref : (throw bad_optional_access("bad optional access"), *ref);
    return *ref;
  }
Comment 1 Michael Catanzaro 2018-06-11 15:57:10 PDT
I'm going to attach a speculative patch, which will probably neither build nor work. I can try setting up a build environment to test it in a VM another day.
Comment 2 Michael Catanzaro 2018-06-11 15:58:10 PDT
Created attachment 342475 [details]
Patch
Comment 3 EWS Watchlist 2018-06-11 17:18:16 PDT
Comment on attachment 342475 [details]
Patch

Attachment 342475 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/8137841

Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2018-06-11 17:18:17 PDT
Created attachment 342491 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Michael Catanzaro 2018-06-11 18:21:33 PDT
Looks like we'll definitely need to fix bug #186189 first. No doubt there will be more issues.
Comment 6 EWS Watchlist 2018-06-11 20:00:54 PDT
Comment on attachment 342475 [details]
Patch

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

New failing tests:
http/tests/security/contentSecurityPolicy/script-src-redirect.html
http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html
Comment 7 EWS Watchlist 2018-06-11 20:01:05 PDT
Created attachment 342505 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Michael Catanzaro 2018-06-27 09:16:25 PDT
Created attachment 343716 [details]
Patch

Resubmitting the same patch for another EWS run
Comment 9 EWS Watchlist 2018-06-27 15:50:14 PDT
Comment on attachment 343716 [details]
Patch

Attachment 343716 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8362922

New failing tests:
imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html
fast/css-grid-layout/flex-sizing-rows-min-max-height.html
fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html
fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
Comment 10 EWS Watchlist 2018-06-27 15:50:16 PDT
Created attachment 343763 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 11 Frédéric Wang (:fredw) 2018-06-28 09:36:11 PDT
Created attachment 343812 [details]
Patch (defines a new ASSERT macro without asm statement)
Comment 12 Frédéric Wang (:fredw) 2018-06-28 09:40:21 PDT
Created attachment 343813 [details]
Patch (defines a new ASSERT macro without asm statement)
Comment 13 Michael Catanzaro 2018-06-28 09:40:32 PDT
Comment on attachment 343812 [details]
Patch (defines a new ASSERT macro without asm statement)

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

> Source/WTF/wtf/Assertions.h:350
> +        __builtin_trap(); \
> +        __builtin_unreachable(); \

This won't work for MSVCC, though.
Comment 14 Frédéric Wang (:fredw) 2018-06-28 09:50:18 PDT
(In reply to Michael Catanzaro from comment #13)
> Comment on attachment 343812 [details]
> Patch (defines a new ASSERT macro without asm statement)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343812&action=review
> 
> > Source/WTF/wtf/Assertions.h:350
> > +        __builtin_trap(); \
> > +        __builtin_unreachable(); \
> 
> This won't work for MSVCC, though.

Good point, I think we can use __debugbreak() instead. Let's see if that version builds on other platforms. I think we will still need to figure out what to do with the 4 crashes you got in our initial patch.
Comment 15 Michael Catanzaro 2018-06-28 10:46:24 PDT
(In reply to Frédéric Wang (:fredw) from comment #14)
> Good point, I think we can use __debugbreak() instead. Let's see if that
> version builds on other platforms. I think we will still need to figure out
> what to do with the 4 crashes you got in our initial patch.

Let's just add Crash expectations for them, in the global TestExpectations file, pointing to other bugs that are not closed.

We already have bug #186752 for the CSS grid crashes. We can simply leave a comment there that the expectations should be removed when the crash is fixed.

That leaves only imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html. I've reported bug #187145 for it now.
Comment 16 EWS Watchlist 2018-06-28 13:12:08 PDT
Comment on attachment 343813 [details]
Patch (defines a new ASSERT macro without asm statement)

Attachment 343813 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8372067

New failing tests:
imported/w3c/web-platform-tests/web-animations/interfaces/Animatable/animate-no-browsing-context.html
fast/css-grid-layout/flex-sizing-rows-min-max-height.html
fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html
fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html
Comment 17 EWS Watchlist 2018-06-28 13:12:10 PDT
Created attachment 343840 [details]
Archive of layout-test-results from ews114 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 18 Frédéric Wang (:fredw) 2018-06-29 01:15:29 PDT
The crash with http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html seems flacky on Windows. From a quick bugzilla search, it also happened in older bugs.
Comment 19 Frédéric Wang (:fredw) 2018-06-29 02:04:41 PDT
Created attachment 343898 [details]
Patch
Comment 20 Frédéric Wang (:fredw) 2018-06-29 02:12:39 PDT
Comment on attachment 343898 [details]
Patch

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

> Source/WTF/ChangeLog:17
> +        WTF's implementation of std::optional.

Like other ASSERT_* macros, it's not used in release mode. Should we instead introduce a RELEASE_ASSERT_*?

> Source/WTF/ChangeLog:22
> +        (std::optional::operator ->): Add an assert to ensure the optional value is available.

It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes
What do we prefer between making implementation consistent with C++17 and making check stricter?
Comment 21 Michael Catanzaro 2018-06-29 07:46:13 PDT
(In reply to Frédéric Wang (:fredw) from comment #20)
> Comment on attachment 343898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343898&action=review
> 
> > Source/WTF/ChangeLog:17
> > +        WTF's implementation of std::optional.
> 
> Like other ASSERT_* macros, it's not used in release mode. Should we instead
> introduce a RELEASE_ASSERT_*?

Yes, this should be a RELEASE_ASSERT() because the goal is for our std::optional to match the behavior of the real std::optional.

> > Source/WTF/ChangeLog:22
> > +        (std::optional::operator ->): Add an assert to ensure the optional value is available.
> 
> It seems the bad access check is actually not done for operator-> and
> operator* according to
> https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes
> What do we prefer between making implementation consistent with C++17 and
> making check stricter?

Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library.
Comment 22 Michael Catanzaro 2018-06-29 07:54:52 PDT
Comment on attachment 343898 [details]
Patch

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

> LayoutTests/TestExpectations:394
> +webkit.org/b/186752 fast/css-grid-layout/flex-sizing-rows-min-max-height.html [ Pass Crash ]
> +webkit.org/b/186752 fast/css-grid-layout/grid-indefinite-size-auto-repeat-crash.html [ Pass Crash ]
> +webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Pass Crash ]

These tests are fixed in bug #186752, so remember to remove them from your patch.
Comment 23 Frédéric Wang (:fredw) 2018-06-29 07:58:14 PDT
Comment on attachment 343898 [details]
Patch

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

>>> Source/WTF/ChangeLog:17
>>> +        WTF's implementation of std::optional.
>> 
>> Like other ASSERT_* macros, it's not used in release mode. Should we instead introduce a RELEASE_ASSERT_*?
> 
> Yes, this should be a RELEASE_ASSERT() because the goal is for our std::optional to match the behavior of the real std::optional.

OK. I fear this is going to affect more people, but maybe not a bad thing...

>>> Source/WTF/ChangeLog:22
>>> +        (std::optional::operator ->): Add an assert to ensure the optional value is available.
>> 
>> It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes
>> What do we prefer between making implementation consistent with C++17 and making check stricter?
> 
> Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library.

OK, let's remove the assert for these cases.
Comment 24 Frédéric Wang (:fredw) 2018-06-29 08:08:49 PDT
Comment on attachment 343898 [details]
Patch

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

>> LayoutTests/TestExpectations:394
>> +webkit.org/b/186752 fast/css-grid-layout/maximize-tracks-definite-indefinite-height.html [ Pass Crash ]
> 
> These tests are fixed in bug #186752, so remember to remove them from your patch.

Yes, I'm waiting for rego to land it (and maybe Antoine will fix webkit.org/b/187145 soon too).
Comment 25 Frédéric Wang (:fredw) 2018-06-29 22:42:20 PDT
Comment on attachment 343898 [details]
Patch

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

>>>> Source/WTF/ChangeLog:22
>>>> +        (std::optional::operator ->): Add an assert to ensure the optional value is available.
>>> 
>>> It seems the bad access check is actually not done for operator-> and operator* according to https://en.cppreference.com/w/cpp/utility/optional/operator*#Notes
>>> What do we prefer between making implementation consistent with C++17 and making check stricter?
>> 
>> Good catch. Sigh, it says the behavior is undefined in this case. :S We could go either way. We are allowed to keep the assert, because it says undefined, but I checked libstdc++ and it does not do so... probably best to match the behavior of the standard library.
> 
> OK, let's remove the assert for these cases.

On further thoughts, I see that we already assert in some cases and so does https://github.com/akrzemi1/Optional/blob/master/optional.hpp where the code was taken from. So let's keep the assert for consistency. If we want to match the behavior of the standard library, maybe we should do that in a follow-up bug.
Comment 26 Frédéric Wang (:fredw) 2018-06-29 23:03:27 PDT
Created attachment 344000 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2018-06-29 23:42:11 PDT
Comment on attachment 344000 [details]
Patch for landing

Clearing flags on attachment: 344000

Committed r233391: <https://trac.webkit.org/changeset/233391>
Comment 28 WebKit Commit Bot 2018-06-29 23:42:13 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Radar WebKit Bug Importer 2018-06-29 23:51:44 PDT
<rdar://problem/41669560>
Comment 30 WebKit Commit Bot 2018-06-30 00:06:11 PDT
Re-opened since this is blocked by bug 187217
Comment 31 Frédéric Wang (:fredw) 2018-06-30 01:27:54 PDT
Created attachment 344007 [details]
Patch
Comment 32 EWS Watchlist 2018-06-30 02:58:33 PDT
Comment on attachment 344007 [details]
Patch

Attachment 344007 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8393859

New failing tests:
http/tests/cache-storage/cache-persistency.https.html
Comment 33 EWS Watchlist 2018-06-30 02:58:35 PDT
Created attachment 344009 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 34 EWS Watchlist 2018-06-30 03:32:34 PDT
Comment on attachment 344007 [details]
Patch

Attachment 344007 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8393899

New failing tests:
http/tests/cache-storage/cache-persistency.https.html
Comment 35 EWS Watchlist 2018-06-30 03:32:36 PDT
Created attachment 344010 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 36 Frédéric Wang (:fredw) 2018-07-02 00:17:18 PDT
Created attachment 344081 [details]
Patch for landing
Comment 37 WebKit Commit Bot 2018-07-02 00:56:53 PDT
Comment on attachment 344081 [details]
Patch for landing

Clearing flags on attachment: 344081

Committed r233417: <https://trac.webkit.org/changeset/233417>
Comment 38 WebKit Commit Bot 2018-07-02 00:56:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 39 Frédéric Wang (:fredw) 2018-07-02 03:13:05 PDT
Committed r233418: <https://trac.webkit.org/changeset/233418>
Comment 40 WebKit Commit Bot 2018-07-05 14:42:30 PDT
Re-opened since this is blocked by bug 187364
Comment 41 Ryosuke Niwa 2018-07-05 14:44:19 PDT
This patch introduced an occasional crash on Google Drive.
When you navigate to drive.google.com with Safari, open sheets, and refresh drive.google.com 5-6 times, WebContent processes crash with the following trace.

0   com.apple.WebCore             	0x00007fff53c378dd WebCore::AnimationBase::updateStateMachine(WebCore::AnimationBase::AnimationStateInput, double) + 3261
1   com.apple.WebCore             	0x00007fff52c2cb3c WebCore::CompositeAnimation::resumeAnimations() + 460
2   com.apple.WebCore             	0x00007fff53c3a1ae WebCore::CSSAnimationControllerPrivate::resumeAnimationsForDocument(WebCore::Document*) + 398
3   com.apple.WebCore             	0x00007fff53c39fda WebCore::CSSAnimationControllerPrivate::resumeAnimations() + 42
4   com.apple.WebCore             	0x00007fff53c05ad0 WebCore::Page::setIsVisibleInternal(bool) + 432
5   com.apple.WebCore             	0x00007fff53c04835 WebCore::Page::setActivityState(unsigned int) + 85
6   com.apple.WebKit              	0x00007fff547f6f55 WebKit::WebPage::setActivityState(unsigned int, bool, WTF::Vector<WebKit::CallbackID, 0ul, WTF::CrashOnOverflow, 16ul> const&) + 83
7   com.apple.WebKit              	0x00007fff54813d95 WebKit::WebPage::didReceiveWebPageMessage(IPC::Connection&, IPC::Decoder&) + 4601
8   com.apple.WebKit              	0x00007fff546275ff IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) + 127
9   com.apple.WebKit              	0x00007fff54881f6e WebKit::WebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&) + 28
10  com.apple.WebKit              	0x00007fff545f488d IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 119
11  com.apple.WebKit              	0x00007fff545f79d7 IPC::Connection::dispatchOneIncomingMessage() + 177
12  com.apple.JavaScriptCore      	0x00007fff48bb8db9 WTF::RunLoop::performWork() + 313
13  com.apple.JavaScriptCore      	0x00007fff48bb8ff2 WTF::RunLoop::performWork(void*) + 34
14  com.apple.CoreFoundation      	0x7fff45548fcb __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:1980)
15  com.apple.CoreFoundation      	0x7fff455e543f __CFRunLoopDoSource0 + 108 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2015)
16  com.apple.CoreFoundation      	0x7fff4553067f __CFRunLoopDoSources0 + 283 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2059)
17  com.apple.CoreFoundation      	0x7fff4552fbcc __CFRunLoopRun + 1219 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:2922)
18  com.apple.CoreFoundation      	0x7fff4552f4f1 CFRunLoopRunSpecific + 463 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/CoreFoundation/Foundation-1543.10/CoreFoundation/RunLoop.subproj/CFRunLoop.c:3247)
19  com.apple.Foundation          	0x7fff47924e85 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 280 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1543.10/Foundation/Soil.subproj/NSRunLoop.m:367)
20  com.apple.Foundation          	0x7fff47924d5a -[NSRunLoop(NSRunLoop) run] + 76 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/Foundation/Foundation-1543.10/Foundation/Soil.subproj/NSRunLoop.m:389)
21  libxpc.dylib                  	0x7fff72a53f8a _xpc_objc_main + 567 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1336.200.83/src/main.m:170)
22  libxpc.dylib                  	0x7fff72a52bde xpc_main + 443 (/BuildRoot/Library/Caches/com.apple.xbs/Sources/libxpc/libxpc-1336.200.83/src/init.c:1471)
23  com.apple.WebKit.WebContent   	0x00000001022a763f 0x1022a6000 + 5695
24  libdyld.dylib                 	0x00007fff7280a36d start + 1
Comment 42 Ryosuke Niwa 2018-07-05 14:45:09 PDT
Also, adding a release assertion has a significant cost in terms of the binary size as well as runtime branch check. I don't think we should be adding these checks everywhere in release code in optional.h like this patch is doing.
Comment 43 Frédéric Wang (:fredw) 2018-07-05 16:12:32 PDT
(In reply to Ryosuke Niwa from comment #42)
> Also, adding a release assertion has a significant cost in terms of the
> binary size as well as runtime branch check. I don't think we should be
> adding these checks everywhere in release code in optional.h like this patch
> is doing.

OK, that's what I was wondering in comment 20. I guess a debug release like in the initial patch will be enough...

(In reply to Ryosuke Niwa from comment #41)
> This patch introduced an occasional crash on Google Drive.
> When you navigate to drive.google.com with Safari, open sheets, and refresh
> drive.google.com 5-6 times, WebContent processes crash with the following
> trace.
> 

I'll take a look. The purpose of this patch is to verify illegal use of std::optional so this is probably an actual bug in the WebAnimation code that should be handled separately.
Comment 44 Ryosuke Niwa 2018-07-05 20:17:18 PDT
(In reply to Frédéric Wang (:fredw) from comment #43)
> (In reply to Ryosuke Niwa from comment #42)
> > Also, adding a release assertion has a significant cost in terms of the
> > binary size as well as runtime branch check. I don't think we should be
> > adding these checks everywhere in release code in optional.h like this patch
> > is doing.
> 
> OK, that's what I was wondering in comment 20. I guess a debug release like
> in the initial patch will be enough...

You mean debug assert? Yes, that would be the right thing to do here.

> (In reply to Ryosuke Niwa from comment #41)
> > This patch introduced an occasional crash on Google Drive.
> > When you navigate to drive.google.com with Safari, open sheets, and refresh
> > drive.google.com 5-6 times, WebContent processes crash with the following
> > trace.
> > 
> 
> I'll take a look. The purpose of this patch is to verify illegal use of
> std::optional so this is probably an actual bug in the WebAnimation code
> that should be handled separately.

Indeed.
Comment 45 Antoine Quint 2018-07-05 21:38:28 PDT
(In reply to Ryosuke Niwa from comment #44)
> (In reply to Frédéric Wang (:fredw) from comment #43)
> > (In reply to Ryosuke Niwa from comment #41)
> > > This patch introduced an occasional crash on Google Drive.
> > > When you navigate to drive.google.com with Safari, open sheets, and refresh
> > > drive.google.com 5-6 times, WebContent processes crash with the following
> > > trace.
> > > 
> > 
> > I'll take a look. The purpose of this patch is to verify illegal use of
> > std::optional so this is probably an actual bug in the WebAnimation code
> > that should be handled separately.
> 
> Indeed.

This is the old animation code, not the Web Animations code. It’s going to be removed soon.
Comment 46 Frédéric Wang (:fredw) 2018-07-05 23:50:09 PDT
(In reply to Ryosuke Niwa from comment #44)
> You mean debug assert? Yes, that would be the right thing to do here.

I'll work on a new patch.

(In reply to Antoine Quint from comment #45)
> This is the old animation code, not the Web Animations code. It’s going to
> be removed soon.

Ah, my bad indeed it's AnimationBase. Do you mean it's not worth patching that code in the short term?
Comment 47 Frédéric Wang (:fredw) 2018-07-06 02:30:16 PDT
Created attachment 344412 [details]
Patch
Comment 48 EWS Watchlist 2018-07-06 04:37:19 PDT
Comment on attachment 344412 [details]
Patch

Attachment 344412 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8454879

New failing tests:
animations/needs-layout.html
Comment 49 EWS Watchlist 2018-07-06 04:37:22 PDT
Created attachment 344420 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 50 Frédéric Wang (:fredw) 2018-07-06 04:46:37 PDT
(In reply to Build Bot from comment #48)
> New failing tests:
> animations/needs-layout.html

This flacky failure is bug 185659. The Windows bot win-ews is in release mode so the tests failures are unrelated.
Comment 51 Frédéric Wang (:fredw) 2018-07-06 04:49:48 PDT
Committed r233577: <https://trac.webkit.org/changeset/233577>
Comment 52 Frédéric Wang (:fredw) 2018-07-06 04:52:50 PDT
(In reply to Frédéric Wang (:fredw) from comment #51)
> Committed r233577: <https://trac.webkit.org/changeset/233577>

For future reference: If you detect new crashes after r233577 then it's likely to be an error in the code (illegal use of uninitialized std::optional value). Please fix them in separate bugs rather than reverting this commit. Thank you.
Comment 53 Saam Barati 2018-07-06 09:39:52 PDT
(In reply to Frédéric Wang (:fredw) from comment #52)
> (In reply to Frédéric Wang (:fredw) from comment #51)
> > Committed r233577: <https://trac.webkit.org/changeset/233577>
> 
> For future reference: If you detect new crashes after r233577 then it's
> likely to be an error in the code (illegal use of uninitialized
> std::optional value). Please fix them in separate bugs rather than reverting
> this commit. Thank you.

Agreed
Comment 54 Michael Catanzaro 2018-07-09 05:46:43 PDT
(In reply to Ryosuke Niwa from comment #42)
> Also, adding a release assertion has a significant cost in terms of the
> binary size as well as runtime branch check. I don't think we should be
> adding these checks everywhere in release code in optional.h like this patch
> is doing.

The spec requires it to throw std::bad_optional_access. Since we compile with -fno-exceptions, that will be replaced with a call to std::abort(). A RELEASE_ASSERT() seems like a good equivalent to me.

We do need to have the same behavior for all ports here. This std::optional behavior difference has been so exceptionally problematic that IMO we *really* need to switch back to C++ 14 if you're not OK with a release assert.

CC: JF, please help us with this remaining fallout from the switch to C++ 17. ;)
Comment 55 Michael Catanzaro 2018-07-09 05:47:00 PDT
Reopening.
Comment 56 Michael Catanzaro 2018-07-09 05:49:55 PDT
(In reply to Ryosuke Niwa from comment #42)
> Also, adding a release assertion has a significant cost in terms of the
> binary size as well as runtime branch check. I don't think we should be
> adding these checks everywhere in release code in optional.h like this patch
> is doing.

Please keep in mind that we don't control the C++ standard... you're going to get the runtime branch check no matter what when you enable C++ 17.
Comment 57 Saam Barati 2018-07-09 10:22:40 PDT
(In reply to Michael Catanzaro from comment #56)
> (In reply to Ryosuke Niwa from comment #42)
> > Also, adding a release assertion has a significant cost in terms of the
> > binary size as well as runtime branch check. I don't think we should be
> > adding these checks everywhere in release code in optional.h like this patch
> > is doing.
> 
> Please keep in mind that we don't control the C++ standard... you're going
> to get the runtime branch check no matter what when you enable C++ 17.

We could always deviate away from using optional from the stdlib if we think this runtime check is prohibitive.

However, I agree that we should add a RELEASE_ASSERT to align with C++17 behavior if we decide we want to use the stdlib's optional.
Comment 58 Frédéric Wang (:fredw) 2018-07-09 10:54:05 PDT
Personally, I think the main issue here was that we were not catching illegal use of uninitalized std::optional (and in particular it was crashing on some platforms). So a debug assert seems good enough, assuming some developers & bots run debug builds. Deviating from the stdlib behavior (or using * or -> which don't assert) would avoid the crashes but also prevent us from detecting the bad usage, so I'm not sure it's a good idea :-)

I don't have strong preference for what to do with release builds, but maybe this could be handled/discussed in a new separate bug?
Comment 59 JF Bastien 2018-07-09 13:07:53 PDT
Accessing the "wrong" optional field with * and -> is UB, so we can do whatever we want, including navigating to combo.com if we're so inclined. We're not deviating from the standard in doing so.

I believe Darin was surprised a while ago when Dan made a change around this. Can't find the patch now, but it was either with optional or expected (which follow the same pattern). We had discussed, IIRC, making the UB always trap. Realistically if there's a cost that the compiler can't eliminate then it's either a compiler bug (please file a bug), or it's hard enough to prove that your code likely has or will have a bug in the future. In this case think it's an invalid argument to worry about code bloat without proof.
Comment 60 Saam Barati 2018-07-09 13:36:24 PDT
(In reply to JF Bastien from comment #59)
> Accessing the "wrong" optional field with * and -> is UB, so we can do
> whatever we want, including navigating to combo.com if we're so inclined.
> We're not deviating from the standard in doing so.
> 
> I believe Darin was surprised a while ago when Dan made a change around
> this. Can't find the patch now, but it was either with optional or expected
> (which follow the same pattern). We had discussed, IIRC, making the UB
> always trap. Realistically if there's a cost that the compiler can't
> eliminate then it's either a compiler bug (please file a bug), or it's hard
> enough to prove that your code likely has or will have a bug in the future.
> In this case think it's an invalid argument to worry about code bloat
> without proof.

Almost all optional code I’ve read already branches around the optional being set or not. I’d be surprised if the compiler didn’t eliminate the redundant check as long as things are properly inlined
Comment 61 JF Bastien 2018-07-09 13:37:48 PDT
(In reply to Saam Barati from comment #60)
> (In reply to JF Bastien from comment #59)
> > Accessing the "wrong" optional field with * and -> is UB, so we can do
> > whatever we want, including navigating to combo.com if we're so inclined.
> > We're not deviating from the standard in doing so.
> > 
> > I believe Darin was surprised a while ago when Dan made a change around
> > this. Can't find the patch now, but it was either with optional or expected
> > (which follow the same pattern). We had discussed, IIRC, making the UB
> > always trap. Realistically if there's a cost that the compiler can't
> > eliminate then it's either a compiler bug (please file a bug), or it's hard
> > enough to prove that your code likely has or will have a bug in the future.
> > In this case think it's an invalid argument to worry about code bloat
> > without proof.
> 
> Almost all optional code I’ve read already branches around the optional
> being set or not. I’d be surprised if the compiler didn’t eliminate the
> redundant check as long as things are properly inlined

Exactly. If the compiler can't eliminate the extra check then file a bug on the compiler.

That being said, maybe don't call abort() and use CRASH() instead, so that in release builds it's a single instruction.
Comment 62 Ryosuke Niwa 2018-07-10 14:41:25 PDT
I still don't think we should be adding a release assertion.
Comment 63 Frédéric Wang (:fredw) 2018-07-13 23:35:35 PDT
(In reply to Frédéric Wang (:fredw) from comment #58)
> I don't have strong preference for what to do with release builds, but maybe
> this could be handled/discussed in a new separate bug?

Let's move discussion for release in bug 187669.