Bug 157274 – Add the ability to accumulate logs for specific logging channels to help diagnose test timeouts

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Brady Eidson]

Add the ability to accumulate logs for specific logging channels to help diagnose test timeouts

This is the current strategy going forward to explore https://bugs.webkit.org/show_bug.cgi?id=154968

[Brady Eidson]

This will add the WTF code as well as implement it in DRT.

WK2 (including getting it to all the processes) will come shortly after.

[Brady Eidson]

Created attachment 277929 [details]
Patch

[Brady Eidson]

Okay, apparently my attempt at exporting some WebCore symbols fails in release builds. Dang.
Fixing.

[Brady Eidson]

Created attachment 277935 [details]
Patch

[Brady Eidson]

Relying on EWS for this because it will be faster than me building release locally.

[Brady Eidson]

The GTK build failure:

Last 500 characters of output:
./Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: error: 'initializeLoggingChannelsIfNecessary' is not a member of 'WebCore'
../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: note: suggested alternative:
../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:106:6: note:   'WebCoreTestSupport::initializeLoggingChannelsIfNecessary'

Except... that's obviously bull, because initializeLoggingChannelsIfNecessary is clearly a member of WebCore in the included header "Logging.h"

[Brady Eidson]

(In reply to comment #6)
> The GTK build failure:
> 
> Last 500 characters of output:
> ./Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: error:
> 'initializeLoggingChannelsIfNecessary' is not a member of 'WebCore'
> ../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:108:5: note:
> suggested alternative:
> ../../Source/WebCore/testing/js/WebCoreTestSupport.cpp:106:6: note:  
> 'WebCoreTestSupport::initializeLoggingChannelsIfNecessary'
> 
> Except... that's obviously bull, because
> initializeLoggingChannelsIfNecessary is clearly a member of WebCore in the
> included header "Logging.h"

I get the same error on Mac in a release build.
Odd.

[Brady Eidson]

It's a compiled ifdef for logging. Of course!

[Brady Eidson]

Created attachment 277939 [details]
Patch

[Alex Christensen]

Comment on attachment 277939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277939&action=review

> Source/WTF/wtf/Assertions.cpp:593
>  } // extern "C"
> +
> +namespace WTF {

Everything else in this file has the WTF prefix on its name.  Is there a reason you didn't want to put this in its own file?  LoggingAccumulator.cpp

[Brady Eidson]

(In reply to comment #10)
> Comment on attachment 277939 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277939&action=review
> 
> > Source/WTF/wtf/Assertions.cpp:593
> >  } // extern "C"
> > +
> > +namespace WTF {
> 
> Everything else in this file has the WTF prefix on its name.  Is there a
> reason you didn't want to put this in its own file?  LoggingAccumulator.cpp

The only reason everything in this file is prefixed with WTF and not in the WTF namespace is because Assertions.h has to be includible from C or Obj-C code.

LoggingAccumulator.h exposes two convenience methods for the mechanism with C++ concepts, but there's no need for a .cpp. In fact adding a .cpp would separate the implementation details from where they are actively used, inside of Assertions.cpp.

[Brady Eidson]

As I work on the WK2 part of this, I realize, this approach really only works for WK1.

In WK2 we have to message around to each process to enable the accumulation, and then message to each process for the accumulated logs.

The result is we have multiple "chunks" of logs, such as one chunk from the web process and one from the database process.

Unlike how the logging information comes to stderr, those chunks are not intertwined.

I'm going to go ahead with this patch as-is for WK1, as there will still be benefit from doing so, but revisit WK2 later.

[Brady Eidson]

http://trac.webkit.org/changeset/200346

[Fujii Hironori]

If a test case calls testRunner.accummulateLogsForChannel("IndexedDB"),
all subsequent IndexedDB tests dumps the log.
This is bloating DumpRenderTree log because DumpRenderTree is run with --debug-rwt-logging switch.
For example,
https://build.webkit.org/builders/Apple-Catalina-Debug-WK1-Tests/builds/70/steps/layout-test/logs/stdio

Brady, do you still need this logging function?
Can I remove this?
Or, should I add a function to turn off WTFLogChannelOnWithAccumulation flag and call it after test execution like getAndResetAccumulatedLogs?

[Fujii Hironori]

Filed: Bug 203024 – DumpRenderTree should clear WTFLogChannelState::OnWithAccumulation state set by testRunner.accummulateLogsForChannel