Bug 189585 – [GLib] Fix format string in KeyedEncoderGlib::beginObject().

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Ms2ger (he/him; ⌚ UTC+1/+2)]

We are hitting the following assertion during layout tests:

GLib-CRITICAL **: g_variant_builder_add_value: assertion '!GVSB(builder)->expected_type || g_variant_is_of_type (value, GVSB(builder)->expected_type)' failed

with the stack

g_variant_builder_add () at /home/slave/webkitgtk/gtk-linux-64-debug-tests/build/WebKitBuild/DependenciesGTK/Source/glib-2.54.2/glib/gvariant.c:5531
(anonymous namespace)::KeyedEncoderGlib::encodeBool (this=0x7fdcd4001aa0, key="wasDataRead", value=false) at ../../Source/WebCore/platform/glib/KeyedEncoderGlib.cpp:61
(anonymous namespace)::<lambda(WebCore::KeyedEncoder&, const WebCore::CanvasActivityRecord&)>::operator()((anonymous namespace)::KeyedEncoder &, const (anonymous namespace)::CanvasActivityRecord &) const (__closure=0x7fdcf7ffe10f, encoderInner=..., canvasActivityRecord=...) at ../../Source/WebCore/loader/ResourceLoadStatistics.cpp:86
(anonymous namespace)::KeyedEncoder::encodeObject<WebCore::CanvasActivityRecord, WebCore::encodeCanvasActivityRecord(WebCore::KeyedEncoder&, const WTF::String&, const WebCore::CanvasActivityRecord&)::<lambda(WebCore::KeyedEncoder&, const WebCore::CanvasActivityRecord&)> >(const WTF::String &, const (anonymous namespace)::CanvasActivityRecord &, <unknown type in /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37, CU 0x0, DIE 0x28e67f>) (this=0x7fdcd4001aa0, key="canvasActivityRecord", object=..., function=<unknown type in /home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37, CU 0x0, DIE 0x28e67f>) at ../../Source/WebCore/platform/KeyedCoding.h:175
(anonymous namespace)::encodeCanvasActivityRecord (encoder=..., label="canvasActivityRecord", canvasActivityRecord=...) at ../../Source/WebCore/loader/ResourceLoadStatistics.cpp:90

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Created attachment 349664 [details]
Patch

[Michael Catanzaro]

Comment on attachment 349664 [details]
Patch

I had to stare at this for a while to convince myself it should be different from KeyedEncoderGlib::beginArray, but I think the change is correct.

But surely this changes the database serialization format. How do we fix existing databases that are already broken? Will the critical continue to occur forever for existing broken databases? That requires further investigation. Hopefully we have some mechanism somewhere to handle incompatible changes like this? I'd like Carlos to review this, please.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

This method is not used in the IDB case, only for ResourceLoadStatistics, as I understand it. I don't think that serializes to disk? (This is why it's only come up since r235897.)

[Michael Catanzaro]

Comment on attachment 349664 [details]
Patch

OK then

It *does* serialize to disk, but we have not yet enabled resource load statistics for GTK or WPE (we need to) so doesn't matter.

[Michael Catanzaro]

Of course, there's another problem here: this code should not be running at all, because resource load statistics should never be enabled.

[Michael Catanzaro]

BTW this was added for database process, years before resource load statistics existed, so I'm a bit surprised it's only used for resource load statistics nowadays.

[WebKit Commit Bot]

Comment on attachment 349664 [details]
Patch

Clearing flags on attachment: 349664

Committed r235982: <https://trac.webkit.org/changeset/235982>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.