Bug 205368

Created attachment 385930 [details] Patch v1

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

(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?

My question would be, why wouldn't it?

(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.

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.

(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?

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 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 on attachment 385930 [details] Patch v1 Will post a new patch with suggested changes from Comment #9.

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?

(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.

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).

Created attachment 387594 [details] Patch v2

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 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.

Created attachment 387728 [details] Patch v3

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

(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 on attachment 387728 [details] Patch v3 r=me

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 on attachment 387728 [details] Patch v3 Clearing flags on attachment: 387728 Committed r254569: <https://trac.webkit.org/changeset/254569>

All reviewed patches have been landed. Closing bug.