Bug 73572 – [WK2] Add further support for notifications

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jon Lee]

This includes being able to post and cancel a notification, dispatching appropriate callbacks when the user interacts with the notification, and cleaning up caches when a notification is destroyed.

<rdar://problem/10472195>

[Jon Lee]

Created attachment 117495 [details]
Patch

[Darin Adler]

Comment on attachment 117495 [details]
Patch

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

> Source/WebCore/notifications/Notification.idl:45
> +        attribute EventListener onshow;
> +        // TODO: remove this event listener
>          attribute EventListener ondisplay;

I don’t like this comment. First, we don’t use TODO. Second, we normally use sentence capitalization. Third, this is between two attributes and I don’t know which it refers to. Fourth, this gives no rationale. Why should this be done, and why wasn’t it already done?

> Source/WebKit2/UIProcess/API/C/WKNotification.h:39
> +WK_EXPORT uint64_t WKNotificationGetNotificationID(WKNotificationRef notification);

Can this be just named GetID instead of GetNotificationID?

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:76
>  void WebNotificationManagerProxy::show(const String& title, const String& body, uint64_t notificationID)
>  {
> -    RefPtr<WebNotification> notification = WebNotification::create(title, body);
> +    m_provider.addNotificationManager(this);
> +
> +    RefPtr<WebNotification> notification = WebNotification::create(title, body, notificationID);
> +    m_notifications.set(notificationID, notification);
>      m_provider.show(notification.get());
>  }

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:85
> +    if (!m_notifications.contains(notificationID))
> +        return;
> +    
> +    m_provider.addNotificationManager(this);
> +    WebNotification* notification = m_notifications.get(notificationID).get();
> +    m_provider.cancel(notification);

It’s more efficient to do just a get or a find, not a combination of both a contains and a get. Since get will return 0 I think you can just use get.

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:92
> +void WebNotificationManagerProxy::didDestroyNotification(uint64_t notificationID)
> +{
> +    RefPtr<WebNotification> notification = m_notifications.take(notificationID);
> +    m_provider.didDestroyNotification(notification.get());
> +}

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

You should probably check for null to make it harmless to call this with a bad notification ID.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:100
> +void WebNotificationManager::didShowNotification(uint64_t notificationID)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    if (!m_notificationIDMap.contains(notificationID))
> +        return;
> +    m_notificationIDMap.get(notificationID)->dispatchShowEvent();
> +#endif
> +}

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do the get and check for 0.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:109
> +void WebNotificationManager::didClickNotification(uint64_t notificationID)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    if (!m_notificationIDMap.contains(notificationID))
> +        return;
> +    m_notificationIDMap.get(notificationID)->dispatchClickEvent();
> +#endif
> +}

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do the get and check for 0.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:122
> +void WebNotificationManager::didCloseNotifications(const Vector<uint64_t>& notificationIDs)
> +{
> +#if ENABLE(NOTIFICATIONS)
> +    size_t count = notificationIDs.size();
> +    for (size_t i = 0; i < count; ++i) {
> +        uint64_t notificationID = notificationIDs[i];
> +        if (!m_notificationIDMap.contains(notificationID))
> +            return;
> +        m_notificationIDMap.get(notificationID)->dispatchCloseEvent();
> +    }
>  #endif
>  }

You should probably check for invalid notification IDs, since the hash table code will malfunction spectacularly if you try to use a 0 or -1.

It’s bad for performance to do both a contains and a get. You should just do the get and check for 0.

> Source/WebKit2/WebProcess/WebCoreSupport/WebNotificationClient.cpp:72
>      notImplemented();

Need to use UNUSED_PARAM here.

[Jon Lee]

Thanks for the review.

(In reply to comment #2)
> I don’t like this comment. First, we don’t use TODO. Second, we normally use sentence capitalization. Third, this is between two attributes and I don’t know which it refers to. Fourth, this gives no rationale. Why should this be done, and why wasn’t it already done?

I will fix this comment. This ondisplay event listener should be deprecated in favor of the onshow listener because that is what the latest spec uses. But I will try to get more info on whether anyone is using this listener, and try to remove it right now instead of waiting.

> 
> > Source/WebKit2/UIProcess/API/C/WKNotification.h:39
> > +WK_EXPORT uint64_t WKNotificationGetNotificationID(WKNotificationRef notification);
> 
> Can this be just named GetID instead of GetNotificationID?

Done. I was going for naming consistency (I haven't seen any instances of getID), but it does end up being redundant.

[Jon Lee]

Created attachment 117523 [details]
Patch

[Darin Adler]

Comment on attachment 117523 [details]
Patch

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

> Source/WebCore/notifications/Notification.cpp:40
> +#include "ErrorEvent.h"
> +#include "Event.h"

No need to add the include of Event.h since ErrorEvent.h includes it.

> Source/WebKit2/UIProcess/WebNotificationManagerProxy.cpp:71
> +    ASSERT(notificationID);

0 is not the only bad value for hash tables. 0xFFFFFFFFFFFFFFFF is also bad.

Why is it OK to assert this? What guarantees we won’t get a 0 or -1 here? In particular, if this value comes from the web process, that’s considered “untrusted” since it could be controlled by an attacker.

[Jon Lee]

Created attachment 117534 [details]
Patch #3

Added a new inline function to check the notification ID, and converted most of the previous ASSERTs to use that method.

[Darin Adler]

Comment on attachment 117534 [details]
Patch #3

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

> Source/WebKit2/UIProcess/WebNotification.h:75
> +inline bool isNotificationIDValid(uint64_t id)
> +{
> +    return id && id != static_cast<uint64_t>(-1);
> +}

Would be nice to have a rationale comment explaining why these two values are invalid.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:46
> +    while (!isNotificationIDValid(uniqueNotificationID))
> +        ++uniqueNotificationID;

This is dead code. You should remove it. There’s literally no chance we will ever count from 1 all the way up to 2^64. And I don’t think we need to pretend that the isNotificationIDValid function is a secret abstraction. This is literally untestable code that will never do anything!

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:82
> +    uint64_t notificationID = m_notificationMap.get(notification);
> +    if (!notificationID)
> +        return;

Can this function be passed a null pointer? If so you need a check for that.

> Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp:93
> +    uint64_t notificationID = m_notificationMap.take(notification);
> +    if (!notificationID)
> +        return;

Can this function be passed a null pointer? If so you need a check for that.

[Jon Lee]

(In reply to comment #7)
> (From update of attachment 117534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117534&action=review
> 
> Would be nice to have a rationale comment explaining why these two values are invalid.

Done

> This is dead code. You should remove it. There’s literally no chance we will ever count from 1 all the way up to 2^64. And I don’t think we need to pretend that the isNotificationIDValid function is a secret abstraction. This is literally untestable code that will never do anything!

Done

> Can this function be passed a null pointer? If so you need a check for that.

A null pointer should not be passable, but I've added the check.

[Jon Lee]

Committed r101750: <http://trac.webkit.org/changeset/101750>

[Andras Becsi]

(In reply to comment #9)
> Committed r101750: <http://trac.webkit.org/changeset/101750>

A build fix for Qt/WK2 was landed in http://trac.webkit.org/changeset/101780.
Seems like the Qt port is the only one building with WK2 _and_ ENABLE_NOTIFICATIONS=1.