Bug 196744 – RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

RemoteObjectRegistry message receiver should be removed when WebPage::close is called instead of waiting until dealloc

[Alex Christensen]

Created attachment 367070 [details]
Patch

[Chris Dumez]

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?

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 367095 [details]
Patch

[EWS Watchlist]

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.

[Chris Dumez]

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.

[Alex Christensen]

Created attachment 367139 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 367139 [details]
Patch

Clearing flags on attachment: 367139

Committed r244139: <https://trac.webkit.org/changeset/244139>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49783746>

[Darin Adler]

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?

[Alex Christensen]

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.

[Darin Adler]

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.

[Alex Christensen]

http://trac.webkit.org/r244384