Bug 248232

Summary: REGRESSION(256488@main): Gibberish text visible in blank placeholder tabs before first "real" page load
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKit Misc.Assignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alicem, bugs-noreply, mcatanzaro, pascoe
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Screenshot
none
Screenshot none

Michael Catanzaro
Reported 2022-11-22 08:16:29 PST
Created attachment 463672 [details] Screenshot When Epiphany starts, it loads completely blank HTML in each web view, then later replaces it with the saved WebKitWebViewSessionState when you click on the tab for the first time. This worked well until 2.39.1. Now, I briefly see gibberish in the tab when I click on it for the first time, which is when it's supposed to be displaying blank content. Sometimes this includes the title of the tab. I'll attach two screenshots.
Attachments
Screenshot (37.12 KB, image/png)
2022-11-22 08:16 PST, Michael Catanzaro 		no flags
Details
Screenshot (42.80 KB, image/png)
2022-11-22 08:16 PST, Michael Catanzaro 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Michael Catanzaro
Comment 1 2022-11-22 08:16:47 PST
Created attachment 463673 [details] Screenshot
Michael Catanzaro
Comment 2 2022-11-22 13:52:41 PST
First, I thought perhaps the session state serialization changed by mistake in 256310@main. But Epiphany doesn't actually use any of the WebKitWebViewSessionState to construct the placeholder tab, so that doesn't make sense. ephy_web_view_set_placeholder() sets the placeholder tab contents to "<head><title>%s</title></head>" where %s is the title of the page as saved in Epiphany's session state, so it's not a *completely* blank page. Next, I thought perhaps the %s here is bogus. But it's not: it's fine. The bug occurs with, for example, "<head><title>Example Domain</title></head>". I don't know where the garbage is coming from, or why WebKit is rendering it, other than that it seems to be related to the page title....
Michael Catanzaro
Comment 3 2022-11-22 14:01:41 PST
Oooh, valgrind found it right away. Good job, valgrind! WebKit is missing a strdup() somewhere; it's using the original html string passed in by Epiphany after a run loop iteration without duplicating it, but WebKit doesn't own that string and Epiphany has correctly already freed it. Not sure where exactly the bug is yet. ==384967== Invalid read of size 8 ==384967== at 0x484C35D: memmove (vg_replace_strmem.c:1398) ==384967== by 0x6B838E8: encode<IPC::Encoder> (ArgumentCoders.h:77) ==384967== by 0x6B838E8: operator<< <const WTF::Span<unsigned char const>&> (Encoder.h:72) ==384967== by 0x6B838E8: WebKit::LoadParameters::encode(IPC::Encoder&) const (LoadParameters.cpp:46) ==384967== by 0x6C62639: encode<IPC::Encoder> (ArgumentCoder.h:52) ==384967== by 0x6C62639: operator<< <const WebKit::LoadParameters&> (Encoder.h:72) ==384967== by 0x6C62639: encode<IPC::Encoder, 0> (ArgumentCoders.h:319) ==384967== by 0x6C62639: encode<IPC::Encoder> (ArgumentCoders.h:312) ==384967== by 0x6C62639: operator<< <const std::tuple<const WebKit::LoadParameters&>&> (Encoder.h:72) ==384967== by 0x6C62639: send<Messages::WebPage::LoadAlternateHTML> (MessageSender.h:48) ==384967== by 0x6C62639: send<Messages::WebPage::LoadAlternateHTML> (MessageSender.h:40) ==384967== by 0x6C62639: operator() (WebPageProxy.cpp:1733) ==384967== by 0x6C62639: WTF::Detail::CallableWrapper<WebKit::WebPageProxy::loadAlternateHTML(WTF::Span<unsigned char const, 18446744073709551615ul> const&, WTF::String const&, WTF::URL const&, WTF::URL const&, API::Object*)::{lambda()#1}, void>::call() (Function.h:53) ==384967== by 0x68E9503: Messages::NetworkProcess::AddAllowedFirstPartyForCookies::callReply(IPC::Decoder&, WTF::CompletionHandler<void ()>&&) (NetworkProcessMessageReceiver.cpp:147) ==384967== by 0x6C39B29: operator() (Connection.h:383) ==384967== by 0x6C39B29: WTF::Detail::CallableWrapper<IPC::Connection::makeAsyncReplyHandler<Messages::NetworkProcess::AddAllowedFirstPartyForCookies, WebKit::WebPageProxy::loadAlternateHTML(WTF::Span<unsigned char const, 18446744073709551615ul> const&, WTF::String const&, WTF::URL const&, WTF::URL const&, API::Object*)::{lambda()#1}>(WebKit::WebPageProxy::loadAlternateHTML(WTF::Span<unsigned char const, 18446744073709551615ul> const&, WTF::String const&, WTF::URL const&, WTF::URL const&, API::Object*)::{lambda()#1}&&, WTF::ThreadLikeAssertion)::{lambda(IPC::Decoder*)#1}, void, IPC::Decoder*>::call(IPC::Decoder*) (Function.h:53) ==384967== by 0x6BDFAF5: operator() (Function.h:82) ==384967== by 0x6BDFAF5: operator() (CompletionHandler.h:75) ==384967== by 0x6BDFAF5: operator() (AuxiliaryProcessProxy.cpp:219) ==384967== by 0x6BDFAF5: WTF::Detail::CallableWrapper<WebKit::AuxiliaryProcessProxy::sendMessage(WTF::UniqueRef<IPC::Encoder>&&, WTF::OptionSet<IPC::SendOption>, std::optional<IPC::Connection::AsyncReplyHandler>, WebKit::AuxiliaryProcessProxy::ShouldStartProcessThrottlerActivity)::{lambda(IPC::Decoder*)#2}, void, IPC::Decoder*>::call(IPC::Decoder*) (Function.h:53) ==384967== by 0x6B6F0EF: operator() (Function.h:82) ==384967== by 0x6B6F0EF: operator() (CompletionHandler.h:75) ==384967== by 0x6B6F0EF: IPC::Connection::dispatchMessage(IPC::Decoder&) (Connection.cpp:1180) ==384967== by 0x6B6F28C: IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (Connection.cpp:1242) ==384967== by 0x6B70A85: IPC::Connection::dispatchOneIncomingMessage() (Connection.cpp:1303) ==384967== by 0xB6A26DD: operator() (Function.h:82) ==384967== by 0xB6A26DD: WTF::RunLoop::performWork() (RunLoop.cpp:146) ==384967== by 0xB6FE138: WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) (RunLoopGLib.cpp:80) ==384967== by 0xB6FEA8E: operator() (RunLoopGLib.cpp:53) ==384967== by 0xB6FEA8E: WTF::RunLoop::{lambda(_GSource*, int (*)(void*), void*)#1}::_FUN(_GSource*, int (*)(void*), void*) (RunLoopGLib.cpp:56) ==384967== Address 0x54c47b20 is 0 bytes inside a block of size 128 free'd ==384967== at 0x48450E4: free (vg_replace_malloc.c:884) ==384967== by 0x4AC9A2D: g_free (gmem.c:229) ==384967== by 0x4933A42: ephy_web_view_set_placeholder (ephy-web-view.c:1742) ==384967== by 0x48D15CA: session_parse_embed (ephy-session.c:1293) ==384967== by 0x48D177E: session_start_element (ephy-session.c:1338) ==384967== by 0x4AC6287: emit_start_element (gmarkup.c:1066) ==384967== by 0x4AC6F46: g_markup_parse_context_parse (gmarkup.c:1425) ==384967== by 0x48D1C22: load_stream_read_cb (ephy-session.c:1492) ==384967== by 0x4C4D508: async_ready_callback_wrapper (ginputstream.c:565) ==384967== by 0x4C947BF: g_task_return_now (gtask.c:1259) ==384967== by 0x4C94814: complete_in_idle_cb (gtask.c:1273) ==384967== by 0x4AC35AD: g_idle_dispatch (gmain.c:6124) ==384967== Block was alloc'd at ==384967== at 0x484278A: malloc (vg_replace_malloc.c:392) ==384967== by 0x484770B: realloc (vg_replace_malloc.c:1451) ==384967== by 0x4AC99BA: g_realloc (gmem.c:201) ==384967== by 0x4AED57F: g_string_maybe_expand (gstring.c:92) ==384967== by 0x4AED5E5: g_string_sized_new (gstring.c:116) ==384967== by 0x4AED61D: g_string_new (gstring.c:137) ==384967== by 0x4AC8CCF: g_markup_vprintf_escaped (gmarkup.c:2547) ==384967== by 0x4AC8ECE: g_markup_printf_escaped (gmarkup.c:2636) ==384967== by 0x49339D2: ephy_web_view_set_placeholder (ephy-web-view.c:1738) ==384967== by 0x48D15CA: session_parse_embed (ephy-session.c:1293) ==384967== by 0x48D177E: session_start_element (ephy-session.c:1338) ==384967== by 0x4AC6287: emit_start_element (gmarkup.c:1066)
Michael Catanzaro
Comment 4 2022-11-22 14:47:08 PST
Hi Alex, I suspect 256488@main "Ensure AddAllowedFirstPartyForCookies message is processed before ScheduleResourceLoad" is to blame. I think the problem is the const IPC::DataReference& htmlData contains unowned data and must not be used after the function call completes. It's assigned to loadParameters.data, which after your commit is now used inside a callback that executes after the data has already been freed. So we just need to make a copy somehow. I hope other places do not have a similar problem. IPC::DataReference is really a WTF::span, which is equivalent to std::span, which is a non-owning reference, not a refcount-style reference like our WTF::Ref. The pretty name IPC::DataReference sure *seems* like it should be owned, but it's not. It's just too easy to mess up with C++. :/
Michael Catanzaro
Comment 5 2022-11-22 14:54:42 PST
No clue what to do about this tbh. I guess we should just make a copy before assigning to LoadParameters. But then I worry about other uses of LoadParameters, and other uses of IPC::DataReference. Seems likely that the same problem lurks elsewhere. :/
Michael Catanzaro
Comment 6 2022-11-22 16:44:05 PST
(In reply to Michael Catanzaro from comment #4) > Hi Alex, I suspect 256488@main "Ensure AddAllowedFirstPartyForCookies > message is processed before ScheduleResourceLoad" is to blame. Confirmed, that commit is to blame.
Michael Catanzaro
Comment 7 2022-11-22 17:13:13 PST
Pull request: https://github.com/WebKit/WebKit/pull/6748
Michael Catanzaro
Comment 8 2022-11-22 17:15:57 PST
(In reply to Michael Catanzaro from comment #7) > Pull request: https://github.com/WebKit/WebKit/pull/6748 This fixes the bug in LoadAlternateHTML, but who knows how many other similar problems exist elsewhere. :/
EWS
Comment 9 2022-11-28 13:28:28 PST
Committed 257084@main (baf4b59bb10a): <https://commits.webkit.org/257084@main> Reviewed commits have been landed. Closing PR #6748 and removing active labels.
Radar WebKit Bug Importer
Comment 10 2022-12-12 16:53:44 PST
<rdar://problem/103275823>
Note You need to log in before you can comment on or make changes to this bug.