Bug 205368 - [Cocoa] Create a simulated crash log when the UI Process receives an invalid IPC message
Summary: [Cocoa] Create a simulated crash log when the UI Process receives an invalid ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-12-17 16:56 PST by David Kilzer (:ddkilzer)
Modified: 2020-01-15 08:18 PST (History)
15 users (show)

See Also:


Attachments
Patch v1 (29.04 KB, patch)
2019-12-17 17:24 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (10.24 KB, patch)
2020-01-13 17:00 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (10.29 KB, patch)
2020-01-14 16:29 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2019-12-17 16:56:38 PST
Create a simulated crash log before the UI Process kills the WebContent or GPU processes.

<rdar://problem/58024593>
Comment 1 David Kilzer (:ddkilzer) 2019-12-17 17:24:59 PST
Created attachment 385930 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2019-12-17 18:51:06 PST
How can we be certain that this doesn’t change performance testing results?
Comment 3 Chris Dumez 2019-12-17 18:53:56 PST
(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?
Comment 4 Alexey Proskuryakov 2019-12-18 10:02:28 PST
My question would be, why wouldn't it?
Comment 5 Chris Dumez 2019-12-18 10:53:58 PST
(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.
Comment 6 Alexey Proskuryakov 2019-12-18 12:38:42 PST
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.
Comment 7 David Kilzer (:ddkilzer) 2019-12-18 14:16:28 PST
(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?
Comment 8 Alexey Proskuryakov 2019-12-20 08:54:21 PST
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.
Comment 9 Alexey Proskuryakov 2019-12-20 12:48:03 PST
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.
Comment 10 David Kilzer (:ddkilzer) 2020-01-07 10:42:05 PST
Comment on attachment 385930 [details]
Patch v1

Will post a new patch with suggested changes from Comment #9.
Comment 11 Brent Fulgham 2020-01-07 11:41:46 PST
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?
Comment 12 David Kilzer (:ddkilzer) 2020-01-13 14:57:37 PST
(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.
Comment 13 David Kilzer (:ddkilzer) 2020-01-13 16:49:54 PST
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).
Comment 14 David Kilzer (:ddkilzer) 2020-01-13 17:00:22 PST
Created attachment 387594 [details]
Patch v2
Comment 15 Chris Dumez 2020-01-14 08:22:53 PST
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
Comment 16 David Kilzer (:ddkilzer) 2020-01-14 16:25:04 PST
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.
Comment 17 David Kilzer (:ddkilzer) 2020-01-14 16:29:52 PST
Created attachment 387728 [details]
Patch v3
Comment 18 Saam Barati 2020-01-14 16:33:01 PST
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
Comment 19 David Kilzer (:ddkilzer) 2020-01-14 16:39:04 PST
(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.
Comment 20 Chris Dumez 2020-01-14 16:58:07 PST
Comment on attachment 387728 [details]
Patch v3

r=me
Comment 21 David Kilzer (:ddkilzer) 2020-01-15 07:33:19 PST
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).
Comment 22 WebKit Commit Bot 2020-01-15 08:18:22 PST
Comment on attachment 387728 [details]
Patch v3

Clearing flags on attachment: 387728

Committed r254569: <https://trac.webkit.org/changeset/254569>
Comment 23 WebKit Commit Bot 2020-01-15 08:18:25 PST
All reviewed patches have been landed.  Closing bug.