Bug 180607 - Web Inspector: get rid of remaining uses of OptOutput<T>
Summary: Web Inspector: get rid of remaining uses of OptOutput<T>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-08 14:24 PST by BJ Burg
Modified: 2018-02-19 01:04 PST (History)
12 users (show)

See Also:


Attachments
Patch (156.60 KB, patch)
2018-02-14 21:50 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (158.03 KB, patch)
2018-02-15 11:57 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (179.25 KB, patch)
2018-02-15 14:06 PST, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (182.38 KB, patch)
2018-02-15 15:20 PST, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-12-08 14:24:38 PST
Gross
Comment 1 Darin Adler 2018-02-14 21:50:47 PST
Created attachment 333874 [details]
Patch
Comment 2 EWS Watchlist 2018-02-14 21:51:31 PST
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)
Comment 3 Darin Adler 2018-02-15 11:57:40 PST
Created attachment 333928 [details]
Patch
Comment 4 BJ Burg 2018-02-15 13:26:59 PST
Comment on attachment 333928 [details]
Patch

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

r=me, great cleanup! Thanks!

> Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:36
> +template<typename> struct BindingTraits;

I didn't know you could do that.
Comment 5 Darin Adler 2018-02-15 14:06:58 PST
Created attachment 333941 [details]
Patch
Comment 6 Darin Adler 2018-02-15 15:20:05 PST
Created attachment 333957 [details]
Patch
Comment 7 Darin Adler 2018-02-15 18:14:33 PST
Committed r228546: <https://trac.webkit.org/changeset/228546>
Comment 8 Radar WebKit Bug Importer 2018-02-15 18:15:29 PST
<rdar://problem/37591967>
Comment 9 Claudio Saavedra 2018-02-16 05:13:28 PST
This change seems to have broken inspector/heap/getPreview.html, in WebKitGTK+ and Mac as well. Might be related to the change in signature of InspectorHeapAgent::getPreview(), but I am not sure. Please check it? Below is the failure diff. Seems that the string variable in the test is undefined.


--- /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/inspector/heap/getPreview-expected.txt
+++ /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/inspector/heap/getPreview-actual.txt
@@ -9,7 +9,7 @@
 -- Running test case: GetPreviewForString
 PASS: Should not have an error creating a snapshot.
 PASS: Should not have an error getting preview.
-STRING: This is the test string.
+STRING: undefined
 
 -- Running test case: GetPreviewForFunction
 PASS: Should not have an error creating a snapshot.
Comment 10 Matt Lewis 2018-02-16 10:42:18 PST
It looks like it has actually cause the same test to crash with an assertion failure on Debug macOS testers as well.

sderr:

LEAK: 6 CachedResource
LEAK: 25 WebCoreNode
Assertion failed: (initialized()), function operator*, file /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/Optional.h, line 595.
LEAK: 2 WebPageProxy


https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/r228557%20(2088)/results.html
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20(Tests)/builds/2088
Comment 11 Matt Lewis 2018-02-16 10:52:28 PST
Reverted r228546 for reason:

This caused a consistent crash on all macOS WK2 platforms.

Committed r228563: <https://trac.webkit.org/changeset/228563>
Comment 12 BJ Burg 2018-02-16 10:58:53 PST
(In reply to Claudio Saavedra from comment #9)
> This change seems to have broken inspector/heap/getPreview.html, in
> WebKitGTK+ and Mac as well. Might be related to the change in signature of
> InspectorHeapAgent::getPreview(), but I am not sure. Please check it? Below
> is the failure diff. Seems that the string variable in the test is undefined.
> 
> 
> ---
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/
> inspector/heap/getPreview-expected.txt
> +++
> /home/slave/webkitgtk/gtk-linux-64-release-tests/build/layout-test-results/
> inspector/heap/getPreview-actual.txt
> @@ -9,7 +9,7 @@
>  -- Running test case: GetPreviewForString
>  PASS: Should not have an error creating a snapshot.
>  PASS: Should not have an error getting preview.
> -STRING: This is the test string.
> +STRING: undefined
>  
>  -- Running test case: GetPreviewForFunction
>  PASS: Should not have an error creating a snapshot.

The code used to do *result = ..., but now it should not use operator*. It should just assign to result. So the crash is using operator* on an empty std::optional. (I'm not sure what the semantics of this, or whether it's allowed. Should just not use operator* here.)

As for the other ServiceWorker crashes, I have no idea what's going on and it looks like unrelated code.
Comment 13 Darin Adler 2018-02-17 11:10:21 PST
Committed r228600: <https://trac.webkit.org/changeset/228600>
Comment 14 Darin Adler 2018-02-17 11:10:39 PST
Re-landed with the offending "*" character removed.
Comment 15 Claudio Saavedra 2018-02-19 01:04:11 PST
Thanks, test passing again.