Bug 224390

Summary: [CoreIPC] Crash in logDiagnosticMessage code
Product: WebKit Reporter: Julian Gonzalez <julian_a_gonzalez>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, ggaren, kkinnunen, rniwa
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Description Julian Gonzalez 2021-04-09 14:03:33 PDT
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>
Comment 1 Julian Gonzalez 2021-04-09 14:07:36 PDT
Created attachment 425649 [details]
Patch
Comment 2 Chris Dumez 2021-04-09 14:10:55 PDT
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()??
Comment 3 Chris Dumez 2021-04-09 14:17:30 PDT
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.
Comment 4 Chris Dumez 2021-04-09 14:19:19 PDT
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.
Comment 5 Chris Dumez 2021-04-09 14:42:35 PDT
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
```
Comment 6 Julian Gonzalez 2021-04-12 14:03:23 PDT
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
> ```
Comment 7 Julian Gonzalez 2021-04-12 14:07:06 PDT
Created attachment 425786 [details]
Patch
Comment 8 Julian Gonzalez 2021-04-12 14:08:47 PDT
Patch here doesn't merge cleanly, I'll upload a new one.
Comment 9 Julian Gonzalez 2021-04-12 16:07:57 PDT
Created attachment 425803 [details]
Patch
Comment 10 Chris Dumez 2021-04-12 16:18:55 PDT
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.
Comment 11 Julian Gonzalez 2021-04-12 17:45:49 PDT
Created attachment 425819 [details]
Patch
Comment 12 Chris Dumez 2021-04-12 17:46:49 PDT
Comment on attachment 425819 [details]
Patch

r=me assuming the bots are happy.
Comment 13 EWS 2021-04-12 21:57:46 PDT
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].