Bug 187669 – WTF's internal std::optional implementation should release assert on all bad accesses

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

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.

[Radar WebKit Bug Importer]

<rdar://problem/42185658>

[David Kilzer (:ddkilzer)]

Oh, maybe this is a dupe of Bug 186536.  I thought a fix landed for that, but I see it was only Debug assertions.

[Frédéric Wang (:fredw)]

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.

[David Kilzer (:ddkilzer)]

Created attachment 345037 [details]
Patch v1

[David Kilzer (:ddkilzer)]

(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.

[Ryosuke Niwa]

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.

[Michael Catanzaro]

(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?

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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.

[Ryosuke Niwa]

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.

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

(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).

[WebKit Commit Bot]

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 [e-mail]...
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

[David Kilzer (:ddkilzer)]

Committed r234980: <https://trac.webkit.org/changeset/234980>