Summary: | RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, ews-watchlist, mkwst, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alex Christensen
2019-04-09 13:53:47 PDT
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. 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. |