WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
162754
More logging to diagnose "WebKit encountered an internal error" messages
https://bugs.webkit.org/show_bug.cgi?id=162754
Summary
More logging to diagnose "WebKit encountered an internal error" messages
Keith Rollin
Reported
2016-09-29 14:32:25 PDT
We’re getting a number of reports about the “WebKit encountered an internal error" message, and it would be good if we had logging that captured information that explicitly told us why this error message appeared (if that doesn’t already exist). <
rdar://problem/28460265
>
Attachments
Patch
(9.28 KB, patch)
2016-09-30 15:01 PDT
,
Keith Rollin
no flags
Details
Formatted Diff
Diff
Patch
(9.27 KB, patch)
2016-10-03 14:46 PDT
,
Keith Rollin
no flags
Details
Formatted Diff
Diff
Patch
(9.50 KB, patch)
2016-10-03 16:10 PDT
,
Keith Rollin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Keith Rollin
Comment 1
2016-09-30 15:01:08 PDT
Created
attachment 290390
[details]
Patch
Antti Koivisto
Comment 2
2016-10-03 06:00:58 PDT
Comment on
attachment 290390
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=290390&action=review
> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110 > LOG_ERROR("Failed to allocate mach_vm_allocate shared memory (%zu bytes). %s (%x)", size, mach_error_string(kr), kr); > + RELEASE_LOG_ERROR(VirtualMemory, "%p - SharedMemory::allocate: Failed to allocate mach_vm_allocate shared memory (%zu bytes). %{public}s (%x)", nullptr, size, mach_error_string(kr), kr);
Why do we need both log messages?
> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:144 > LOG_ERROR("Failed to create a mach port for shared memory. %s (%x)", mach_error_string(kr), kr); > + RELEASE_LOG_ERROR(VirtualMemory, "%p - SharedMemory::makeMemoryEntry: Failed to create a mach port for shared memory. %{public}s (%x)", nullptr, mach_error_string(kr), kr);
Here too?
> Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:200 > + ASSERT(kr == KERN_SUCCESS);
Why not just ASSERT_NOT_REACHED?
Keith Rollin
Comment 3
2016-10-03 12:31:13 PDT
(In reply to
comment #2
)
> Comment on
attachment 290390
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=290390&action=review
> > > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:110 > > LOG_ERROR("Failed to allocate mach_vm_allocate shared memory (%zu bytes). %s (%x)", size, mach_error_string(kr), kr); > > + RELEASE_LOG_ERROR(VirtualMemory, "%p - SharedMemory::allocate: Failed to allocate mach_vm_allocate shared memory (%zu bytes). %{public}s (%x)", nullptr, size, mach_error_string(kr), kr); > > Why do we need both log messages? > > > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:144 > > LOG_ERROR("Failed to create a mach port for shared memory. %s (%x)", mach_error_string(kr), kr); > > + RELEASE_LOG_ERROR(VirtualMemory, "%p - SharedMemory::makeMemoryEntry: Failed to create a mach port for shared memory. %{public}s (%x)", nullptr, mach_error_string(kr), kr); > > Here too?
The idea is to preserve previous logging that people may be using for pre-existing workflows. The RELEASE_LOG macros always log and will send their output to the Console app. The other logging shows up in syslog, the terminal, or Xcode console. It's also conditionally enabled. I didn't want to upset the workflows of people who rely on any of that. There are other instances of double-logging in: NetworkResourceLoader.cpp, WebLoaderStrategy.cpp, WebResourceLoader.cpp After having a chat with Chris about this double-logging, he felt that it would be OK to take the old logging out. It's not generally used, we're transitioning to the os_log-based logging, almost everyone is now living on a system that supports os_log, etc. So I'll take it out of this patch and will file a bug to cover the other cases.
> > Source/WebKit2/Platform/mac/SharedMemoryMac.cpp:200 > > + ASSERT(kr == KERN_SUCCESS); > > Why not just ASSERT_NOT_REACHED?
I considered ASSERT(false) (which seems equivalent to ASSERT_NOT_REACHED), but I felt that it was not as good as ASSERT(kr == KERN_SUCCESS). The latter will result in a message that says "ASSERTION FAILED: kr == KERN_SUCCESS". This message includes more information on what failed and so seems better. But ASSERT_NOT_REACHED seems more consistent with existing usage, so I'll switch to that.
Keith Rollin
Comment 4
2016-10-03 14:46:10 PDT
Created
attachment 290525
[details]
Patch
WebKit Commit Bot
Comment 5
2016-10-03 15:20:51 PDT
Comment on
attachment 290525
[details]
Patch Clearing flags on attachment: 290525 Committed
r206754
: <
http://trac.webkit.org/changeset/206754
>
WebKit Commit Bot
Comment 6
2016-10-03 15:20:55 PDT
All reviewed patches have been landed. Closing bug.
Ryan Haddad
Comment 7
2016-10-03 15:56:12 PDT
Reverted
r206754
for reason: This change broke the El Capitan and Yosemite debug builds. Committed
r206757
: <
http://trac.webkit.org/changeset/206757
>
Ryan Haddad
Comment 8
2016-10-03 15:59:13 PDT
(In reply to
comment #7
)
> Reverted
r206754
for reason: > > This change broke the El Capitan and Yosemite debug builds. > > Committed
r206757
: <
http://trac.webkit.org/changeset/206757
>
EWS seems to have caught this on the first patch. Here is a link to failure:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20%28Build%29/builds/19189
Keith Rollin
Comment 9
2016-10-03 16:07:39 PDT
I've updated the patch to use the previous code on older systems.
Keith Rollin
Comment 10
2016-10-03 16:10:55 PDT
Created
attachment 290533
[details]
Patch
WebKit Commit Bot
Comment 11
2016-10-03 17:35:08 PDT
Comment on
attachment 290533
[details]
Patch Clearing flags on attachment: 290533 Committed
r206762
: <
http://trac.webkit.org/changeset/206762
>
WebKit Commit Bot
Comment 12
2016-10-03 17:35:13 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug