Bug 239042 – Handle public token updates in webpushd

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ben Nham]

Handle public token updates in webpushd

[Ben Nham]

Created attachment 457189 [details]
Patch

[Ben Nham]

<rdar://90455314>

[Ben Nham]

Created attachment 457190 [details]
Patch

[Chris Dumez]

Comment on attachment 457190 [details]
Patch

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

> Source/WebCore/Modules/push-api/PushDatabase.cpp:44
> +#define PUSHDB_ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))

I think the modern way to do this is std::size().

> Source/WebCore/Modules/push-api/PushDatabase.cpp:52
> +static constexpr ASCIILiteral publicTokenKey = "publicToken"_s;

Is this intentionally outside the namespace?

> Source/WebCore/Modules/push-api/PushDatabase.cpp:95
> +    PUSHDB_ARRAY_SIZE(pushDatabaseSchemaV1Statements),

Why do we need an array for the counts? Why can't we just use std::size() in our code whenever we need the array size?
std::size() is constexpr.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:100
> +static constexpr int currentPushDatabaseVersion = PUSHDB_ARRAY_SIZE(pushDatabaseSchemaStatements);

Why is this signed?
Seems it should be a size_t or similar.

Same for your loop indexes below.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:320
> +    dispatchOnWorkQueue([this, newPublicToken = Vector { publicToken.data(), publicToken.size() }, completionHandler = WTFMove(completionHandler)]() mutable {

You can construct a Vector from a Span directly: Vector { publicToken }

Also, what guarantees that |this| is still alive when the lambda runs on the work queue?

> Source/WebCore/Modules/push-api/PushDatabase.cpp:385
> +        completeOnMainQueue(WTFMove(completionHandler), result);

WTFMove(result)

BTW, the `WTFMove(result)` inside completeOnMainQueue() should probably be a std::forward(result), not a move.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:391
> +    dispatchOnWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {

Same comment as above about the lifetime of |this|.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:408
> +        completeOnMainQueue(WTFMove(completionHandler), result);

WTFMove(result)

> Source/WebKit/webpushd/ApplePushServiceConnection.mm:54
> +        _connection->didReceivePublicToken(Vector<uint8_t> { reinterpret_cast<const uint8_t*>(publicToken.bytes), publicToken.length });

static_cast<> would be a lot nicer than a reinterpret_cast<> and should work given that the input is a const void*.

> Source/WebKit/webpushd/PushService.mm:119
> +    m_connection->startListeningForPublicToken([this](auto&& token) {

What guarantees that |this| is still alive when the lambda runs?

> Source/WebKit/webpushd/PushService.mm:635
> +    m_database->updatePublicToken(token, [this](auto result) mutable {

Same comment as above about lifetime.

> Source/WebKit/webpushd/PushServiceConnection.mm:48
> +    m_pendingPublicToken.swap(token);

We don't use swap() much in new code. Should probably be:
m_publicTokenHandler(WTFMove(m_pendingPublicToken));
or
m_publicTokenHandler(std::exchange(m_pendingPublicToken, { }));

> Source/WebKit/webpushd/WebPushDaemon.mm:740
> +    runAfterStartingPushService([this, publicToken, replySender = WTFMove(replySender)]() mutable {

I am guessing the |this| here is OK because Daemon is a singleton, right?

> Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:46
> +static std::unique_ptr<PushDatabase> createDatabaseSync(String path)

const String&

> Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:562
> +static bool createDatabaseFromStatements(String path, ASCIILiteral* statements, size_t count)

const String&

> Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:596
> +    auto count = sizeof(pushDatabaseV2Statements) / sizeof(pushDatabaseV2Statements[0]);

std::size()

[Ben Nham]

In terms of lifetime concerns, the classes in the WebPushDaemon namespace (WebPushDaemon, PushService, and PushServiceConnection) are only meant to be used by webpushd, and all are ultimately owned either directly or indirectly by WebPushDaemon, which is a NeverDestroyed singleton. So in the previous patches I left out the weak/protectThis logic when using lambdas as it would never be used, and makes the code simpler to read.

The same is true for PushDatabase, although since it's in WebCore and I suppose it's possible that someone could try to use it outside of webpushd, there is some logic in that class's destructor which prevents the instance from being destroyed while any existing work is outstanding on the work queue.

std::size sounds like what I should be using instead of a macro, but I don't think it will work for finding the size of an array in the array-of-arrays (e.g. for pushDatabaseSchemaStatements)? That is why that array of counts exists.

(In reply to Chris Dumez from comment #4)
> Comment on attachment 457190 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457190&action=review
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:44
> > +#define PUSHDB_ARRAY_SIZE(arr) (sizeof(arr) / sizeof(arr[0]))
> 
> I think the modern way to do this is std::size().
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:52
> > +static constexpr ASCIILiteral publicTokenKey = "publicToken"_s;
> 
> Is this intentionally outside the namespace?
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:95
> > +    PUSHDB_ARRAY_SIZE(pushDatabaseSchemaV1Statements),
> 
> Why do we need an array for the counts? Why can't we just use std::size() in
> our code whenever we need the array size?
> std::size() is constexpr.
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:100
> > +static constexpr int currentPushDatabaseVersion = PUSHDB_ARRAY_SIZE(pushDatabaseSchemaStatements);
> 
> Why is this signed?
> Seems it should be a size_t or similar.
> 
> Same for your loop indexes below.
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:320
> > +    dispatchOnWorkQueue([this, newPublicToken = Vector { publicToken.data(), publicToken.size() }, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> You can construct a Vector from a Span directly: Vector { publicToken }
> 
> Also, what guarantees that |this| is still alive when the lambda runs on the
> work queue?
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:385
> > +        completeOnMainQueue(WTFMove(completionHandler), result);
> 
> WTFMove(result)
> 
> BTW, the `WTFMove(result)` inside completeOnMainQueue() should probably be a
> std::forward(result), not a move.
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:391
> > +    dispatchOnWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Same comment as above about the lifetime of |this|.
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:408
> > +        completeOnMainQueue(WTFMove(completionHandler), result);
> 
> WTFMove(result)
> 
> > Source/WebKit/webpushd/ApplePushServiceConnection.mm:54
> > +        _connection->didReceivePublicToken(Vector<uint8_t> { reinterpret_cast<const uint8_t*>(publicToken.bytes), publicToken.length });
> 
> static_cast<> would be a lot nicer than a reinterpret_cast<> and should work
> given that the input is a const void*.
> 
> > Source/WebKit/webpushd/PushService.mm:119
> > +    m_connection->startListeningForPublicToken([this](auto&& token) {
> 
> What guarantees that |this| is still alive when the lambda runs?
> 
> > Source/WebKit/webpushd/PushService.mm:635
> > +    m_database->updatePublicToken(token, [this](auto result) mutable {
> 
> Same comment as above about lifetime.
> 
> > Source/WebKit/webpushd/PushServiceConnection.mm:48
> > +    m_pendingPublicToken.swap(token);
> 
> We don't use swap() much in new code. Should probably be:
> m_publicTokenHandler(WTFMove(m_pendingPublicToken));
> or
> m_publicTokenHandler(std::exchange(m_pendingPublicToken, { }));
> 
> > Source/WebKit/webpushd/WebPushDaemon.mm:740
> > +    runAfterStartingPushService([this, publicToken, replySender = WTFMove(replySender)]() mutable {
> 
> I am guessing the |this| here is OK because Daemon is a singleton, right?
> 
> > Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:46
> > +static std::unique_ptr<PushDatabase> createDatabaseSync(String path)
> 
> const String&
> 
> > Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:562
> > +static bool createDatabaseFromStatements(String path, ASCIILiteral* statements, size_t count)
> 
> const String&
> 
> > Tools/TestWebKitAPI/Tests/WebCore/PushDatabase.cpp:596
> > +    auto count = sizeof(pushDatabaseV2Statements) / sizeof(pushDatabaseV2Statements[0]);
> 
> std::size()

[Ben Nham]

Created attachment 457210 [details]
Patch feedback

[Darin Adler]

Comment on attachment 457210 [details]
Patch feedback

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

> Source/WebCore/Modules/push-api/PushDatabase.cpp:56
>  static const ASCIILiteral pushDatabaseSchemaV1Statements[] = {

Should use constexpr.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:60
> +static const ASCIILiteral pushDatabaseSchemaV2Statements[] = {

Ditto.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:84
> +static const ASCIILiteral pushDatabaseSchemaV3Statements[] = {

Ditto.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:88
> +static const ASCIILiteral* pushDatabaseSchemaStatements[] = {

Should use constexpr in addition or add a const after the "*". As written, this array is not a constant.

> Source/WebCore/Modules/push-api/PushDatabase.cpp:94
> +static const size_t pushDatabaseSchemaStatementCounts[] = {

Should be constexpr.

[Ben Nham]

Created attachment 457540 [details]
Use span to get rid of count array

[Ben Nham]

Created attachment 457544 [details]
Patch feedback

[Darin Adler]

Comment on attachment 457544 [details]
Patch feedback

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

> Source/WebCore/Modules/push-api/PushDatabase.cpp:159
> +    size_t version = 0;

The type "size_t" seems strange for a version number. Normally we’d use that for things that depend on the memory model, not a plain old number. For a version number we’d want to use a type like uint32_t or uint64_t.

Are we switching because this needs to be 64-bit on 64-bit systems? Or because it needs to be unsigned? Or maybe that you want this to be >32-bit on 64-bit systems? Or maybe it’s just handy to have a type that is 64-bit, yet we can log with %z and not need fancy macros? Sorry to be so critical on such a small point, but this does not seem quite right.

[Ben Nham]

In the first patch, it was an int, because that's what made sense to me; that's the type that `PRAGMA user_version` will provide us from the database. But I changed it to a size_t due to Chris's comments.

I'll probably switch this back to being an int because that seems more correct. We're persisting an int, so that should be the type that we compare it to as well.

(In reply to Darin Adler from comment #10)
> Comment on attachment 457544 [details]
> Patch feedback
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457544&action=review
> 
> > Source/WebCore/Modules/push-api/PushDatabase.cpp:159
> > +    size_t version = 0;
> 
> The type "size_t" seems strange for a version number. Normally we’d use that
> for things that depend on the memory model, not a plain old number. For a
> version number we’d want to use a type like uint32_t or uint64_t.
> 
> Are we switching because this needs to be 64-bit on 64-bit systems? Or
> because it needs to be unsigned? Or maybe that you want this to be >32-bit
> on 64-bit systems? Or maybe it’s just handy to have a type that is 64-bit,
> yet we can log with %z and not need fancy macros? Sorry to be so critical on
> such a small point, but this does not seem quite right.

[Ben Nham]

Created attachment 457548 [details]
Patch feedback

[Brady Eidson]

Comment on attachment 457548 [details]
Patch feedback

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

> Source/WebKit/webpushd/PushServiceConnection.h:46
> +    using PublicTokenHandler = Function<void(Vector<uint8_t>&&)>;

I generally really dislike these types of "using"s and typedefs. I think they often make it harder to reason about code in other places.

This is a good example of that - this is masking over a very straightforward "function that takes a uint8_t vector" with an unrelated name of "PublicTokenHandler"

I don't think this "using" makes the code better. I think it makes the code less readable.  More on this below:

> Source/WebKit/webpushd/PushServiceConnection.h:87
> +    PublicTokenHandler m_publicTokenHandler;
> +    Vector<uint8_t> m_pendingPublicToken;

These two members are obviously related to each other. There's obviously a symmetry to them.
I think it would be more readable here (and in other places) if it simply read:

Function<void(Vector<uint8_t>&&)>  m_publicTokenChangeHandler;
Vector<uint8_t> m_pendingPublicToken;

(Notice I also renamed the function's variable name to make it even more clear what it's for)

[Ben Nham]

Created attachment 457956 [details]
Patch for landing

[EWS]

Committed r293060 (249791@main): <https://commits.webkit.org/249791@main>

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