Bug 95465 – [Mac] Registering web views for notifications is unbalanced

• Component: DOM
• Status: RESOLVED FIXED

[Jon Lee]

Setting the notification provider for a WK1 WebView registers that web view to the provider. When the WebView is closed, WebCore is essentially responsible for unregistering the web view. This leads to an imbalance on Lion since notifications are not supported, and the NotificationController responsible for making the unregistration call doesn't exist.

Instead, the unregisterWebView method should be called when the web view is closed, keeping both calls within WebKit.

[Radar WebKit Bug Importer]

<rdar://problem/12206765>

[Jon Lee]

Created attachment 161499 [details]
Patch

[Darin Adler]

Comment on attachment 161499 [details]
Patch

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

> Source/WebKit/mac/WebView/WebView.mm:1143
> +    [[self _notificationProvider] unregisterWebView:self];

What about the _closeWithFastTeardown case? Are we OK not unregistering in that case? Could something bad happen during the rest of the application termination process?

> Source/WebKit/mac/WebView/WebView.mm:6526
>  - (void)_setNotificationProvider:(id<WebNotificationProvider>)notificationProvider
>  {
> -    if (_private) {
> +    if (_private && !_private->_notificationProvider) {
>          _private->_notificationProvider = notificationProvider;
>          [_private->_notificationProvider registerWebView:self];
>      }
>  }

It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.

[Jon Lee]

(In reply to comment #3)
> (From update of attachment 161499 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161499&action=review
> 
> > Source/WebKit/mac/WebView/WebView.mm:1143
> > +    [[self _notificationProvider] unregisterWebView:self];
> 
> What about the _closeWithFastTeardown case? Are we OK not unregistering in that case? Could something bad happen during the rest of the application termination process?

I considered that, but did not see any harm with not unregistering in this situation. The registered web view is called either from WebCore or when there is some user interaction with the platform notification, but in the latter case that is code in the provider.

> 
> > Source/WebKit/mac/WebView/WebView.mm:6526
> >  - (void)_setNotificationProvider:(id<WebNotificationProvider>)notificationProvider
> >  {
> > -    if (_private) {
> > +    if (_private && !_private->_notificationProvider) {
> >          _private->_notificationProvider = notificationProvider;
> >          [_private->_notificationProvider registerWebView:self];
> >      }
> >  }
> 
> It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.

The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation.

Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive.

[Jon Lee]

Committed r127161: <http://trac.webkit.org/changeset/127161>

[Darin Adler]

Comment on attachment 161499 [details]
Patch

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

>>> Source/WebKit/mac/WebView/WebView.mm:6526
>>>  }
>> 
>> It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.
> 
> The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation.
> 
> Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive.

The name change is one possible way to increase clarity. Another would be to add an ASSERT_NOT_REACHED along with early return instead of nesting the function inside an if. The _private == nil case seems expected and harmless while the !_private->_notificationProvider seems like it always reflects a programming error. Might also want to assert that the provider is non-nil or something.

[Jon Lee]

Comment on attachment 161499 [details]
Patch

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

>>>> Source/WebKit/mac/WebView/WebView.mm:6526
>>>>  }
>>> 
>>> It seems a little non-obvious that this method does nothing once a notification provider is set. Are the callers able to tolerate this behavior? The alternative would be to teach this function to unregister the old provider when setting a new one.
>> 
>> The provider is set once, typically during initialization of the web view. There is no reason I can think of that one would want to switch providers in the middle of the web view's lifetime. The case is similar with other kinds of providers like geolocation.
>> 
>> Because of the use case, I opted to just go with initialize-once rather than unregister-the-old-provider, since that is not the intended use for this method. Maybe renaming this to _initializeNotificationProvider would be more descriptive.
> 
> The name change is one possible way to increase clarity. Another would be to add an ASSERT_NOT_REACHED along with early return instead of nesting the function inside an if. The _private == nil case seems expected and harmless while the !_private->_notificationProvider seems like it always reflects a programming error. Might also want to assert that the provider is non-nil or something.

Filed bug 95589 to track this work.