More logging to diagnose "WebKit encountered an internal error" messages

[Keith Rollin]

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>

[Keith Rollin]

Created attachment 290390 [details]
Patch

[Antti Koivisto]

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]

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

Created attachment 290525 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290525 [details]
Patch

Clearing flags on attachment: 290525

Committed r206754: <http://trac.webkit.org/changeset/206754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

Reverted r206754 for reason:

This change broke the El Capitan and Yosemite debug builds.

Committed r206757: <http://trac.webkit.org/changeset/206757>

[Ryan Haddad]

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

I've updated the patch to use the previous code on older systems.

[Keith Rollin]

Created attachment 290533 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 290533 [details]
Patch

Clearing flags on attachment: 290533

Committed r206762: <http://trac.webkit.org/changeset/206762>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.