Bug 162754 - More logging to diagnose "WebKit encountered an internal error" messages
Summary: More logging to diagnose "WebKit encountered an internal error" messages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Rollin
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-29 14:32 PDT by Keith Rollin
Modified: 2016-10-03 17:35 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Rollin 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>
Comment 1 Keith Rollin 2016-09-30 15:01:08 PDT
Created attachment 290390 [details]
Patch
Comment 2 Antti Koivisto 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?
Comment 3 Keith Rollin 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.
Comment 4 Keith Rollin 2016-10-03 14:46:10 PDT
Created attachment 290525 [details]
Patch
Comment 5 WebKit Commit Bot 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>
Comment 6 WebKit Commit Bot 2016-10-03 15:20:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Ryan Haddad 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>
Comment 8 Ryan Haddad 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
Comment 9 Keith Rollin 2016-10-03 16:07:39 PDT
I've updated the patch to use the previous code on older systems.
Comment 10 Keith Rollin 2016-10-03 16:10:55 PDT
Created attachment 290533 [details]
Patch
Comment 11 WebKit Commit Bot 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>
Comment 12 WebKit Commit Bot 2016-10-03 17:35:13 PDT
All reviewed patches have been landed.  Closing bug.