IOSurface memory attribution is hard to use in constructors

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

IOSurface memory attribution is hard to use in constructors because currently the feature is implemented always in ifdefs.
Constructor taking a memory attribution token would need to be duplicated once for Cocoa and once for no-cocoa (no ifdefs).

The concept probably is useful cross-platform, so maybe it could be first-class WebKit code.

[Kimmo Kinnunen]

Created attachment 443927 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443928 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443929 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443930 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443931 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443932 [details]
Patch

[Kimmo Kinnunen]

Created attachment 443985 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444050 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444051 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444052 [details]
Patch

[Kimmo Kinnunen]

Created attachment 444247 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/85541918>

[Kimmo Kinnunen]

Created attachment 445820 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445822 [details]
Patch

[Kimmo Kinnunen]

Created attachment 445844 [details]
Patch

[Chris Dumez]

Comment on attachment 445844 [details]
Patch

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

> Source/WebCore/platform/ProcessIdentity.h:41
> +class WEBCORE_EXPORT ProcessIdentity final : public ThreadSafeRefCounted<ProcessIdentity> {

Wrapping an integer (task_id_token_t) into a thread-safe-refcounted pointer seems overkill to me
I can understanding wanting a cross-platform wrapper for the task_id_token_t but I have trouble understanding why a simple wrapper doesn't suffice and why we need a thread-safe-refcounted pointer.

[Chris Dumez]

Comment on attachment 445844 [details]
Patch

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

>> Source/WebCore/platform/ProcessIdentity.h:41
>> +class WEBCORE_EXPORT ProcessIdentity final : public ThreadSafeRefCounted<ProcessIdentity> {
> 
> Wrapping an integer (task_id_token_t) into a thread-safe-refcounted pointer seems overkill to me
> I can understanding wanting a cross-platform wrapper for the task_id_token_t but I have trouble understanding why a simple wrapper doesn't suffice and why we need a thread-safe-refcounted pointer.

Note that Mach send rights are already thread-safe refcounted AFAIK (see MachSendRight copy constructor).

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:491
> +    task_id_token_t ownerTaskIdToken = resourceOwner ? resourceOwner->taskIdToken() : 0;

I am not sure what it does to call IOSurfaceSetOwnershipIdentity() with 0. All in all, I think I'd prefer it if IOSurface::setOwnershipIdentity() took in a C++ reference. Call sites can always pass their own handle if they want to "reset" ownership.

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:271
> +    WTFLogAlways("DATAURL:%s", urlString.utf8().data());

Probably shouldn't be here.

[Kimmo Kinnunen]

Created attachment 446352 [details]
Patch

[Chris Dumez]

EWS bubbles are red.

[Kimmo Kinnunen]

Created attachment 446374 [details]
Patch

[Chris Dumez]

Comment on attachment 446374 [details]
Patch

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

> Source/WebCore/platform/ProcessIdentity.h:55
> +    const Handle& handle() const;

Why do we need a public Handle type?

Can we just define an IPC encoder / decoder for ProcessIdentity and have clients only deal with ProcessIdentity objects (never handles?)

> Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272
> +    WTFLogAlways("DATAURL:%s", urlString.utf8().data());

You still haven't removed this extra logging.

[Kimmo Kinnunen]

Created attachment 446406 [details]
Patch

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #21)
> Why do we need a public Handle type?
> Can we just define an IPC encoder / decoder for ProcessIdentity and have
> clients only deal with ProcessIdentity objects (never handles?)

Done.

> > Source/WebKit/GPUProcess/graphics/RemoteRenderingBackend.cpp:272
> > +    WTFLogAlways("DATAURL:%s", urlString.utf8().data());
> 
> You still haven't removed this extra logging.

Sorry, have been rebasing this so many times on different computers, I thought I removed it already but re-uploaded a version with the removal removed..

[Chris Dumez]

Comment on attachment 446406 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3237
> +void ArgumentCoder<ProcessIdentity>::encode(Encoder& encoder, const ProcessIdentity& identity)

As far as I know, we don't put coders in WebCoreArgumentCoders.cpp anymore. We now add the template specializations directly on the class. See ProcessQualified.h for e.g.

[Chris Dumez]

(In reply to Chris Dumez from comment #24)
> Comment on attachment 446406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446406&action=review
> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3237
> > +void ArgumentCoder<ProcessIdentity>::encode(Encoder& encoder, const ProcessIdentity& identity)
> 
> As far as I know, we don't put coders in WebCoreArgumentCoders.cpp anymore.
> We now add the template specializations directly on the class. See
> ProcessQualified.h for e.g.

The reason is that it is just nicer and easier to maintain if the class and its coders and in the same header.

[Kimmo Kinnunen]

Cannot put that there, it's an exported class. Exported classes are not allowed to have inline member functions because potential ODR reasons?

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #26)
> Cannot put that there, it's an exported class. Exported classes are not
> allowed to have inline member functions because potential ODR reasons?

Then don't export the class and only export the non inline functions?

[Kimmo Kinnunen]

Created attachment 446413 [details]
Patch

[Chris Dumez]

Comment on attachment 446413 [details]
Patch

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

> Source/WebCore/platform/ProcessIdentity.cpp:49
> +ProcessIdentity::ProcessIdentity() = default;

Do we really still need to go out of our way to un-inline all of these? Can't we let the compiler generate all those and omit them here and in the header?

> Source/WebCore/platform/ProcessIdentity.cpp:62
> +task_id_token_t ProcessIdentity::taskIdToken() const

We can probably inline this too?

> Source/WebCore/platform/ProcessIdentity.h:49
> +    WEBCORE_EXPORT ProcessIdentity();

Same comment as above.

> Source/WebCore/platform/graphics/RemoteVideoSample.h:53
> +    void setOwnershipIdentity(const ProcessIdentity& resourceOwner);

I think can omit the parameter name.

> Source/WebCore/platform/graphics/cocoa/GraphicsContextGLCocoa.mm:210
> +    m_resourceOwner = ProcessIdentity { WTFMove(resourceOwner) };

Doesn't `m_resourceOwner = WTFMove(resourceOwner);` work?

> Source/WebCore/platform/graphics/cocoa/IOSurface.h:163
> +    WEBCORE_EXPORT void setOwnershipIdentity(const ProcessIdentity& resourceOwner);

We can probably omit the parameter name.

> Source/WebCore/platform/graphics/cocoa/IOSurface.mm:490
> +    ASSERT(resourceOwner);

Shouldn't the assertion be inside the #if check?

> Source/WebKit/GPUProcess/webrtc/LibWebRTCCodecsProxy.mm:80
> +        resourceOwner = gpuConnectionToWebProcess.webProcessIdentity(),

now that there is no longer any #if, we should probably put all the lambda captures on a single line.

> Source/WebKit/WebProcess/GPU/graphics/cocoa/ImageBufferShareableMappedIOSurfaceBackend.h:53
> +    void setOwnershipIdentity(const WebCore::ProcessIdentity& resourceOwner);

We can probably omit the parameter name.

[Chris Dumez]

Also, some EWS bubbles seem to be red.

[Kimmo Kinnunen]

Created attachment 446505 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 446510 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 446512 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 446546 [details]
Patch for landing

[Kimmo Kinnunen]

Created attachment 446581 [details]
Patch for landing

[EWS]

ChangeLog entry in Tools/ChangeLog contains OOPS!.

[Kimmo Kinnunen]

Created attachment 446669 [details]
Patch for landing

[EWS]

Committed r286838 (245071@main): <https://commits.webkit.org/245071@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446669 [details].