Bug 238544 – ServiceWorkerRegistration.getNotifications should list all persistent notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: Service Workers

[youenn fablet]

ServiceWorkerRegistration.getNotifications should list all persistent notifications

[youenn fablet]

Created attachment 456109 [details]
Patch

[youenn fablet]

Created attachment 456220 [details]
Patch

[Darin Adler]

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?

[youenn fablet]

(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.

[youenn fablet]

Created attachment 456338 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/91158736>