Summary: | Web Inspector: get rid of remaining uses of OptOutput<T> | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||
Component: | Web Inspector | Assignee: | Darin Adler <darin> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bburg, csaavedra, darin, ews-watchlist, inspector-bugzilla-changes, jlewis3, joepeck, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
BJ Burg
2017-12-08 14:24:38 PST
Created attachment 333874 [details]
Patch
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`) Created attachment 333928 [details]
Patch
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. Created attachment 333941 [details]
Patch
Created attachment 333957 [details]
Patch
Committed r228546: <https://trac.webkit.org/changeset/228546> 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. 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 Reverted r228546 for reason: This caused a consistent crash on all macOS WK2 platforms. Committed r228563: <https://trac.webkit.org/changeset/228563> (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. Committed r228600: <https://trac.webkit.org/changeset/228600> Re-landed with the offending "*" character removed. Thanks, test passing again. |