Bug 94368 – [WK2] Get rid of WebContext::m_pendingMessagesToPostToInjectedBundle

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

I'm very much unsure about this patch, because there must have been a reason to do this. However, the patch that added m_pendingMessagesToPostToInjectedBundle didn't list any rationale, and I couldn't catch this array being used in debugger, even with some extensions enabled.

Initialization messages are now kept separate, WebProcessProxy has its own m_pendingMessages array in case someone sends other messages early. So, the only case that isn't covered is when WebProcessProxy::isValid() returns false. I do not see how queueing messages sent to a process in this state can be of any use - we'll start from a clean slate when relaunching it anyway.

[Alexey Proskuryakov]

Created attachment 159165 [details]
proposed patch

[Alexey Proskuryakov]

Comment on attachment 159165 [details]
proposed patch

Some things to consider:
- Perhaps injectedBundleInitializationUserData should be sent before all other queued process messages. If we rely on process proxy queueing, that could result in some injected bundle messages being sent before injected bundle initialization.
- If we drop any messages on the floor, consider returning a boolean from postMessageToInjectedBundle().

[Alexey Proskuryakov]

> - Perhaps injectedBundleInitializationUserData should be sent before all other queued process messages. If we rely on process proxy queueing, that could result in some injected bundle messages being sent before injected bundle initialization.

WebProcessProxy::sendMessage only does queueing when the process is launching, so WebProcess::InitializeWebProcess is always the first message already, and it carries injectedBundleInitializationUserData.

> - If we drop any messages on the floor, consider returning a boolean from postMessageToInjectedBundle().

I think that if we're to do something like this, it should be done separately, in a fix that has known observable effect. Just returning false would not be terribly helpful, because that wouldn't tell the caller how many of the processes were offline.

[Alexey Proskuryakov]

Created attachment 165701 [details]
Patch updated to ToT

[Anders Carlsson]

Comment on attachment 165701 [details]
Patch updated to ToT

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

> Source/WebKit2/UIProcess/WebContext.cpp:567
> +        // FIXME: Can we encode the message body outside the loop for all the processes?

Please add another FIXME here too - we should fail to send the message if the body contains any references to WKPageRefs/WKFrameRefs etc since they're local to a process.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/129575>.