Bug 187669

Summary: WTF's internal std::optional implementation should release assert on all bad accesses
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: Web Template FrameworkAssignee: David Kilzer (:ddkilzer) <ddkilzer>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, commit-queue, dbates, ddkilzer, ews, fred.wang, mcatanzaro, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=186536
https://bugs.webkit.org/show_bug.cgi?id=185159
https://bugs.webkit.org/show_bug.cgi?id=187672
https://bugs.webkit.org/show_bug.cgi?id=187744
Attachments:
Description Flags
Patch v1 rniwa: review+, commit-queue: commit-queue-

Description David Kilzer (:ddkilzer) 2018-07-13 16:04:23 PDT
Bug 186536 changed some WTF std::optional methods to release assert when accessing invalid (garbage) values, but those methods that use the TR2_OPTIONAL_ASSERTED_EXPRESSION macro still only assert on Debug builds.

To match std::optional from the C++ standard library, we should release assert any time invalid (garbage) values are accessed.
Comment 1 Radar WebKit Bug Importer 2018-07-13 16:19:53 PDT
<rdar://problem/42185658>
Comment 2 David Kilzer (:ddkilzer) 2018-07-13 16:58:22 PDT
Oh, maybe this is a dupe of Bug 186536.  I thought a fix landed for that, but I see it was only Debug assertions.
Comment 3 Frédéric Wang (:fredw) 2018-07-13 23:38:01 PDT
This was initially done in https://bugs.webkit.org/show_bug.cgi?id=186536#c27 but some people think it should not assert in release mode so an alternative debug-only version was landed.
Comment 4 David Kilzer (:ddkilzer) 2018-07-14 10:23:51 PDT
Created attachment 345037 [details]
Patch v1
Comment 5 David Kilzer (:ddkilzer) 2018-07-14 10:24:29 PDT
(In reply to David Kilzer (:ddkilzer) from comment #4)
> Created attachment 345037 [details]
> Patch v1

Just posting a patch to do this; not going to review/land yet.
Comment 6 Ryosuke Niwa 2018-07-16 19:58:20 PDT
This caused a bunch of real crashes which broke a bunch of manual QA testing scenarios so all those cases need to be first fixed before this patch can be re-landed.

Also, someone should get this through Speedometer 2, Dromaeo, etc... on macOS and iOS to make sure there are no perf impact.
Comment 7 Michael Catanzaro 2018-07-17 08:40:24 PDT
(In reply to Ryosuke Niwa from comment #6)
> This caused a bunch of real crashes which broke a bunch of manual QA testing
> scenarios so all those cases need to be first fixed before this patch can be
> re-landed.

Then I insist on switching GTK/WPE back to C++ 14. JF, sorry, but I don't want GTK/WPE to be dogfooding these crashes until you're ready to switch the XCode build over as well. I don't think it makes any sense that we've been building different ports with different C++ standards versions for several months now. When you're ready to switch XCode over to C++ 17 (at which point you'll have no choice but to fix these crashes :) then we can do so for the CMake ports as well. Does that sound OK?
Comment 8 David Kilzer (:ddkilzer) 2018-07-17 17:41:31 PDT
(In reply to Michael Catanzaro from comment #7)
> (In reply to Ryosuke Niwa from comment #6)
> > This caused a bunch of real crashes which broke a bunch of manual QA testing
> > scenarios so all those cases need to be first fixed before this patch can be
> > re-landed.
> 
> Then I insist on switching GTK/WPE back to C++ 14. JF, sorry, but I don't
> want GTK/WPE to be dogfooding these crashes until you're ready to switch the
> XCode build over as well. I don't think it makes any sense that we've been
> building different ports with different C++ standards versions for several
> months now. When you're ready to switch XCode over to C++ 17 (at which point
> you'll have no choice but to fix these crashes :) then we can do so for the
> CMake ports as well. Does that sound OK?

I won’t land this until we’ve fixed known issues.
Comment 9 David Kilzer (:ddkilzer) 2018-07-21 10:07:32 PDT
(In reply to David Kilzer (:ddkilzer) from comment #8)
> I won’t land this until we’ve fixed known issues.

I've been living on a build of WebKit with this patch applied, and haven't run into any crashes.  I suspect we've fixed whatever (common) situations there were where unset values were being used.
Comment 10 Ryosuke Niwa 2018-07-31 19:38:27 PDT
Please make sure <rdar://problem/41860280> is fixed.

Also, please run all perf tests: PLT, Speedometer, MotionMark, etc... to make sure there is no perf regression from this change.
Comment 11 David Kilzer (:ddkilzer) 2018-08-10 20:25:41 PDT
(In reply to Ryosuke Niwa from comment #10)
> Please make sure <rdar://problem/41860280> is fixed.

This is fixed.  Verified by source code inspection, and steps to reproduce:

1. Launch Safari
2. Navigate to drive.google.com
3. Open sheets.
4. Refresh drive.google.com
5. Repeat steps 2-4.

> Also, please run all perf tests: PLT, Speedometer, MotionMark, etc... to
> make sure there is no perf regression from this change.

Running those now.
Comment 12 David Kilzer (:ddkilzer) 2018-08-16 16:05:18 PDT
(In reply to David Kilzer (:ddkilzer) from comment #11)
> (In reply to Ryosuke Niwa from comment #10)
> > Please make sure <rdar://problem/41860280> is fixed.
> 
> This is fixed.  Verified by source code inspection, and steps to reproduce:
> 
> 1. Launch Safari
> 2. Navigate to drive.google.com
> 3. Open sheets.
> 4. Refresh drive.google.com
> 5. Repeat steps 2-4.
> 
> > Also, please run all perf tests: PLT, Speedometer, MotionMark, etc... to
> > make sure there is no perf regression from this change.
> 
> Running those now.

Speedometer was statistically insignificant (0.13% worse for Time : Total; 0.07% worse for Score).

MotionMark was 1.13% better (statistically significant).

PLT was 0.31% better for arithmetic mean, and 0.32% better for geometric mean (both statistically significant).
Comment 13 WebKit Commit Bot 2018-08-16 17:48:58 PDT
Comment on attachment 345037 [details]
Patch v1

Rejecting attachment 345037 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 345037, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=345037&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=187669&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 345037 from bug 187669.
Fetching: https://bugs.webkit.org/attachment.cgi?id=345037
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Parsed 3 diffs from patch file(s).
patching file Source/WTF/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WTF/wtf/Assertions.h
Hunk #1 FAILED at 517.
1 out of 1 hunk FAILED -- saving rejects to file Source/WTF/wtf/Assertions.h.rej
patching file Source/WTF/wtf/Optional.h
Hunk #1 succeeded at 516 (offset -4 lines).
Hunk #2 succeeded at 661 (offset -4 lines).

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Ryosuke Niwa']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: https://webkit-queues.webkit.org/results/8886784
Comment 14 David Kilzer (:ddkilzer) 2018-08-17 10:36:12 PDT
Committed r234980: <https://trac.webkit.org/changeset/234980>