95465 2012-08-30 08:48:10 -0700 [Mac] Registering web views for notifications is unbalanced 2012-08-31 10:53:34 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Mac OS X 10.7 RESOLVED FIXED InRadar P2 Normal --- 1 jonlee jonlee ap jberlin sam webkit-bug-importer oldest_to_newest 708670 0 jonlee 2012-08-30 08:48:10 -0700 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. 708671 1 webkit-bug-importer 2012-08-30 08:48:20 -0700 <rdar://problem/12206765> 708737 2 161499 jonlee 2012-08-30 09:48:59 -0700 Created attachment 161499 Patch 708771 3 161499 darin 2012-08-30 10:09:39 -0700 Comment on attachment 161499 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. 708834 4 jonlee 2012-08-30 10:56:41 -0700 (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. 708835 5 jonlee 2012-08-30 10:59:17 -0700 Committed r127161: <http://trac.webkit.org/changeset/127161> 709784 6 161499 darin 2012-08-31 10:39:09 -0700 Comment on attachment 161499 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. 709801 7 161499 jonlee 2012-08-31 10:53:34 -0700 Comment on attachment 161499 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. 161499 2012-08-30 09:48:59 -0700 2012-08-31 10:53:34 -0700 Patch bug-95465-20120830094848.patch text/plain 4197 jonlee U3VidmVyc2lvbiBSZXZpc2lvbjogMTI3MTUyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L21h Yy9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0L21hYy9DaGFuZ2VMb2cKaW5kZXggMTFmMDM1OGRi NDk3NDUzYWUyMWNiMzM3M2FkYzJmMzBkMTIzZDkxZC4uOWNhMDQyNjZkMmMwNzFjMDI3YTYyNTE0 NjY1ZjBlNDQ5MmNjMDFjZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9tYWMvQ2hhbmdlTG9n CisrKyBiL1NvdXJjZS9XZWJLaXQvbWFjL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI5IEBACisyMDEy LTA4LTMwICBKb24gTGVlICA8am9ubGVlQGFwcGxlLmNvbT4KKworICAgICAgICBbTWFjXSBSZWdp c3RlcmluZyB3ZWIgdmlld3MgZm9yIG5vdGlmaWNhdGlvbnMgaXMgdW5iYWxhbmNlZAorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTU0NjUKKyAgICAgICAg PHJkYXI6Ly9wcm9ibGVtLzEyMjA2NzY1PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAo T09QUyEpLgorCisgICAgICAgIFNldHRpbmcgdGhlIG5vdGlmaWNhdGlvbiBwcm92aWRlciBmb3Ig YSBXSzEgV2ViVmlldyByZWdpc3RlcnMgdGhhdCB3ZWIgdmlldyB0byB0aGUKKyAgICAgICAgcHJv dmlkZXIuIFdoZW4gdGhlIFdlYlZpZXcgaXMgY2xvc2VkLCBXZWJDb3JlIGlzIGVzc2VudGlhbGx5 IHJlc3BvbnNpYmxlIGZvcgorICAgICAgICB1bnJlZ2lzdGVyaW5nIHRoZSB3ZWIgdmlldy4gVGhp cyBsZWFkcyB0byBhbiBpbWJhbGFuY2Ugb24gTGlvbiBzaW5jZSBub3RpZmljYXRpb25zIGFyZQor ICAgICAgICBub3Qgc3VwcG9ydGVkLCBhbmQgdGhlIE5vdGlmaWNhdGlvbkNvbnRyb2xsZXIgcmVz cG9uc2libGUgZm9yIG1ha2luZyB0aGUgdW5yZWdpc3RyYXRpb24KKyAgICAgICAgY2FsbCBkb2Vz bid0IGV4aXN0LgorCisgICAgICAgIEluc3RlYWQsIHRoZSB1bnJlZ2lzdGVyV2ViVmlldyBtZXRo b2Qgc2hvdWxkIGJlIGNhbGxlZCB3aGVuIHRoZSB3ZWIgdmlldyBpcyBjbG9zZWQuCisKKyAgICAg ICAgKiBXZWJWaWV3L1dlYlZpZXdQcml2YXRlLmg6IFJlbW92ZSBfbm90aWZpY2F0aW9uQ29udHJv bGxlckRlc3Ryb3llZCwgc2luY2UgaXQgaXMKKyAgICAgICAgdW5uZWVkZWQuCisKKyAgICAgICAg KiBXZWJDb3JlU3VwcG9ydC9XZWJOb3RpZmljYXRpb25DbGllbnQubW06CisgICAgICAgIChXZWJO b3RpZmljYXRpb25DbGllbnQ6Om5vdGlmaWNhdGlvbkNvbnRyb2xsZXJEZXN0cm95ZWQpOgorICAg ICAgICAqIFdlYlZpZXcvV2ViVmlldy5tbToKKyAgICAgICAgKC1bV2ViVmlldyBfY2xvc2VdKTog VW5yZWdpc3RlciB0aGUgd2ViIHZpZXcgZnJvbSB0aGUgYXNzaWduZWQgcHJvdmlkZXIgaGVyZS4K KyAgICAgICAgKC1bV2ViVmlldyBfc2V0Tm90aWZpY2F0aW9uUHJvdmlkZXI6XSk6IE9ubHkgYWxs b3cgc2V0dGluZyB0aGUgcHJvdmlkZXIgb25jZSwgdG8gYXZvaWQKKyAgICAgICAgcmVnaXN0ZXJp bmcgdGhlIHdlYiB2aWV3IHRvIG11bHRpcGxlIHByb3ZpZGVycy4KKwogMjAxMi0wOC0yOSAgSm9u IExlZSAgPGpvbmxlZUBhcHBsZS5jb20+CiAKICAgICAgICAgW01hY10gQWRkIGljb25VUkwgdG8g V2ViTm90aWZpY2F0aW9uCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L21hYy9XZWJDb3JlU3Vw cG9ydC9XZWJOb3RpZmljYXRpb25DbGllbnQubW0gYi9Tb3VyY2UvV2ViS2l0L21hYy9XZWJDb3Jl U3VwcG9ydC9XZWJOb3RpZmljYXRpb25DbGllbnQubW0KaW5kZXggZmQ4NTQ3YWFlOWVhOWM3ZmYx NWE5M2E1MzFmOTk0MWRmOGQ3NzgyNi4uODRhOWQ2YWNmMDRmYTc5YWRiNGMxMWUzNTMzMDAyNzkx MjJlMmU5YSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9tYWMvV2ViQ29yZVN1cHBvcnQvV2Vi Tm90aWZpY2F0aW9uQ2xpZW50Lm1tCisrKyBiL1NvdXJjZS9XZWJLaXQvbWFjL1dlYkNvcmVTdXBw b3J0L1dlYk5vdGlmaWNhdGlvbkNsaWVudC5tbQpAQCAtMTYwLDkgKzE2MCw2IEBAIHZvaWQgV2Vi Tm90aWZpY2F0aW9uQ2xpZW50Ojpub3RpZmljYXRpb25PYmplY3REZXN0cm95ZWQoTm90aWZpY2F0 aW9uKiBub3RpZmljYXRpCiAKIHZvaWQgV2ViTm90aWZpY2F0aW9uQ2xpZW50Ojpub3RpZmljYXRp b25Db250cm9sbGVyRGVzdHJveWVkKCkKIHsKLSNpZiBFTkFCTEUoTk9USUZJQ0FUSU9OUykgfHwg RU5BQkxFKExFR0FDWV9OT1RJRklDQVRJT05TKQotICAgIFttX3dlYlZpZXcgX25vdGlmaWNhdGlv bkNvbnRyb2xsZXJEZXN0cm95ZWRdOwotI2VuZGlmCiAgICAgZGVsZXRlIHRoaXM7CiB9CiAKZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViVmlldy5tbSBiL1NvdXJjZS9X ZWJLaXQvbWFjL1dlYlZpZXcvV2ViVmlldy5tbQppbmRleCA2MGIzNTNmMmQ0NmYwZTg3MjBhYmRi NTI1ZWExN2Y5MzQwZGI2YTk3Li44OGFmMjBjYjBiMWRhMGFhZmZiZGY0MTE5ZjE5YWZmODg0YzZi YjU1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L21hYy9XZWJWaWV3L1dlYlZpZXcubW0KKysr IGIvU291cmNlL1dlYktpdC9tYWMvV2ViVmlldy9XZWJWaWV3Lm1tCkBAIC0xMTQwLDYgKzExNDAs OCBAQCBzdGF0aWMgYm9vbCBmYXN0RG9jdW1lbnRUZWFyZG93bkVuYWJsZWQoKQogICAgIFtzZWxm IF9jbGVhckdsaWJMb29wT2JzZXJ2ZXJdOwogI2VuZGlmCiAKKyAgICBbW3NlbGYgX25vdGlmaWNh dGlvblByb3ZpZGVyXSB1bnJlZ2lzdGVyV2ViVmlldzpzZWxmXTsKKwogICAgIFtbTlNEaXN0cmli dXRlZE5vdGlmaWNhdGlvbkNlbnRlciBkZWZhdWx0Q2VudGVyXSByZW1vdmVPYnNlcnZlcjpzZWxm XTsKICAgICBbW05TTm90aWZpY2F0aW9uQ2VudGVyIGRlZmF1bHRDZW50ZXJdIHJlbW92ZU9ic2Vy dmVyOnNlbGZdOwogCkBAIC02NTE3LDE3ICs2NTE5LDEyIEBAIHN0YXRpYyB2b2lkIGdsaWJDb250 ZXh0SXRlcmF0aW9uQ2FsbGJhY2soQ0ZSdW5Mb29wT2JzZXJ2ZXJSZWYsIENGUnVuTG9vcEFjdGl2 aXR5CiBAaW1wbGVtZW50YXRpb24gV2ViVmlldyAoV2ViVmlld05vdGlmaWNhdGlvbikKIC0gKHZv aWQpX3NldE5vdGlmaWNhdGlvblByb3ZpZGVyOihpZDxXZWJOb3RpZmljYXRpb25Qcm92aWRlcj4p bm90aWZpY2F0aW9uUHJvdmlkZXIKIHsKLSAgICBpZiAoX3ByaXZhdGUpIHsKKyAgICBpZiAoX3By aXZhdGUgJiYgIV9wcml2YXRlLT5fbm90aWZpY2F0aW9uUHJvdmlkZXIpIHsKICAgICAgICAgX3By aXZhdGUtPl9ub3RpZmljYXRpb25Qcm92aWRlciA9IG5vdGlmaWNhdGlvblByb3ZpZGVyOwogICAg ICAgICBbX3ByaXZhdGUtPl9ub3RpZmljYXRpb25Qcm92aWRlciByZWdpc3RlcldlYlZpZXc6c2Vs Zl07CiAgICAgfQogfQogCi0tICh2b2lkKV9ub3RpZmljYXRpb25Db250cm9sbGVyRGVzdHJveWVk Ci17Ci0gICAgW1tzZWxmIF9ub3RpZmljYXRpb25Qcm92aWRlcl0gdW5yZWdpc3RlcldlYlZpZXc6 c2VsZl07Ci19Ci0KIC0gKGlkPFdlYk5vdGlmaWNhdGlvblByb3ZpZGVyPilfbm90aWZpY2F0aW9u UHJvdmlkZXIKIHsKICAgICBpZiAoX3ByaXZhdGUpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0 L21hYy9XZWJWaWV3L1dlYlZpZXdQcml2YXRlLmggYi9Tb3VyY2UvV2ViS2l0L21hYy9XZWJWaWV3 L1dlYlZpZXdQcml2YXRlLmgKaW5kZXggYWRiMGNlOWU4NmMxMDc4OTA1ODQ0YTE2YjBlZGIwNWVk MTc3YWM2Mi4uODI2NDc0ZTQxYjM0YzIyYmZmNTQ1NjBmNmEwYjE4ZjcyM2Y5MDdlMyAxMDA2NDQK LS0tIGEvU291cmNlL1dlYktpdC9tYWMvV2ViVmlldy9XZWJWaWV3UHJpdmF0ZS5oCisrKyBiL1Nv dXJjZS9XZWJLaXQvbWFjL1dlYlZpZXcvV2ViVmlld1ByaXZhdGUuaApAQCAtNzM0LDcgKzczNCw2 IEBAIENvdWxkIGJlIHdvcnRoIGFkZGluZyB0byB0aGUgQVBJLgogQGludGVyZmFjZSBXZWJWaWV3 IChXZWJWaWV3Tm90aWZpY2F0aW9uKQogLSAodm9pZClfc2V0Tm90aWZpY2F0aW9uUHJvdmlkZXI6 KGlkPFdlYk5vdGlmaWNhdGlvblByb3ZpZGVyPilub3RpZmljYXRpb25Qcm92aWRlcjsKIC0gKGlk PFdlYk5vdGlmaWNhdGlvblByb3ZpZGVyPilfbm90aWZpY2F0aW9uUHJvdmlkZXI7Ci0tICh2b2lk KV9ub3RpZmljYXRpb25Db250cm9sbGVyRGVzdHJveWVkOwogCiAtICh2b2lkKV9ub3RpZmljYXRp b25EaWRTaG93Oih1aW50NjRfdClub3RpZmljYXRpb25JRDsKIC0gKHZvaWQpX25vdGlmaWNhdGlv bkRpZENsaWNrOih1aW50NjRfdClub3RpZmljYXRpb25JRDsK