RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc
Created attachment 367070 [details] Patch
Comment on attachment 367070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367070&action=review > Source/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:-354 > - WebKit::WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID()); What guarantees that WebPage::close() has been called or will be? To remove the message receiver?
Comment on attachment 367070 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367070&action=review >> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKWebProcessPlugInBrowserContextController.mm:-354 >> - WebKit::WebProcess::singleton().removeMessageReceiver(Messages::RemoteObjectRegistry::messageReceiverName(), _page->pageID()); > > What guarantees that WebPage::close() has been called or will be? To remove the message receiver? I think ideally RemoteObjectRegistry would register / unregister itself, it would be less fragile.
Created attachment 367095 [details] Patch
Attachment 367095 [details] did not pass style-queue: ERROR: Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm:46: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 367095 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367095&action=review > Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm:67 > + if (m_shouldUnregisterMessageHandler) { m_shouldUnregisterMessageHandler is a bit of a confusing name. It is not so much about saying we should unregister, it is about knowing if we're registered. I think we should change the name to something like: m_isRegisteredAsMessageReceiver. Alternatively, we could drop m_shouldUnregisterMessageHandler / m_messageReceiverID and replace them by a m_unregisterMessageReceiver WTF::Function data member. > Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm:112 > + page.setRemoteObjectRegistry(*_remoteObjectRegistry); Why don't we do this in the RemoteObjectRegistry constructor instead? Seems safer.
Created attachment 367139 [details] Patch
Attachment 367139 [details] did not pass style-queue: ERROR: Source/WebKit/Shared/API/Cocoa/RemoteObjectRegistry.mm:46: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 367139 [details] Patch Clearing flags on attachment: 367139 Committed r244139: <https://trac.webkit.org/changeset/244139>
All reviewed patches have been landed. Closing bug.
<rdar://problem/49783746>
Comment on attachment 367139 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=367139&action=review > Tools/TestWebKitAPI/Tests/WebKitCocoa/BundleRetainPagePlugIn.mm:40 > + [browserContextController _remoteObjectRegistry]; What is this mysterious line of code? Calling it for side effects or something?
Yes, calling it instantiates a _WKRemoteObjectRegistry associated with this page and would have caused an assertion in the test that uses this bundle class without this fix.
Could we write a comment to make it less mysterious. "Would have caused an assertion" is sort of a mechanical explanation. Would like a short, clear, higher level explanation ideally.
http://trac.webkit.org/r244384