Bug 116428 – [WK2] Notifications clobber each other with multiple processes

• Component: DOM
• Status: RESOLVED FIXED

[Jon Lee]

With multiple processes, the IDs of the notifications from the web processes can clobber over each other, leading to errant behavior.

[Radar WebKit Bug Importer]

<rdar://problem/13935191>

[Jon Lee]

Created attachment 202283 [details]
Patch

[WebKit Commit Bot]

Attachment 202283 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/notification-in-multiple-windows.html', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DerivedSources.pri', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.h', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.messages.in', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp', u'Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp']" exit_code: 1
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:104:  Missing spaces around >>  [whitespace/operators] [3]
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:109:  Missing spaces around >>  [whitespace/operators] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 202283 [details]
Patch

Attachment 202283 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/383119

[Jon Lee]

Created attachment 202302 [details]
Patch

[WebKit Commit Bot]

Attachment 202302 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'ManualTests/notification-in-multiple-windows.html', u'Source/WebKit2/CMakeLists.txt', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DerivedSources.make', u'Source/WebKit2/DerivedSources.pri', u'Source/WebKit2/GNUmakefile.list.am', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.h', u'Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.messages.in', u'Source/WebKit2/UIProcess/WebPageProxy.cpp', u'Source/WebKit2/UIProcess/WebPageProxy.h', u'Source/WebKit2/UIProcess/WebPageProxy.messages.in', u'Source/WebKit2/WebKit2.xcodeproj/project.pbxproj', u'Source/WebKit2/WebProcess/Notifications/NotificationPermissionRequestManager.cpp', u'Source/WebKit2/WebProcess/Notifications/WebNotificationManager.cpp']" exit_code: 1
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:104:  Missing spaces around >>  [whitespace/operators] [3]
Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:109:  Missing spaces around >>  [whitespace/operators] [3]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 202302 [details]
Patch

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

I think all the helper functions used to extract things from the hash maps are confusing. It’s better to use local variables and name them accordingly.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:206
> +    for (const auto& notificationItem: m_notifications) {

Unfortunately we can’t use range-for yet.

[Jon Lee]

Created attachment 202943 [details]
Patch

[Darin Adler]

Comment on attachment 202943 [details]
Patch

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

I am not sure I understand why so many different IDs are needed and the data structure so indirect. For example, why can’t we use a WebPageProxy* in the hash table key rather than a page ID? And why not have this be a per-WebPageProxy data structure that just uses the notification ID as the key and has the WebNotification object as the value. If we don’t want to store it in a WebPageProxy data member, we could still use a hash table in the notification manager proxy, but the key should just be a WebPageProxy and the values could be OwnPtr<HashMap<uint64_t, RefPtr<WebNotification>>>.

If I was doing this, maybe I would have a per-WePageProxy data structure, or maybe a per-WebProcessProxy one, that maps notification IDs from that process to WebNotification objects. I don’t think we need a global notification ID.

I’m going to say review+ but I think we can simplify this in the future.

> Source/WebKit2/ChangeLog:12
> +        With multiple processes, the notification IDs, when passed up to the UI process, can clobber
> +        each other. To fix this, we need to maintain a global map of notification IDs. This map is
> +        keyed by its own unique notification ID, and maps to a pair containing the web page ID and that
> +        web page's ID for the notification.

I don’t understand how it follows that we need a global map of notification IDs. Could WebNotificationManagerProxy instead be made into something that’s per-process?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:106
> -    if (!isNotificationIDValid(notificationID))
> +    if (!isNotificationIDValid(pageNotificationID))
>          return;

This check is important if the notification ID itself is used as a hash key and therefore we have to ensure that it’s neither 0 nor -1. But in the current patch, since the key involves both the page and the ID, this check is not needed nor do I think it’s helpful.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:110
> +    pair<uint64_t, uint64_t> notificationPairID = make_pair(webPage->pageID(), pageNotificationID);

This is a “notification ID pair”, not a “notification pair ID”.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:119
> -    if (!isNotificationIDValid(notificationID))
> +    if (!isNotificationIDValid(pageNotificationID))
>          return;

This check was important before when the notification ID itself was a hash key and therefore we had to ensure that it’s neither 0 nor -1. But now since the key involves both the page and the ID, there’s no need for this check.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:121
> +    const auto& notificationItem = m_notifications.find(make_pair(webPage->pageID(), pageNotificationID));

Since an iterator is a scalar, this can just be "auto" rather than "const auto&".

But why use get here instead of find? With get we would get a pair with zero for both, and so the notification would be 0. I think using find makes the code harder to read.

I’d write:

    if (WebNotification* notification = m_notifications.get(make_pair(webPage->pageID(), pageNotificationID)).second.get())
        m_provider.cancel(notification);

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:134
> +    const auto& notificationItem = m_notifications.find(make_pair(webPage->pageID(), pageNotificationID));

Same comment about auto vs const auto&.

But also, again why not use get?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:158
> +void WebNotificationManagerProxy::clearNotifications(WebPageProxy* webPage)
> +{
> +    clearNotifications(webPage, Vector<uint64_t>(), pageIDsMatch);
> +}

This seems unnecessarily inefficient. We have to iterate the entire m_notifications data structure because we chose to key it only off pairs. Why not group all the notifications for a page in a page-specific hash map so we don’t have to iterate everything?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:163
> +void WebNotificationManagerProxy::clearNotifications(WebPageProxy* webPage, const Vector<uint64_t>& pageNotificationIDs)
> +{
> +    clearNotifications(webPage, pageNotificationIDs, pageAndNotificationIDsMatch);
> +}

This seems like a strange idiom. We have an indexed data structure with webPage and notification ID as keys. But instead of using that indexing, we iterate the whole thing. If we’re going to iterate the whole thing, why not use a Vector instead of a HashMap?

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:183
> +        const pair<uint64_t, uint64_t>& pageNotification = m_globalNotificationMap.take(*it);

There’s no need to say const pair<uint64_t, uint64_t>& here. The const& doesn’t help, and auto would work.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:192
> +    const auto& it = m_globalNotificationMap.find(globalNotificationID);

Just auto would be OK here, no need for const auto&.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.cpp:207
> +    const auto& it = m_globalNotificationMap.find(globalNotificationID);

auto would be fine here, no need for const auto&.

> Source/WebKit2/UIProcess/Notifications/WebNotificationManagerProxy.h:86
> +    // Pair comprised of web page ID and the web process's notification ID
> +    HashMap<uint64_t, pair<uint64_t, uint64_t>> m_globalNotificationMap;
> +    // Key pair comprised of web page ID and the web process's notification ID; value pair comprised of global notification ID, and notification object
> +    HashMap<pair<uint64_t, uint64_t>, pair<uint64_t, RefPtr<WebNotification>>> m_notifications;

These us of "pair" makes this code hard to read. You have to think about what the first and what the second item is. I think I might prefer to use a struct instead so the members could have meaningful names.

[Jon Lee]

Committed r150785: <http://trac.webkit.org/changeset/150785>