Bug 232988 - IOSurface memory attribution is hard to use in constructors
Summary: IOSurface memory attribution is hard to use in constructors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-11-11 02:23 PST by Kimmo Kinnunen
Modified: 2021-12-10 00:25 PST (History)
17 users (show)

See Also:


Attachments
Patch (53.23 KB, patch)
2021-11-11 02:50 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.55 KB, patch)
2021-11-11 03:05 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.04 KB, patch)
2021-11-11 03:07 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.62 KB, patch)
2021-11-11 03:11 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.61 KB, patch)
2021-11-11 03:26 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (51.08 KB, patch)
2021-11-11 03:32 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (51.71 KB, patch)
2021-11-11 11:59 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (51.71 KB, patch)
2021-11-11 22:51 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (51.19 KB, patch)
2021-11-11 23:08 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (53.58 KB, patch)
2021-11-12 00:53 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (54.60 KB, patch)
2021-11-15 06:22 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (49.72 KB, patch)
2021-12-03 00:39 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (50.26 KB, patch)
2021-12-03 01:35 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (52.83 KB, patch)
2021-12-03 05:11 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (52.60 KB, patch)
2021-12-08 06:04 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (52.65 KB, patch)
2021-12-08 09:08 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (54.40 KB, patch)
2021-12-08 12:06 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch (52.79 KB, patch)
2021-12-08 12:48 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (53.47 KB, patch)
2021-12-09 00:35 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (53.54 KB, patch)
2021-12-09 02:47 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (53.54 KB, patch)
2021-12-09 03:08 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (53.73 KB, patch)
2021-12-09 06:59 PST, Kimmo Kinnunen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (53.59 KB, patch)
2021-12-09 11:56 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff
Patch for landing (53.59 KB, patch)
2021-12-09 23:17 PST, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-11-11 02:23:59 PST
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.
Comment 1 Kimmo Kinnunen 2021-11-11 02:50:26 PST
Created attachment 443927 [details]
Patch
Comment 2 Kimmo Kinnunen 2021-11-11 03:05:42 PST
Created attachment 443928 [details]
Patch
Comment 3 Kimmo Kinnunen 2021-11-11 03:07:32 PST
Created attachment 443929 [details]
Patch
Comment 4 Kimmo Kinnunen 2021-11-11 03:11:05 PST
Created attachment 443930 [details]
Patch
Comment 5 Kimmo Kinnunen 2021-11-11 03:26:27 PST
Created attachment 443931 [details]
Patch
Comment 6 Kimmo Kinnunen 2021-11-11 03:32:10 PST
Created attachment 443932 [details]
Patch
Comment 7 Kimmo Kinnunen 2021-11-11 11:59:03 PST
Created attachment 443985 [details]
Patch
Comment 8 Kimmo Kinnunen 2021-11-11 22:51:24 PST
Created attachment 444050 [details]
Patch
Comment 9 Kimmo Kinnunen 2021-11-11 23:08:23 PST
Created attachment 444051 [details]
Patch
Comment 10 Kimmo Kinnunen 2021-11-12 00:53:51 PST
Created attachment 444052 [details]
Patch
Comment 11 Kimmo Kinnunen 2021-11-15 06:22:15 PST
Created attachment 444247 [details]
Patch
Comment 12 Radar WebKit Bug Importer 2021-11-18 02:24:21 PST
<rdar://problem/85541918>
Comment 13 Kimmo Kinnunen 2021-12-03 00:39:09 PST
Created attachment 445820 [details]
Patch
Comment 14 Kimmo Kinnunen 2021-12-03 01:35:19 PST
Created attachment 445822 [details]
Patch
Comment 15 Kimmo Kinnunen 2021-12-03 05:11:43 PST
Created attachment 445844 [details]
Patch
Comment 16 Chris Dumez 2021-12-07 07:18:01 PST
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.
Comment 17 Chris Dumez 2021-12-07 07:44:22 PST
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.
Comment 18 Kimmo Kinnunen 2021-12-08 06:04:27 PST
Created attachment 446352 [details]
Patch
Comment 19 Chris Dumez 2021-12-08 07:16:15 PST
EWS bubbles are red.
Comment 20 Kimmo Kinnunen 2021-12-08 09:08:41 PST
Created attachment 446374 [details]
Patch
Comment 21 Chris Dumez 2021-12-08 09:53:16 PST
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.
Comment 22 Kimmo Kinnunen 2021-12-08 12:06:32 PST
Created attachment 446406 [details]
Patch
Comment 23 Kimmo Kinnunen 2021-12-08 12:09:35 PST
(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..
Comment 24 Chris Dumez 2021-12-08 12:11:43 PST
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.
Comment 25 Chris Dumez 2021-12-08 12:14:07 PST
(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.
Comment 26 Kimmo Kinnunen 2021-12-08 12:29:41 PST
Cannot put that there, it's an exported class. Exported classes are not allowed to have inline member functions because potential ODR reasons?
Comment 27 Chris Dumez 2021-12-08 12:32:35 PST
(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?
Comment 28 Kimmo Kinnunen 2021-12-08 12:48:03 PST
Created attachment 446413 [details]
Patch
Comment 29 Chris Dumez 2021-12-08 13:16:40 PST
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.
Comment 30 Chris Dumez 2021-12-08 13:17:59 PST
Also, some EWS bubbles seem to be red.
Comment 31 Kimmo Kinnunen 2021-12-09 00:35:58 PST
Created attachment 446505 [details]
Patch for landing
Comment 32 Kimmo Kinnunen 2021-12-09 02:47:29 PST
Created attachment 446510 [details]
Patch for landing
Comment 33 Kimmo Kinnunen 2021-12-09 03:08:40 PST
Created attachment 446512 [details]
Patch for landing
Comment 34 Kimmo Kinnunen 2021-12-09 06:59:41 PST
Created attachment 446546 [details]
Patch for landing
Comment 35 Kimmo Kinnunen 2021-12-09 11:56:42 PST
Created attachment 446581 [details]
Patch for landing
Comment 36 EWS 2021-12-09 23:15:35 PST
ChangeLog entry in Tools/ChangeLog contains OOPS!.
Comment 37 Kimmo Kinnunen 2021-12-09 23:17:24 PST
Created attachment 446669 [details]
Patch for landing
Comment 38 EWS 2021-12-10 00:25:43 PST
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].