Bug 236868 – Provide a WebCore subspaceImplFor template to make code more readable.

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Mark Lam]

The pre-existing code is difficult to follow, especially in CodeGeneratorJS.pm.

[Radar WebKit Bug Importer]

<rdar://problem/89170090>

[Mark Lam]

Created attachment 452596 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 452596 [details]
proposed patch.

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

r=me

> Source/WebCore/bindings/js/WebCoreJSClientData.h:204
> +    auto* clientSpace = new JSC::GCClient::IsoSubspace(*space);
> +    setClient(clientSubspaces, std::unique_ptr<JSC::GCClient::IsoSubspace>(clientSpace));

why not using makeUnique<> ?

[Mark Lam]

Comment on attachment 452596 [details]
proposed patch.

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

>> Source/WebCore/bindings/js/WebCoreJSClientData.h:204
>> +    setClient(clientSubspaces, std::unique_ptr<JSC::GCClient::IsoSubspace>(clientSpace));
> 
> why not using makeUnique<> ?

Because I need return a copy of the raw pointer anyway.

[Mark Lam]

Comment on attachment 452596 [details]
proposed patch.

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

>>> Source/WebCore/bindings/js/WebCoreJSClientData.h:204
>>> +    setClient(clientSubspaces, std::unique_ptr<JSC::GCClient::IsoSubspace>(clientSpace));
>> 
>> why not using makeUnique<> ?
> 
> Because I need return a copy of the raw pointer anyway.

Yusuke tells me that makeUnique has extra verification compared to new e.g. it ensures that the type is FastAllocated.  I'll make the change locally and do a local test build.

[Mark Lam]

Created attachment 452607 [details]
patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r290188: <http://trac.webkit.org/r290188>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 236875

[Mark Lam]

Created attachment 452618 [details]
EWS test 1

[Mark Lam]

Created attachment 452621 [details]
EWS test 2

[Mark Lam]

Created attachment 452622 [details]
EWS test 3

[Mark Lam]

Created attachment 452626 [details]
patch for landing.

[Mark Lam]

OK, we now have the evidence we need to determine what went wrong.  Basically, for some reason, when gcc and MSVC fails to resolve a template function in the class scope, they don't look in outer scopes for a match.  Clang on the other hand is able to do this.  Specifically, in this case, some classes have a method (not a template) with the same name as the WebCore scoped template function, and they have different parameters.  Perhaps the same name thing is tripping up gcc and MSVC, and overloading is failing to work in this scenario.

Regardless, I'm able to work around this by explicitly qualifying the template function with WebCore:: when invoking it.  That has made the build failures go away.

I'll let the EWS bots run longer before I re-land with the workaround.

[Mark Lam]

The max-AS-debug-wk2 bot claims that there is a failure in this test:
platform/mac/media/media-source/videoplaybackquality-decompressionsession.html

https://results.webkit.org/?suite=layout-tests&test=platform/mac/media/media-source/videoplaybackquality-decompressionsession.html shows that test as having a history of crashing wk2 Debug runs on arm64 Macs, though supposedly fixed recently.

Looking at the crash log for this test failure from the EWS bot's failed test run (https://ews-build.s3-us-west-2.amazonaws.com/macOS-AppleSilicon-Big-Sur-Debug-WK2-Tests-EWS/452626-24391-rerun/platform/mac/media/media-source/videoplaybackquality-decompressionsession-crash-log.txt), we see:

Shader compiler output: Metal
ASSERTION FAILED: platformSample.type == PlatformSample::CMSampleBufferType
/Volumes/Data/worker/macOS-AppleSilicon-Big-Sur-Debug-Build-EWS/build/Source/WebKit/GPUProcess/media/RemoteVideoFrameObjectHeap.cpp(99) : void WebKit::RemoteVideoFrameObjectHeap::getVideoFrameBuffer(WebKit::RemoteVideoFrameReadReference &&)
1   0x13c2cd7e8 WTFCrash
2   0x102ab0620 WebKit::Daemon::Decoder::~Decoder()
3   0x1023e1b14 WebKit::RemoteVideoFrameObjectHeap::getVideoFrameBuffer(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&)
4   0x1020ba9d4 void IPC::callMemberFunctionImpl<WebKit::RemoteVideoFrameObjectHeap, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&), std::__1::tuple<WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > > >, 0ul>(WebKit::RemoteVideoFrameObjectHeap*, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&), std::__1::tuple<WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > > >&&, std::__1::integer_sequence<unsigned long, 0ul>)
5   0x1020b9ed8 void IPC::callMemberFunction<WebKit::RemoteVideoFrameObjectHeap, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&), std::__1::tuple<WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > > >, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > > >&&, WebKit::RemoteVideoFrameObjectHeap*, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&))
6   0x1020b6a20 void IPC::handleMessage<Messages::RemoteVideoFrameObjectHeap::GetVideoFrameBuffer, WebKit::RemoteVideoFrameObjectHeap, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&)>(IPC::Connection&, IPC::Decoder&, WebKit::RemoteVideoFrameObjectHeap*, void (WebKit::RemoteVideoFrameObjectHeap::*)(WebKit::ObjectIdentifierReadReference<WebCore::ProcessQualified<WTF::ObjectIdentifier<WebKit::RemoteVideoFrameIdentifierType> > >&&))
7   0x1020b6874 WebKit::RemoteVideoFrameObjectHeap::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
8   0x102ab9fe8 IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&)
9   0x102229434 WebKit::GPUConnectionToWebProcess::dispatchMessage(IPC::Connection&, IPC::Decoder&)
10  0x1020e08ec WebKit::GPUConnectionToWebProcess::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
11  0x102a805b0 IPC::Connection::dispatchMessage(IPC::Decoder&)
...

That doesn't look related to my patch at all.

Running the test individually and locally on my own M1 Mac, the test passes every time.  Running the entire webkit tests on a Debug build locally, I also cannot reproduce a failure in that test.

Based on all the above, I think the test failure is not caused by my patch.  I'm going to re-land the patch now.

[Mark Lam]

Re-landed in r290217: <http://trac.webkit.org/r290217>.