[Cocoa] Create a simulated crash log when the UI Process receives an invalid IPC message

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Create a simulated crash log before the UI Process kills the WebContent or GPU processes.

<rdar://problem/58024593>

[David Kilzer (:ddkilzer)]

Created attachment 385930 [details]
Patch v1

[Alexey Proskuryakov]

How can we be certain that this doesn’t change performance testing results?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #2)
> How can we be certain that this doesn’t change performance testing results?

I may be missing something but why would the UIProcess kill a child process in the context of performance testing?

[Alexey Proskuryakov]

My question would be, why wouldn't it?

[Chris Dumez]

(In reply to Alexey Proskuryakov from comment #4)
> My question would be, why wouldn't it?

Either you are misunderstanding the change or I am. If the UIProcess terminates a child process due to bad IPC, then either the WebContent process has been compromised or there is a bug causing us to send bad IPC (which we would need to fix ASAP since it would cause crashes for users).

There is no valid reason for this to be happening during performance testing. If there are ANY crashes during perf testing then the perf testing results will be bad no matter what.

[Alexey Proskuryakov]

I constantly see these invalid messages on correctness tests, so I'm not sure if they are actually important enough to get fixed immediately. They may be hit in scenarios that are not affecting correctness.

[David Kilzer (:ddkilzer)]

(In reply to Alexey Proskuryakov from comment #6)
> I constantly see these invalid messages on correctness tests, so I'm not
> sure if they are actually important enough to get fixed immediately. They
> may be hit in scenarios that are not affecting correctness.

Can you link to test results where this is happening?

[Alexey Proskuryakov]

See e.g. bug 200191 that hasn't been looked at since July, or bug 194787 that took a month to be fixed. Or any other bug with "Unhandled web process message" in the title.

Also e-mailing you a recent example from internal testers.

[Alexey Proskuryakov]

Comment on attachment 385930 [details]
Patch v1

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

> Source/WTF/wtf/cocoa/CrashReporter.h:34
> +WTF_EXPORT void simulateCrash(pid_t, mach_exception_data_type_t, const char* description);

It is exceptionally opaque that this function does nothing on macOS and Simulator, and that it’s desirable to keep it this way. I think that s a better  way to express this is to only expose the symbol on iOS, and to have ifdefs at call sites.

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:284
> +    WTF::simulateCrash(connection.remoteProcessID(), kExceptionCode, logMessage.utf8().data());

Public WTF symbols are expected to have “using” in the header, so that prefix is unnecessary at call sites.

[David Kilzer (:ddkilzer)]

Comment on attachment 385930 [details]
Patch v1

Will post a new patch with suggested changes from Comment #9.

[Brent Fulgham]

Comment on attachment 385930 [details]
Patch v1

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

>> Source/WTF/wtf/cocoa/CrashReporter.h:34
>> +WTF_EXPORT void simulateCrash(pid_t, mach_exception_data_type_t, const char* description);
> 
> It is exceptionally opaque that this function does nothing on macOS and Simulator, and that it’s desirable to keep it this way. I think that s a better  way to express this is to only expose the symbol on iOS, and to have ifdefs at call sites.

Why don't we want to simulate a crash on macOS? It seems like that would be a useful thing to get data for, wouldn't it?

[David Kilzer (:ddkilzer)]

(In reply to Brent Fulgham from comment #11)
> Comment on attachment 385930 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385930&action=review
> 
> >> Source/WTF/wtf/cocoa/CrashReporter.h:34
> >> +WTF_EXPORT void simulateCrash(pid_t, mach_exception_data_type_t, const char* description);
> > 
> > It is exceptionally opaque that this function does nothing on macOS and Simulator, and that it’s desirable to keep it this way. I think that s a better  way to express this is to only expose the symbol on iOS, and to have ifdefs at call sites.
> 
> Why don't we want to simulate a crash on macOS? It seems like that would be
> a useful thing to get data for, wouldn't it?

macOS doesn't support this type of simulated crash, so we do NSLog() on that platform instead.

[David Kilzer (:ddkilzer)]

Chris Dumez suggested using RELEASE_LOG_FAULT() to log, which has a side-effect of creating a simulated crash (for the UI process).  He didn't think that the stacks from the remote process would be that useful, and this avoids a possible race condition that Alexey pointed out between creating the simulated crash and killing the process (in the case of the WebContent and GPU processes).

[David Kilzer (:ddkilzer)]

Created attachment 387594 [details]
Patch v2

[Chris Dumez]

Comment on attachment 387594 [details]
Patch v2

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

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:276
> +    RELEASE_LOG_FAULT(IPC, "Received an invalid message '%s::%s' from the %s process.", messageReceiverName.toString().data(), messageName.toString().data(), processName().utf8().data());

Don't we need %{public}s ?

> Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:123
> +    virtual String processName() const = 0;

Seems like this could return an ACSIILiteral

[David Kilzer (:ddkilzer)]

Comment on attachment 387594 [details]
Patch v2

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

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp:276
>> +    RELEASE_LOG_FAULT(IPC, "Received an invalid message '%s::%s' from the %s process.", messageReceiverName.toString().data(), messageName.toString().data(), processName().utf8().data());
> 
> Don't we need %{public}s ?

Fixed.

>> Source/WebKit/UIProcess/AuxiliaryProcessProxy.h:123
>> +    virtual String processName() const = 0;
> 
> Seems like this could return an ACSIILiteral

Fixed.

[David Kilzer (:ddkilzer)]

Created attachment 387728 [details]
Patch v3

[Saam Barati]

Comment on attachment 387728 [details]
Patch v3

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

> Source/WebKit/ChangeLog:3
> +        [Cocoa] Create a simulated crash log when the UI Process receives an invalid CoreIPC message

Is this something that happens a lot? Are there known cases?

Simulated crashes have historically hurt us in terms of our ability to perf test. I just want to make sure we're not knowingly hitting this code

[David Kilzer (:ddkilzer)]

(In reply to Saam Barati from comment #18)
> Comment on attachment 387728 [details]
> Patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387728&action=review
> 
> > Source/WebKit/ChangeLog:3
> > +        [Cocoa] Create a simulated crash log when the UI Process receives an invalid CoreIPC message
> 
> Is this something that happens a lot? Are there known cases?
> 
> Simulated crashes have historically hurt us in terms of our ability to perf
> test. I just want to make sure we're not knowingly hitting this code

It should never happen.  This code will only run if a com.apple.WebKit.* process sends an invalid CoreIPC message to the UI Process.  For the WebContent and GPU processes, that will result in killing these processes (which happens without this patch), which means perf testing will Have A Bad Time regardless of this change.

[Chris Dumez]

Comment on attachment 387728 [details]
Patch v3

r=me

[David Kilzer (:ddkilzer)]

Comment on attachment 387728 [details]
Patch v3

All code changes are in WebKit2, so this can't possibly affect mac-debug-wk1 tests (which still haven't run yet).

[WebKit Commit Bot]

Comment on attachment 387728 [details]
Patch v3

Clearing flags on attachment: 387728

Committed r254569: <https://trac.webkit.org/changeset/254569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.