Bug 180607 – Web Inspector: get rid of remaining uses of OptOutput<T>

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

Gross

[Darin Adler]

Created attachment 333874 [details]
Patch

[EWS Watchlist]

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`)

[Darin Adler]

Created attachment 333928 [details]
Patch

[Blaze Burg]

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.

[Darin Adler]

Created attachment 333941 [details]
Patch

[Darin Adler]

Created attachment 333957 [details]
Patch

[Darin Adler]

Committed r228546: <https://trac.webkit.org/changeset/228546>

[Radar WebKit Bug Importer]

<rdar://problem/37591967>

[Claudio Saavedra]

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.

[Matt Lewis]

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

[Matt Lewis]

Reverted r228546 for reason:

This caused a consistent crash on all macOS WK2 platforms.

Committed r228563: <https://trac.webkit.org/changeset/228563>

[Blaze Burg]

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

[Darin Adler]

Committed r228600: <https://trac.webkit.org/changeset/228600>

[Darin Adler]

Re-landed with the offending "*" character removed.

[Claudio Saavedra]

Thanks, test passing again.