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;
}
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.
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
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
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
(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.
(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 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
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
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 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?
(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 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 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.
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
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
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.
(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.
(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.
(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.
(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?
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
(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.
(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.
(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
(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. ;)
(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.
(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.
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?
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.
(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
(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.
(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.
2018-06-11 15:58 PDT, Michael Catanzaro
2018-06-11 17:18 PDT, EWS Watchlist
2018-06-11 20:01 PDT, EWS Watchlist
2018-06-27 09:16 PDT, Michael Catanzaro
2018-06-27 15:50 PDT, EWS Watchlist
2018-06-28 09:36 PDT, Frédéric Wang (:fredw)
2018-06-28 09:40 PDT, Frédéric Wang (:fredw)
2018-06-28 13:12 PDT, EWS Watchlist
2018-06-29 02:04 PDT, Frédéric Wang (:fredw)
2018-06-29 23:03 PDT, Frédéric Wang (:fredw)
2018-06-30 01:27 PDT, Frédéric Wang (:fredw)
2018-06-30 02:58 PDT, EWS Watchlist
2018-06-30 03:32 PDT, EWS Watchlist
2018-07-02 00:17 PDT, Frédéric Wang (:fredw)
2018-07-06 02:30 PDT, Frédéric Wang (:fredw)
2018-07-06 04:37 PDT, EWS Watchlist