Bug 224390 – [CoreIPC] Crash in logDiagnosticMessage code

• Component: WebKit2
• Status: RESOLVED FIXED

[Julian Gonzalez]

e.g.

#0 0x7fff205f3d92 in _platform_strlen+0x12
#1 0x10958bd67 in wrap_strlen+0x37
#2 0x7fff269d33f2 in AnalyticsIsEventUsed
#3 0x7fff269d585c in AnalyticsSendEventLazy
#4 0x10bde3ed5 in __44-[WBSAnalyticsLogger _sendEvent:usingBlock:]_block_invoke

<rdar://problem/74595042>

[Julian Gonzalez]

Created attachment 425649 [details]
Patch

[Chris Dumez]

Comment on attachment 425649 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:7175
> +    if (!canLogMessage(message))

Why isn't this using MESSAGE_CHECK()??

[Chris Dumez]

Comment on attachment 425649 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:7169
>  void WebPageProxy::logDiagnosticMessage(const String& message, const String& description, WebCore::ShouldSample shouldSample)

Also note that these function are not only called by IPC. They are also called directly from within the UIProcess. If the check was cheap, I don't think this would be an issue. However, isAllASCII is not that cheap so maybe we want to do the check only in the IPC case. On way would be to introduce a new IPC::ASCIIString class with its own IPC decoder that does the isAllASCII() check and use that type in messages.in.

[Chris Dumez]

Comment on attachment 425649 [details]
Patch

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

>> Source/WebKit/UIProcess/WebPageProxy.cpp:7169
>>  void WebPageProxy::logDiagnosticMessage(const String& message, const String& description, WebCore::ShouldSample shouldSample)
> 
> Also note that these function are not only called by IPC. They are also called directly from within the UIProcess. If the check was cheap, I don't think this would be an issue. However, isAllASCII is not that cheap so maybe we want to do the check only in the IPC case. On way would be to introduce a new IPC::ASCIIString class with its own IPC decoder that does the isAllASCII() check and use that type in messages.in.

Another way, which is more code but would have no cost would be to use an enum for the message keys instead of a String. We'd only need to convert the enum to a String when we actually call the client.

[Chris Dumez]

Comment on attachment 425649 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebPageProxy.cpp:7169
>>>  void WebPageProxy::logDiagnosticMessage(const String& message, const String& description, WebCore::ShouldSample shouldSample)
>> 
>> Also note that these function are not only called by IPC. They are also called directly from within the UIProcess. If the check was cheap, I don't think this would be an issue. However, isAllASCII is not that cheap so maybe we want to do the check only in the IPC case. On way would be to introduce a new IPC::ASCIIString class with its own IPC decoder that does the isAllASCII() check and use that type in messages.in.
> 
> Another way, which is more code but would have no cost would be to use an enum for the message keys instead of a String. We'd only need to convert the enum to a String when we actually call the client.

Per Slack discussion, the easiest way to restrict the check and thus the runtime cost to IPC is probably:
```
Like keep logDiagnosticMessage() as is and public. Add a private logDiagnosticMessageFromWebProcess() that does the MESSAGE_CHECK() and then calls logDiagnosticMessage(). Then rename the IPC message to logDiagnosticMessageFromWebProcess() in WebPageProxy.messages.in
```

[Julian Gonzalez]

Thanks, this ends up working pretty well (and is not too large to implement here). Uploading new patch.

(In reply to Chris Dumez from comment #5)
> Comment on attachment 425649 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425649&action=review
> 
> >>> Source/WebKit/UIProcess/WebPageProxy.cpp:7169
> >>>  void WebPageProxy::logDiagnosticMessage(const String& message, const String& description, WebCore::ShouldSample shouldSample)
> >> 
> >> Also note that these function are not only called by IPC. They are also called directly from within the UIProcess. If the check was cheap, I don't think this would be an issue. However, isAllASCII is not that cheap so maybe we want to do the check only in the IPC case. On way would be to introduce a new IPC::ASCIIString class with its own IPC decoder that does the isAllASCII() check and use that type in messages.in.
> > 
> > Another way, which is more code but would have no cost would be to use an enum for the message keys instead of a String. We'd only need to convert the enum to a String when we actually call the client.
> 
> Per Slack discussion, the easiest way to restrict the check and thus the
> runtime cost to IPC is probably:
> ```
> Like keep logDiagnosticMessage() as is and public. Add a private
> logDiagnosticMessageFromWebProcess() that does the MESSAGE_CHECK() and then
> calls logDiagnosticMessage(). Then rename the IPC message to
> logDiagnosticMessageFromWebProcess() in WebPageProxy.messages.in
> ```

[Julian Gonzalez]

Created attachment 425786 [details]
Patch

[Julian Gonzalez]

Patch here doesn't merge cleanly, I'll upload a new one.

[Julian Gonzalez]

Created attachment 425803 [details]
Patch

[Chris Dumez]

Comment on attachment 425803 [details]
Patch

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

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:428
> +        || decoder.messageName() == Messages::WebPageProxy::LogDiagnosticMessageFromWebProcess::name()

That's not quite right. You'll need to provide implementations for these logging functions on ProvisionalPageProxy which do the MESSAGE_CHECKS with m_process and THEN call the public logging functions on WebPageProxy. This is important because WebPageProxy and ProvisionalPageProxy use different processes. You wouldn't want a bad IPC from the provisional process to kill the committed process.

[Julian Gonzalez]

Created attachment 425819 [details]
Patch

[Chris Dumez]

Comment on attachment 425819 [details]
Patch

r=me assuming the bots are happy.

[EWS]

Committed r275861 (236426@main): <https://commits.webkit.org/236426@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425819 [details].