Fault logs should not be skipped #15337
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pvollan
added
the
WebKit Misc.
|
EWS run on previous version of this PR (hash ca29d61)
|
cdumez
reviewed
Jun 27, 2023
| @@ -723,14 +723,30 @@ static void registerLogHook() | |||
| if (msg->buffer_sz > 1024) | |||
| return; | |||
|
|
|||
| // Skip faults and debug logs in non-internal builds. | |||
| if (type == OS_LOG_TYPE_FAULT || type == OS_LOG_TYPE_DEBUG) | |||
There was a problem hiding this comment.
Oh wow, this is an important fix lol
|
|
||
| auto connectionID = WebProcess::singleton().networkProcessConnectionID(); | ||
| if (connectionID) | ||
| IPC::Connection::send(connectionID, Messages::NetworkConnectionToWebProcess::LogOnBehalfOfWebContent(logChannel.bytesInludingNullTerminator(), logCategory.bytesInludingNullTerminator(), logString, OS_LOG_TYPE_ERROR, getpid()), 0); |
There was a problem hiding this comment.
I suggest with still use the dispatch queue but use WorkQueue::dispatchWithQoS() to make the message priority higher. Otherwise, we can end up with out-of-order logging which would be unfortunate.
There was a problem hiding this comment.
We should also pass the QoS to IPC::Connection::send() for FAULT messages (already supported).
There was a problem hiding this comment.
Ah, good point, will fix!
Thanks for reviewing!
pvollan
force-pushed
the
eng/Fault-logs-should-not-be-skipped
branch
from
ca29d61
to
eba8117
Compare
|
EWS run on previous version of this PR (hash eba8117)
|
pvollan
force-pushed
the
eng/Fault-logs-should-not-be-skipped
branch
from
eba8117
to
8528226
Compare
|
EWS run on previous version of this PR (hash 8528226)
|
cdumez
reviewed
Jun 27, 2023
|
|
||
| // Send fault logs with high priority. If the WebContent process is terminated, we might not be able to send the log in time. | ||
| if (type == OS_LOG_TYPE_FAULT) { | ||
| type = type == OS_LOG_TYPE_ERROR; |
There was a problem hiding this comment.
That was a typo, fixed in latest patch :)
pvollan
force-pushed
the
eng/Fault-logs-should-not-be-skipped
branch
from
8528226
to
ddaee3f
Compare
|
EWS run on current version of this PR (hash ddaee3f)
|
https://bugs.webkit.org/show_bug.cgi?id=258577 rdar://111393438 Reviewed by Chris Dumez. Fault logs should not be skipped in the WebContent process, but sent asap to the Networking process. * Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm: (WebKit::registerLogHook): Canonical link: https://commits.webkit.org/265558@main
webkit-commit-queue
force-pushed
the
eng/Fault-logs-should-not-be-skipped
branch
from
ddaee3f
to
4e7db72
Compare
|
Committed 265558@main (4e7db72): https://commits.webkit.org/265558@main Reviewed commits have been landed. Closing PR #15337 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
4e7db72
ddaee3f