Bug 238544

Created attachment 456109 [details]
Patch

Created attachment 456220 [details]
Patch

Comment on attachment 456220 [details]
Patch

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

Not confident that all this code

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:627
> +        std::sort(data.begin(), data.end(), [](auto& a, auto& b) { return a.creationTime < b.creationTime; });

Is it safe to use an unstable sort here? If there are any two notifications with the same creationTime, then they could end up shuffled randomly by the sort algorithm. If we used a stable sort, then they would stay in the order they are already in, even if the times happened to be identical.

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:84
> +    for (auto& callback : navigationPreloadStateCallbacks.values())
> +        callback(Exception { AbortError, "context stopped"_s });

This is in hash table (pseudo-random) order. Is that OK?

> Source/WebCore/workers/service/WorkerSWClientConnection.cpp:88
> +    for (auto& callback : getNotificationsCallbacks.values())
>          callback(Exception { AbortError, "context stopped"_s });

Ditto.

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:306
> +    Vector<NotificationData> result;

Should we be reserving initial capacity? Does this typically get sized the same as m_notifications or is it usually a much smaller subset?

> Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:315
> +    callback(WTFMove(result));

Can this vector be long-lived? If so, maybe we should do shrinkToFit after building it and before transferring ownership?

(In reply to Darin Adler from comment #3)
> Comment on attachment 456220 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456220&action=review
> 
> Not confident that all this code
> 
> > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:627
> > +        std::sort(data.begin(), data.end(), [](auto& a, auto& b) { return a.creationTime < b.creationTime; });
> 
> Is it safe to use an unstable sort here? If there are any two notifications
> with the same creationTime, then they could end up shuffled randomly by the
> sort algorithm. If we used a stable sort, then they would stay in the order
> they are already in, even if the times happened to be identical.

We get the vector from a UIProcess HashMap, there is no guarantee the order of the pre-sorted Vector will be stable.
To be bullet proof, we would need to do the sorting in UIProcess and rely on both creationTime and UIProcess incrementing counter.
I'll change this.

> > Source/WebCore/workers/service/WorkerSWClientConnection.cpp:84
> > +    for (auto& callback : navigationPreloadStateCallbacks.values())
> > +        callback(Exception { AbortError, "context stopped"_s });
> 
> This is in hash table (pseudo-random) order. Is that OK?

I think this is ok. These completion handlers can complete in no particular order.

> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:306
> > +    Vector<NotificationData> result;
> 
> Should we be reserving initial capacity? Does this typically get sized the
> same as m_notifications or is it usually a much smaller subset?

Difficult to say.
I would expect most web sites to have a single registration doing notifications but that is not guaranteed. If tag is empty, my guess is that it should be roughly the same as m_notifications. If tag is used, it probably means it is only a small subset.

> > Source/WebKit/UIProcess/Notifications/WebNotificationManagerProxy.cpp:315
> > +    callback(WTFMove(result));
> 
> Can this vector be long-lived? If so, maybe we should do shrinkToFit after
> building it and before transferring ownership?

Vector is short lived, it is sent directly to WebProcess through IPC.

Created attachment 456338 [details]
Patch for landing

Committed r292218 (249121@main): <https://commits.webkit.org/249121@main>

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

<rdar://problem/91158736>