Bug 233988 – Add ability to inject messages into webpushd

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Add ability to inject messages into webpushd

Gated behind an entitlement that TestWebKitAPI and webpushtool will both have.

[Brady Eidson]

Created attachment 446318 [details]
Patch v1

[Brady Eidson]

Created attachment 446372 [details]
Patch v2

[Alex Christensen]

Comment on attachment 446372 [details]
Patch v2

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

> Source/WebKit/Configurations/webpushtool.xcconfig:32
> +OTHER_LDFLAGS = -framework WebKit -l WTF

you shouldn't need to link WTF.  Link JavaScriptCore instead.

> Source/WebKit/Shared/WebPushMessage.h:2
> + * Copyright (C) 2010-2020 Apple Inc. All rights reserved.

2021

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:6486
> +				517B5F9E275F3F54002DC22D /* libicucore.tbd in Frameworks */,

nope

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:9715
> +				517B5F9D275F3F54002DC22D /* libicucore.tbd */,

nope

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:230
> +    result[1] = (message.length() & 0x000000000000FF00) >> 8;

the bitwise and assumes 64-bit size_t, and it's not necessary.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235
> +        result[5 + i] = message[i];

This truncates 256-65535.  not great.

[Brady Eidson]

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235
> > +        result[5 + i] = message[i];
> 
> This truncates 256-65535.  not great.

It didn't, because we're only encoding 8-bit strings.

But I made that more explicit.

[Brady Eidson]

Created attachment 446405 [details]
Patch v3

[Alex Christensen]

Comment on attachment 446405 [details]
Patch v3

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

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:94
> +    CompletionHandler<void(Vector<WebPushMessage>&&)> replyHandler = [completionHandler = WTFMove(completionHandler)] (Vector<WebPushMessage> messages) mutable {

This looks like an unneeded Vector copy.  Can we just pass a Vector&& and avoid this wrapper too?

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:2
> + * Copyright (C) 2010-2020 Apple Inc. All rights reserved.

2021

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:47
> +        Vector<uint8_t> { (const uint8_t*)pushData.bytes, pushData.length },

static_cast

> Source/WebKit/Shared/Cocoa/WebPushMessageCocoa.mm:48
> +        URL { (NSURL *)url }

unnecessary cast

> Source/WebKit/webpushd/PushClientConnection.mm:126
> +        messageIdentifier = makeString("[", signingIdentifer, " (0x", hex(reinterpret_cast<uint64_t>(m_xpcConnection.get()), WTF::HexConversionMode::Lowercase), ")] ");

nit: I think "[" can be '['

> Source/WebKit/webpushd/WebPushDaemon.mm:369
> +            resultMessages.append(WebKit::WebPushMessage { Vector<uint8_t> { reinterpret_cast<const uint8_t*>(data.data()), data.length() }, message.registrationURL });

nit: I wonder if you could use a Span instead of a Vector here because all you are doing is sending it.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WebPushDaemon.mm:235
> +    >(utf8.length() >> 24);

line break is probably a typo

[Brady Eidson]

Created attachment 446424 [details]
PFL

[EWS]

Committed r286764 (245005@main): <https://commits.webkit.org/245005@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/86254247>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 234081

[Brady Eidson]

Created attachment 446561 [details]
New PFL

webpushtool didn't have any dependencies so it was building out of order with WebKit.framework itself.

That's why incremental builds worked, but not clean builds.

[EWS]

Committed r286788 (245029@main): <https://commits.webkit.org/245029@main>

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