| Summary: | WTF's internal std::optional implementation should release assert on all bad accesses | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Component: | Web Template Framework | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, commit-queue, dbates, ddkilzer, ews-watchlist, 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
David Kilzer (:ddkilzer)
2018-07-13 16:04:23 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. 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. Created attachment 345037 [details]
Patch v1
(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. 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. (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? (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. (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. 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. (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. (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 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 Committed r234980: <https://trac.webkit.org/changeset/234980> |