Bug 198485 – REGRESSION(r245796): [WPE][GTK] Web process crash on startup

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Alicia Boya García]

#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

[Michael Catanzaro]

I'm far enough into a bisection to suspect r245796 "Use a strongly-typed identifier for pages"

[Michael Catanzaro]

BTW this has left all our bots broken for over a week now....

[Michael Catanzaro]

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.

[Chris Dumez]

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

[Michael Catanzaro]

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

[Michael Catanzaro]

Created attachment 371363 [details]
Patch

[EWS Watchlist]

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

[Chris Dumez]

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.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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;

[Michael Catanzaro]

That works, thanks!

[Michael Catanzaro]

Created attachment 371367 [details]
Patch

[Chris Dumez]

Comment on attachment 371367 [details]
Patch

r=me, thanks.

[WebKit Commit Bot]

Comment on attachment 371367 [details]
Patch

Clearing flags on attachment: 371367

Committed r246102: <https://trac.webkit.org/changeset/246102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

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?

[Carlos Garcia Campos]

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

[Chris Dumez]

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.

[Michael Catanzaro]

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.

[Michael Catanzaro]

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