Bug 198485 - REGRESSION(r245796): [WPE][GTK] Web process crash on startup
Summary: REGRESSION(r245796): [WPE][GTK] Web process crash on startup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-06-03 09:07 PDT by Alicia Boya García
Modified: 2019-06-05 07:22 PDT (History)
12 users (show)

See Also:


Attachments
Patch (7.02 KB, patch)
2019-06-04 19:39 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (5.05 KB, patch)
2019-06-04 20:10 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alicia Boya García 2019-06-03 09:07:42 PDT
#0  WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:305
#1  0x00007f607bffc44c in WTF::ObjectIdentifier<WebCore::PageIdentifierType>::encode<IPC::Encoder> (this=0x7ffe45293f28, encoder=...)
    at DerivedSources/ForwardingHeaders/wtf/ObjectIdentifier.h:62
#2  0x00007f607bfdf986 in IPC::ArgumentCoder<WTF::ObjectIdentifier<WebCore::PageIdentifierType> >::encode (encoder=..., t=...) at ../../Source/WebKit/Platform/IPC/ArgumentCoder.h:99
#3  0x00007f607bfc4070 in IPC::Encoder::encode<WTF::ObjectIdentifier<WebCore::PageIdentifierType> const&, (void*)0> (this=0x7f605adba000, t=...)
    at ../../Source/WebKit/Platform/IPC/Encoder.h:71
#4  0x00007f607bfb4b5e in IPC::Encoder::operator<< <WTF::ObjectIdentifier<WebCore::PageIdentifierType> const&, (void*)0> (this=0x7f605adba000, t=...)
    at ../../Source/WebKit/Platform/IPC/Encoder.h:84
#5  0x00007f607c530ea0 in WebCore::ResourceRequest::encodeWithPlatformData<IPC::Encoder> (this=0x7ffe45293e68, encoder=...) at DerivedSources/ForwardingHeaders/WebCore/ResourceRequest.h:136
#6  0x00007f607c5300eb in IPC::ArgumentCoder<WebCore::ResourceRequest>::encodePlatformData (encoder=..., resourceRequest=...)
    at ../../Source/WebKit/Shared/soup/WebCoreArgumentCodersSoup.cpp:46
#7  0x00007f607c493074 in IPC::ArgumentCoder<WebCore::ResourceRequest>::encode (encoder=..., resourceRequest=...) at ../../Source/WebKit/Shared/WebCoreArgumentCoders.cpp:1270
#8  0x00007f607be2a610 in IPC::Encoder::encode<WebCore::ResourceRequest const&, (void*)0> (this=0x7f605adba000, t=...) at ../../Source/WebKit/Platform/IPC/Encoder.h:71
#9  0x00007f607be24374 in IPC::Encoder::operator<< <WebCore::ResourceRequest const&, (void*)0> (this=0x7f605adba000, t=...) at ../../Source/WebKit/Platform/IPC/Encoder.h:84
#10 0x00007f607c46a008 in WebKit::LoadParameters::encode (this=0x7ffe45293e60, encoder=...) at ../../Source/WebKit/Shared/LoadParameters.cpp:37
#11 0x00007f607c64fad4 in IPC::ArgumentCoder<WebKit::LoadParameters>::encode (encoder=..., t=...) at ../../Source/WebKit/Platform/IPC/ArgumentCoder.h:99
#12 0x00007f607c64c1a4 in IPC::Encoder::encode<WebKit::LoadParameters const&, (void*)0> (this=0x7f605adba000, t=...) at ../../Source/WebKit/Platform/IPC/Encoder.h:71
#13 0x00007f607c646212 in IPC::Encoder::operator<< <WebKit::LoadParameters const&, (void*)0> (this=0x7f605adba000, t=...) at ../../Source/WebKit/Platform/IPC/Encoder.h:84
#14 0x00007f607c63e54e in IPC::TupleEncoder<1ul, WebKit::LoadParameters const&>::encode (encoder=..., tuple=std::tuple containing = {...})
    at ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:167
#15 0x00007f607c6356b3 in IPC::ArgumentCoder<std::tuple<WebKit::LoadParameters const&> >::encode (encoder=..., tuple=std::tuple containing = {...})
    at ../../Source/WebKit/Platform/IPC/ArgumentCoders.h:239
#16 0x00007f607c6286b6 in IPC::Encoder::encode<std::tuple<WebKit::LoadParameters const&> const&, (void*)0> (this=0x7f605adba000, t=std::tuple containing = {...})
    at ../../Source/WebKit/Platform/IPC/Encoder.h:71
#17 0x00007f607c6111a6 in WebKit::AuxiliaryProcessProxy::send<Messages::WebPage::LoadRequest> (this=0x7f601abfa480, message=..., destinationID=5, sendOptions=...)
    at ../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:141
#18 0x00007f607c5fbe34 in WebKit::AuxiliaryProcessProxy::send<Messages::WebPage::LoadRequest, WebCore::PageIdentifierType> (this=0x7f601abfa480, message=..., destinationID=..., 
    sendOptions=...) at ../../Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:58
#19 0x00007f607c5b4da8 in WebKit::WebPageProxy::loadRequestWithNavigationShared (this=0x7f601a9f3000, process=..., navigation=..., request=..., 
    shouldOpenExternalURLsPolicy=WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, userData=0x0, shouldTreatAsContinuingLoad=WebCore::ShouldTreatAsContinuingLoad::No, 
    websitePolicies=...) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:1118
#20 0x00007f607c5b49e2 in WebKit::WebPageProxy::loadRequest (this=0x7f601a9f3000, request=..., 
    shouldOpenExternalURLsPolicy=WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, userData=0x0) at ../../Source/WebKit/UIProcess/WebPageProxy.cpp:1087
#21 0x00007f607c7cb099 in webkit_web_view_load_uri (webView=0xab7290, uri=0x8a5fd0 "http://localhost:9998/main.html?") at ../../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2694
#22 0x000000000041a88e in main (argc=1, argv=0x7ffe452946f8) at ../../Tools/MiniBrowser/gtk/main.c:608
Comment 1 Michael Catanzaro 2019-06-04 18:52:46 PDT
I'm far enough into a bisection to suspect r245796 "Use a strongly-typed identifier for pages"
Comment 2 Michael Catanzaro 2019-06-04 18:53:36 PDT
BTW this has left all our bots broken for over a week now....
Comment 3 Michael Catanzaro 2019-06-04 19:10:19 PDT
It's just a small mistake in ResourceRequest (it uses 0 to indicate no pageID, but that's illegal now), which would have been difficult to avoid. The problem isn't that the bug was introduced. The problem is we either didn't notice or else ignored it for over a week, and now we've lost an entire week's worth of layout test results. So instead of knowing a small range of commits that introduced a test regression, we're instead going to have a range of 300 commits.
Comment 4 Chris Dumez 2019-06-04 19:24:55 PDT
(In reply to Michael Catanzaro from comment #3)
> It's just a small mistake in ResourceRequest (it uses 0 to indicate no
> pageID, but that's illegal now), which would have been difficult to avoid.
> The problem isn't that the bug was introduced. The problem is we either
> didn't notice or else ignored it for over a week, and now we've lost an
> entire week's worth of layout test results. So instead of knowing a small
> range of commits that introduced a test regression, we're instead going to
> have a range of 300 commits.

an Optional<PageIdentifier> will need to be used. 0 is not a valid strongly typed identifier.
Comment 5 Michael Catanzaro 2019-06-04 19:29:43 PDT
(In reply to Chris Dumez from comment #4)
> an Optional<PageIdentifier> will need to be used. 0 is not a valid strongly
> typed identifier.

Well problem is I don't know how to encode/decode an Optional, so I'm just changing it back to uint64_t for now. If we already have working coders for Optional types, that would be even better.
Comment 6 Michael Catanzaro 2019-06-04 19:39:29 PDT
Created attachment 371363 [details]
Patch
Comment 7 Build Bot 2019-06-04 19:41:21 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 8 Chris Dumez 2019-06-04 19:44:49 PDT
Comment on attachment 371363 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceRequest.h:97
> +        uint64_t initiatingPageID() const { return m_initiatingPageID; }

I'd much rather this used Optional<PageIdentifier>. We are moving away from uint64_t for identifier, this patch is going in the wrong direction.
Comment 9 Chris Dumez 2019-06-04 19:45:17 PDT
(In reply to Michael Catanzaro from comment #5)
> (In reply to Chris Dumez from comment #4)
> > an Optional<PageIdentifier> will need to be used. 0 is not a valid strongly
> > typed identifier.
> 
> Well problem is I don't know how to encode/decode an Optional, so I'm just
> changing it back to uint64_t for now. If we already have working coders for
> Optional types, that would be even better.

Optional gets IPC encoded just fine.
Comment 10 Chris Dumez 2019-06-04 19:46:13 PDT
(In reply to Chris Dumez from comment #9)
> (In reply to Michael Catanzaro from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > an Optional<PageIdentifier> will need to be used. 0 is not a valid strongly
> > > typed identifier.
> > 
> > Well problem is I don't know how to encode/decode an Optional, so I'm just
> > changing it back to uint64_t for now. If we already have working coders for
> > Optional types, that would be even better.
> 
> Optional gets IPC encoded just fine.

Source/WebKit/Platform/IPC/ArgumentCoders.h:    static void encode(Encoder& encoder, const Optional<T>& optional)
Comment 11 Chris Dumez 2019-06-04 19:50:39 PDT
Comment on attachment 371363 [details]
Patch

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

> Source/WebCore/platform/network/soup/ResourceRequest.h:-111
> -        PageIdentifier m_initiatingPageID;

Optional<PageIdentifier>

> Source/WebCore/platform/network/soup/ResourceRequest.h:-160
> -    Optional<PageIdentifier> initiatingPageID;

Optional<Optional<PageIdentifier> initiatingPageID;
Comment 12 Michael Catanzaro 2019-06-04 20:09:50 PDT
That works, thanks!
Comment 13 Michael Catanzaro 2019-06-04 20:10:16 PDT
Created attachment 371367 [details]
Patch
Comment 14 Chris Dumez 2019-06-04 20:13:14 PDT
Comment on attachment 371367 [details]
Patch

r=me, thanks.
Comment 15 WebKit Commit Bot 2019-06-05 00:56:14 PDT
Comment on attachment 371367 [details]
Patch

Clearing flags on attachment: 371367

Committed r246102: <https://trac.webkit.org/changeset/246102>
Comment 16 WebKit Commit Bot 2019-06-05 00:56:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Zan Dobersek 2019-06-05 01:07:04 PDT
Comment on attachment 371367 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:87
> -    request->priv->initiatingPage = WebProcessProxy::webPage(resourceRequest.initiatingPageID());
> +    request->priv->initiatingPage = WebProcessProxy::webPage(*resourceRequest.initiatingPageID());

Is there a guarantee the initiating-page-ID Optional is initialized at this point?
Comment 18 Carlos Garcia Campos 2019-06-05 01:23:57 PDT
(In reply to Zan Dobersek from comment #17)
> Comment on attachment 371367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371367&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:87
> > -    request->priv->initiatingPage = WebProcessProxy::webPage(resourceRequest.initiatingPageID());
> > +    request->priv->initiatingPage = WebProcessProxy::webPage(*resourceRequest.initiatingPageID());
> 
> Is there a guarantee the initiating-page-ID Optional is initialized at this
> point?

Good point, we should indeed check the optional first.
Comment 19 Chris Dumez 2019-06-05 06:58:33 PDT
Comment on attachment 371367 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:87
>>> +    request->priv->initiatingPage = WebProcessProxy::webPage(*resourceRequest.initiatingPageID());
>> 
>> Is there a guarantee the initiating-page-ID Optional is initialized at this point?
> 
> Good point, we should indeed check the optional first.

I assumed it was guaranteed because I think calling webPage() with 0 would have crashed since it would try and look up 0 in a HashMap whose key type is uint64_t.
Comment 20 Michael Catanzaro 2019-06-05 07:15:19 PDT
Comment on attachment 371367 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitURISchemeRequest.cpp:87
> -    request->priv->initiatingPage = WebProcessProxy::webPage(resourceRequest.initiatingPageID());
> +    request->priv->initiatingPage = WebProcessProxy::webPage(*resourceRequest.initiatingPageID());

I could add an ASSERT here to make it more clear that the initiatingPageID is required to be set/engaged. Otherwise, the webkit_uri_scheme_request_get_web_view() public API would be broken. It has to be set.

However, I do see this FIXME:

    // FIXME: initiatingPage is now always null, we need to re-implement this somehow.
    return request->priv->initiatingPage ? webkitWebContextGetWebViewForPage(request->priv->webContext, request->priv->initiatingPage.get()) : nullptr;

which indicates that the API is already broken. The return value is not nullable, so applications should not null-check it, and so returning nullptr is clearly illegal. Looks like this has been broken since r162835 "[GTK] Implement custom URI schemes with CustomProtocols." I don't know how to fix it, I'll create a follow-up bug report. In any case, it seems clear that we should crash rather than allow initiatingPageID to be unset.
Comment 21 Michael Catanzaro 2019-06-05 07:22:11 PDT
(In reply to Chris Dumez from comment #19)
> I assumed it was guaranteed because I think calling webPage() with 0 would
> have crashed since it would try and look up 0 in a HashMap whose key type is
> uint64_t.

Oh well, I missed that, but that too.

Let's continue in bug #198564.