Bug 183744

Summary: Add some tests for lldb_webkit.py
Product: WebKit Reporter: Daniel Bates <dbates>
Component: Tools / TestsAssignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ap, cdumez, commit-queue, ews-watchlist, glenn, jer.noble, lforschler, msaboff, realdawei, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: Mac   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=185801
Bug Depends on: 186240    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch and unit tests
none
[Patch] Instrument unti tests to dump timing info
none
Patch and unit tests that run fast
none
Patch for landing
none
For EWS
none
For EWS
none
For EWS
none
For EWS
none
For EWS
none
For EWS
none
For EWS
none
For EWS
none
Patch for landing none

Description Daniel Bates 2018-03-19 10:42:18 PDT
Tools/lldb/lldb_webkit.py adds various data formatters to LLDB to pretty-print common WTF and WebCore data structures (e.g. WTF::String) as well as exposes other helpful debugging functions in an LLDB session. The script lldb_webkit.py is fragile and can break when the memory layout of these pretty-printable data structures change among other code changes. People that make changes to these data structures may not be using lldb_webkit.py, be aware of its existence, or may not notice the breakage they cause to lldb_webkit.py. We should add tests for lldb_webkit.py to help identify changes that may break it as well as allow developers extending lldb_webkit.py to write correctness tests.
Comment 1 Daniel Bates 2018-03-19 10:57:09 PDT
Created attachment 336054 [details]
Patch
Comment 2 Daniel Bates 2018-03-19 10:58:36 PDT
Created attachment 336055 [details]
Patch
Comment 3 EWS Watchlist 2018-03-19 10:59:37 PDT
Attachment 336055 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Daniel Bates 2018-03-19 11:08:47 PDT
Created attachment 336057 [details]
Patch
Comment 5 EWS Watchlist 2018-03-19 11:12:14 PDT
Attachment 336057 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Daniel Bates 2018-03-19 11:36:11 PDT
I did not mark this patch for review since it add ~15 seconds to building and testing :( On my 2.9 GHz Intel Core i7 MacBook Pro (15-inch, 2017) building lldbWebKitTester takes ~6 seconds and running the LLDB tests take ~9 seconds; => test-webkitpy (without --all) now takes ~16 seconds to run. The reason the LLDB tests take so long is because they run serially. If I do not run them serially then I see error output that has the form:

objc[76794]: +[__MDQuery initialize] may have been in progress in another thread when fork() was called. We cannot safely call it or ignore it in the fork() child process. Crashing instead.

Without the LLDB tests, test-webkitpy (without --all) takes ~7 seconds to run.

I am open to ideas/suggestions on how to decreases the build and test time to make it viable to run these tests on the bots by default.
Comment 7 Daniel Bates 2018-05-21 12:16:48 PDT
Created attachment 340870 [details]
Patch

Updated patch that runs the lldb tests in a new Python process to avoid the +[__MDQuery initialize] issue. This allows us to run the lldb tests in parallel. Although this approach speeds up the tests ~20%, running "test-webkitpy" takes a little more than double the amount of time it takes today: ~12.978666666666667s with the lldb tests vs ~7s without the lldb tests. I am open to ideas/suggestions on how we can further speed the lldb tests up.
Comment 8 Daniel Bates 2018-05-21 12:17:38 PDT
(In reply to Daniel Bates from comment #7)
> [...] Although this approach speeds up the tests ~20%, running
> "test-webkitpy" takes a little more than double the amount of time it takes
*a little less than double
Comment 9 EWS Watchlist 2018-05-21 12:19:22 PDT
Attachment 340870 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Alexey Proskuryakov 2018-05-21 12:51:36 PDT
Do you know what makes these lldb tests take so much time? Is it launching lldb many times over?
Comment 11 Daniel Bates 2018-05-21 15:53:15 PDT
(In reply to Alexey Proskuryakov from comment #10)
> Do you know what makes these lldb tests take so much time? Is it launching
> lldb many times over?

Yes, it is launching lldb and the target app many times over.
Comment 12 Daniel Bates 2018-05-21 15:58:27 PDT
Created attachment 340913 [details]
Patch and unit tests

Add missing file stateless_process_runner.py.
Comment 13 EWS Watchlist 2018-05-21 16:01:04 PDT
Attachment 340913 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Alexey Proskuryakov 2018-05-21 16:04:23 PDT
Would it be reasonable to run the tests in a single lldb instance?
Comment 15 Daniel Bates 2018-05-22 16:24:55 PDT
(In reply to Alexey Proskuryakov from comment #14)
> Would it be reasonable to run the tests in a single lldb instance?

This is what we were doing when we ran them serially in earlier iterations of this patch (e.g. attachment #336057 [details]).
Comment 16 Alexey Proskuryakov 2018-05-23 09:16:58 PDT
That brings me back to question 10 - what's making the tests so slow? If they are slow regardless of whether they start lldb many times, then it's probably something else.
Comment 17 Daniel Bates 2018-05-23 11:21:13 PDT
Created attachment 341110 [details]
[Patch] Instrument unti tests to dump timing info

Apply this diff on top of the patch (attachment #340913 [details]). Ensure you debug built lldbWebKitTester. (You can do this by running ` make debug -C Tools/lldb/lldbWebKitTester` from the top-level WebKit checkout). Then run test-webkitpy.

On my MacBook Pro (Retina, 15-inch, Early 2013) running macOS High Sierra 10.13.3 (17D47) here are my results:

[[
$ Tools/Scripts/test-webkitpy -p -v
Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

[1/13] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif passed
[2/13] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else passed
[3/13] webkit.messages_unittest.ParsingTest.test_receiver passed     
[4/13] webkit.messages_unittest.HeaderTest.test_receiver_headers passed
[5/13] webkit.messages_unittest.ReceiverImplementationTest.test_receiver_implementations passed
[5/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_16bit_string
	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took 0.197887420654ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took 349.189043045ms to run WTFStringImpl_SummaryProvider
[6/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_16bit_string passed
[6/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_16bit_string
	[serial_test_WTFString_SummaryProvider_16bit_string] Took 0.0581741333008ms to find variable
	[serial_test_WTFString_SummaryProvider_16bit_string] Took 6.78706169128ms to run WTFString_SummaryProvider
[7/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_16bit_string passed
[7/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_null_string
	[serial_test_WTFStringImpl_SummaryProvider_null_string] Took 0.0638961791992ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_null_string] Took 1.42097473145ms to run WTFStringImpl_SummaryProvider
[8/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_null_string passed
[8/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_empty_string
	[serial_test_WTFString_SummaryProvider_empty_string] Took 0.0629425048828ms to find variable
	[serial_test_WTFString_SummaryProvider_empty_string] Took 1.49893760681ms to run WTFString_SummaryProvider
[9/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_empty_string passed
[9/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_null_string
	[serial_test_WTFString_SummaryProvider_null_string] Took 0.0629425048828ms to find variable
	[serial_test_WTFString_SummaryProvider_null_string] Took 1.52897834778ms to run WTFString_SummaryProvider
[10/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_null_string passed
[10/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_empty_string
	[serial_test_WTFStringImpl_SummaryProvider_empty_string] Took 0.0650882720947ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_empty_string] Took 1.46508216858ms to run WTFStringImpl_SummaryProvider
[11/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_empty_string passed
[11/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_8bit_string
	[serial_test_WTFString_SummaryProvider_8bit_string] Took 0.0619888305664ms to find variable
	[serial_test_WTFString_SummaryProvider_8bit_string] Took 2.22396850586ms to run WTFString_SummaryProvider
[12/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFString_SummaryProvider_8bit_string passed
[12/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_8bit_string
	[serial_test_WTFStringImpl_SummaryProvider_8bit_string] Took 0.0631809234619ms to find variable
	[serial_test_WTFStringImpl_SummaryProvider_8bit_string] Took 1.48797035217ms to run WTFStringImpl_SummaryProvider
[13/13] lldb_webkit_unittest.TestSummaryProviders.serial_test_WTFStringImpl_SummaryProvider_8bit_string passed
Ran 13 tests in 9.402s

OK
]]
Comment 18 Daniel Bates 2018-05-23 11:24:07 PDT
(In reply to Daniel Bates from comment #17)
> lldb_webkit_unittest.TestSummaryProviders.
> serial_test_WTFStringImpl_SummaryProvider_16bit_string
> 	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took
> 0.197887420654ms to find variable
> 	[serial_test_WTFStringImpl_SummaryProvider_16bit_string] Took
> 349.189043045ms to run WTFStringImpl_SummaryProvider

For some reason the first time we call code in Tools/lldb/lldb_webkit.py it takes ~349.189043045ms. That is, 349.189043045ms is not specific to the test serial_test_WTFStringImpl_SummaryProvider_16bit_string.
Comment 19 Daniel Bates 2018-05-23 11:32:10 PDT
Here is the output when parallelizing the lldb unit tests:

[[
$ Tools/Scripts/test-webkitpy -p -v
Skipping QueueStatusServer tests; the Google AppEngine Python SDK is not installed.
Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really, slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

[1/5] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_elif passed
[2/5] webkit.messages_unittest.UnsupportedPrecompilerDirectiveTest.test_error_at_else passed
[3/5] webkit.messages_unittest.ParsingTest.test_receiver passed     
[4/5] webkit.messages_unittest.HeaderTest.test_receiver_headers passed
[5/5] webkit.messages_unittest.ReceiverImplementationTest.test_receiver_implementations passed
[5/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_null_string (+7)
	[stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string] Took 0.221014022827ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_8bit_string] Took 0.229120254517ms to find variable

	[stateless_process_test_WTFStringImpl_SummaryProvider_empty_string] Took 0.221967697144ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_null_string] Took 0.21505355835ms to find variable

	[stateless_process_test_WTFString_SummaryProvider_16bit_string] Took 0.209808349609ms to find variable


	[stateless_process_test_WTFStringImpl_SummaryProvider_null_string] Took 0.21505355835ms to find variable	[stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string] Took 0.197172164917ms to find variable


	[stateless_process_test_WTFString_SummaryProvider_empty_string] Took 0.226020812988ms to find variable
	[stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string] Took 564.28194046ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_8bit_string] Took 566.425085068ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_null_string] Took 566.066980362ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_empty_string] Took 578.790903091ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string] Took 570.056915283ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_16bit_string] Took 577.664136887ms to run WTFString_SummaryProvider
	[stateless_process_test_WTFStringImpl_SummaryProvider_null_string] Took 578.140974045ms to run WTFStringImpl_SummaryProvider
	[stateless_process_test_WTFString_SummaryProvider_empty_string] Took 577.935218811ms to run WTFString_SummaryProvider
[6/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_16bit_string passed
[7/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_8bit_string passed
[8/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_null_string passed
[9/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_null_string passed
[10/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_8bit_string passed
[11/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_16bit_string passed
[12/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFStringImpl_SummaryProvider_empty_string passed
[13/5] lldb_webkit_unittest.TestSummaryProviders.stateless_process_test_WTFString_SummaryProvider_empty_string passed
Ran 13 tests in 5.345s

OK
]]
Comment 20 Daniel Bates 2018-05-23 16:53:22 PDT
Comment on attachment 340913 [details]
Patch and unit tests

Clearing review flag for further investigation.
Comment 21 Daniel Bates 2018-05-24 11:03:51 PDT
As it turns out the way we support test parallelism in test-webkitpy does not permit using class and module fixtures (i.e. using setUpClass()/setUpModule()). This is because we achieve parallelism by breaking down the structure of test classes into a listing of test methods and then have each worker run exactly one test method at a time. This reorganization of test methods is non-standard and explicitly not supported by the Python 2.7 unittest module: "Shared fixtures are not intended to work with suites with non-standard ordering" [1]. As a result of our non-standard ordering (one test method per TestSuite), we invoke setUpClass()/setUpModule() for each test method. 

[1] <https://docs.python.org/2/library/unittest.html#class-and-module-fixtures>
Comment 22 Daniel Bates 2018-05-24 11:06:43 PDT
Created attachment 341210 [details]
Patch and unit tests that run fast

Cache the LLDB debug session to avoid creating the target and launching the process each time we run a test method. Using this patch, it takes ~2.5 seconds to run just the LLDB tests included in the patch and ~9.5 seconds to run all tests with the default invocatino of "test-webkitpy"
Comment 23 EWS Watchlist 2018-05-24 11:07:56 PDT
Attachment 341210 [details] did not pass style-queue:


ERROR: Tools/lldb/lldbWebKitTester/main.cpp:26:  Streams are highly discouraged.  [readability/streams] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Alexey Proskuryakov 2018-05-24 11:34:52 PDT
Comment on attachment 341210 [details]
Patch and unit tests that run fast

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

> Tools/Scripts/webkitpy/common/system/systemhost.py:55
> +        return self.filesystem.abspath(self.filesystem.join(xcode_developer_directory, '..', 'SharedFrameworks', 'LLDB.framework', 'Versions', 'A', 'Resources', 'Python'))

Is it documented anywhere that LLDB.framework can be used from this location? https://lldb.llvm.org says that the framework is exposed as a public on on macOS, but I don't see it in /S/L/F. And https://lldb.llvm.org/python-reference.html says that it's in /S/L/PF, which doesn't seem correct either.

> Tools/Scripts/webkitpy/test/main.py:60
> +        _log.info("Skipping lldb_webkit tests; could not find '{}'.".format(lldb_webkit_tester_executable))

This seems fairly dangerous, given that only "make" builds build the tool. So one may run the tests and not realize that they are not testing lldb.

Do you intend to add it to all other build scenarios?

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:1
> +// Copyright (C) 2015-2018 Apple Inc. All rights reserved.

It may be useful to compare config files to some project that's more actively maintained (perhaps WebCore). I don't see anything that is an obvious problem, but there aren't a lot of xcconfig problems that I can readily notice.

> Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:68
> +HEADER_SEARCH_PATHS = ${BUILT_PRODUCTS_DIR}/usr/local/include;

What are we including from here?

> Tools/lldb/lldbWebKitTester/main.cpp:28
> +#include <iostream>
> +#include <wtf/text/StringBuilder.h>
> +#include <wtf/text/WTFString.h>

This is an unusual mix of headers. Do we have to use iostream here?

> Tools/lldb/lldbWebKitTester/main.cpp:33
> +// From Tools/TestWebKitAPI/WTFStringUtilities.h

I'm not sure what useful information this line adds. Is it a FIXME: Find a way to share implementation with TestWebKitAPI?

> Tools/lldb/lldb_webkit_unittest.py:2
> +# -*- coding: utf-8 -*-

I see that we have this line in a few source files, but that's rare, and last it was discussed on webkit-dev, I think that the consensus was that we don't add editor specific markup of any kind.

> Tools/lldb/lldb_webkit_unittest.py:85
> +        cls.sbProcess.Kill()

Is it not a child process that will be killed automatically?

> Tools/lldb/lldb_webkit_unittest.py:86
> +        cls.sbTarget.DeleteAllBreakpoints()

Given that this is only called on exit, not sure what benefit this provides.
Comment 25 Daniel Bates 2018-05-25 21:55:32 PDT
(In reply to Alexey Proskuryakov from comment #24)
> Comment on attachment 341210 [details]
> Patch and unit tests that run fast
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341210&action=review
> 
> > Tools/Scripts/webkitpy/common/system/systemhost.py:55
> > +        return self.filesystem.abspath(self.filesystem.join(xcode_developer_directory, '..', 'SharedFrameworks', 'LLDB.framework', 'Versions', 'A', 'Resources', 'Python'))
> 
> Is it documented anywhere that LLDB.framework can be used from this
> location? https://lldb.llvm.org says that the framework is exposed as a
> public on on macOS, but I don't see it in /S/L/F. And
> https://lldb.llvm.org/python-reference.html says that it's in /S/L/PF, which
> doesn't seem correct either.
> 

Modified the function that contained this code to now query lldb for the path to the directory that contains lldb.py:

[[
    def path_to_lldb_python_directory(self):
        if not self.platform.is_mac():
            return ''
        return self.executive.run_command(['xcrun', 'lldb', '--python-path'], return_stderr=False).rstrip()
]]


> > Tools/Scripts/webkitpy/test/main.py:60
> > +        _log.info("Skipping lldb_webkit tests; could not find '{}'.".format(lldb_webkit_tester_executable))
> 
> This seems fairly dangerous, given that only "make" builds build the tool.
> So one may run the tests and not realize that they are not testing lldb.
> 
> Do you intend to add it to all other build scenarios?
> 

Will add logic to check if lldbWebKitTester was built for Release or Debug. If neither exists then will build lldbWebKitTester for Debug.

> > Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:1
> > +// Copyright (C) 2015-2018 Apple Inc. All rights reserved.
> 
> It may be useful to compare config files to some project that's more
> actively maintained (perhaps WebCore). I don't see anything that is an
> obvious problem, but there aren't a lot of xcconfig problems that I can
> readily notice.
> 

Will update the xcconfig files in this patch  using WebCore's xccconfig files as a template.

> > Tools/lldb/lldbWebKitTester/Configurations/Base.xcconfig:68
> > +HEADER_SEARCH_PATHS = ${BUILT_PRODUCTS_DIR}/usr/local/include;
> 
> What are we including from here?
> 

Nothing. Will remove.

> > Tools/lldb/lldbWebKitTester/main.cpp:28
> > +#include <iostream>
> > +#include <wtf/text/StringBuilder.h>
> > +#include <wtf/text/WTFString.h>
> 
> This is an unusual mix of headers. Do we have to use iostream here?
> 

No, we do not need to use iostream. Will switch to using stdio.h.

> > Tools/lldb/lldbWebKitTester/main.cpp:33
> > +// From Tools/TestWebKitAPI/WTFStringUtilities.h
> 
> I'm not sure what useful information this line adds. Is it a FIXME: Find a
> way to share implementation with TestWebKitAPI?
> 

Will remove this comment.

> > Tools/lldb/lldb_webkit_unittest.py:2
> > +# -*- coding: utf-8 -*-
> 
> I see that we have this line in a few source files, but that's rare, and
> last it was discussed on webkit-dev, I think that the consensus was that we
> don't add editor specific markup of any kind.
> 

This line is required to tell Python that the source is UTF-8 encoded so that string literals with UTF-8 encoded characters are interpreted correctly. Otherwise, Python interprets source files as ASCII. See <https://www.python.org/dev/peps/pep-0263/> for more details. 

> > Tools/lldb/lldb_webkit_unittest.py:85
> > +        cls.sbProcess.Kill()
> 
> Is it not a child process that will be killed automatically?
> 

I am going to leave this as-is.

It does not seem like good programming practice to depend on program termination/the OS to clean up resources, including child processes, when we can clean them up earlier in the program lifecycle. The lldb_webkit tests are not the only serial tests that we have. I do not see the need to keep the child process around longer than necessary.

On another note, I hope that one day we can reconcile how test-webkitpy runs unit tests such that we can use setUpClass(). When we reconcile this issue then the code in LLDBDebugSession.setup() and LLDBDebugSession.tearDown() can be moved as-is to setUpClass() and tearDownClass(), respectively.

> > Tools/lldb/lldb_webkit_unittest.py:86
> > +        cls.sbTarget.DeleteAllBreakpoints()
> 
> Given that this is only called on exit, not sure what benefit this provides.

Will remove.
Comment 26 Daniel Bates 2018-05-25 21:59:42 PDT
Created attachment 341388 [details]
Patch for landing
Comment 27 Daniel Bates 2018-05-25 22:01:09 PDT
Comment on attachment 341210 [details]
Patch and unit tests that run fast

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

> Tools/lldb/lldb_webkit_unittest.py:65
> +        cls.sbTarget = cls.sbDebugger.CreateTargetWithFileAndArch(str(cls.lldbWebKitTesterExecutable), lldb.LLDB_ARCH_DEFAULT)

Hit a bug with CreateTargetWithFileAndArch(). Switched to CreateTarget(). See <rdar://problem/39960149> for more details.
Comment 28 Daniel Bates 2018-05-31 14:16:10 PDT
Created attachment 341691 [details]
For EWS
Comment 29 Daniel Bates 2018-05-31 17:15:25 PDT
Created attachment 341712 [details]
For EWS
Comment 30 Daniel Bates 2018-05-31 17:23:41 PDT
Created attachment 341714 [details]
For EWS
Comment 31 Daniel Bates 2018-05-31 17:32:40 PDT
Created attachment 341716 [details]
For EWS
Comment 32 Daniel Bates 2018-05-31 21:08:54 PDT
Created attachment 341731 [details]
For EWS
Comment 33 Daniel Bates 2018-05-31 21:29:23 PDT
Created attachment 341733 [details]
For EWS
Comment 34 Daniel Bates 2018-06-01 15:08:28 PDT
Created attachment 341791 [details]
For EWS
Comment 35 Daniel Bates 2018-06-01 15:18:19 PDT
Created attachment 341794 [details]
For EWS
Comment 36 Daniel Bates 2018-06-01 15:54:40 PDT
Committed r232421: <https://trac.webkit.org/changeset/232421>
Comment 37 Radar WebKit Bug Importer 2018-06-01 16:08:03 PDT
<rdar://problem/40737972>
Comment 38 WebKit Commit Bot 2018-06-02 15:44:20 PDT
Re-opened since this is blocked by bug 186240
Comment 39 Simon Fraser (smfr) 2018-06-27 20:40:07 PDT
I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to link.
Comment 40 Daniel Bates 2018-06-27 20:48:08 PDT
(In reply to Simon Fraser (smfr) from comment #39)
> I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to
> link.

Are you able to share what linker errors or insight led you to add this dependency?
Comment 41 Daniel Bates 2018-06-27 20:54:35 PDT
(In reply to Daniel Bates from comment #40)
> (In reply to Simon Fraser (smfr) from comment #39)
> > I had to add -framework Security to OTHER_LDFLAGS to get lldbWebKitTester to
> > link.
> 
> Are you able to share what linker errors or insight led you to add this
> dependency?

Never mind.

$ Tools/Scripts/build-lldbwebkittester --debug
...
Ld /Volumes/Data/WebKitDev/OpenSource/WebKitBuild/Debug/lldbWebKitTester normal x86_64
...
Undefined symbols for architecture x86_64:
  "_SecTaskCopyValueForEntitlement", referenced from:
      WTF::processHasEntitlement(char const*) in libWTF.a(Entitlements.o)
  "_SecTaskCreateFromSelf", referenced from:
      WTF::processHasEntitlement(char const*) in libWTF.a(Entitlements.o)
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)
Comment 42 Daniel Bates 2018-06-27 21:16:08 PDT
Comment on attachment 341794 [details]
For EWS

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

> Tools/Scripts/webkitpy/test/main.py:67
> +    return _host.executive.run_command([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], return_exit_code=True) == 0

I am going to change this line to emit the full output on failure:

try:
    _host.executive.run_and_throw_if_fail([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], quiet=True)
except ScriptError as e:
    _log.error(e.message_with_output(output_limit=None))
    return False
return True

This will help diagnose the cause of the lldbWebKitTester compile-time error that led to this patch being rolled out in bug #186240. I will re-land this patch with the above change and add -framework Security to OTHER_LDFLAGS[sdk=macosx*] in file Tools/lldb/lldbWebKitTester/Configurations/lldbWebKitTester.xcconfig. I will watch the commit-queue for failure.
Comment 43 Daniel Bates 2018-06-27 21:17:40 PDT
Created attachment 343791 [details]
Patch for landing
Comment 44 Daniel Bates 2018-06-27 21:19:52 PDT
Committed r233299: <https://trac.webkit.org/changeset/233299>
Comment 45 Daniel Bates 2018-06-28 00:29:46 PDT
On Mac Developer Tools Access is required in order for the lldb tests to be able to debug the lldbWebKitTester process. Running the lldb tests will prompt for administrator credentials to enable such access. You can change you system-wide security authorization policy to avoid such prompts by running `/usr/sbin/DevToolsSecurity -enable`. See DevToolsSecurity(1) for more details.
Comment 46 Simon Fraser (smfr) 2018-06-28 08:28:43 PDT
(In reply to Daniel Bates from comment #42)
> Comment on attachment 341794 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341794&action=review
> 
> > Tools/Scripts/webkitpy/test/main.py:67
> > +    return _host.executive.run_command([build_lldbwebkittester, config.flag_for_configuration(config.default_configuration())], return_exit_code=True) == 0
> 
> I am going to change this line to emit the full output on failure:
> 
> try:
>     _host.executive.run_and_throw_if_fail([build_lldbwebkittester,
> config.flag_for_configuration(config.default_configuration())], quiet=True)
> except ScriptError as e:
>     _log.error(e.message_with_output(output_limit=None))
>     return False
> return True
> 
> This will help diagnose the cause of the lldbWebKitTester compile-time error
> that led to this patch being rolled out in bug #186240. I will re-land this
> patch with the above change and add -framework Security to
> OTHER_LDFLAGS[sdk=macosx*] in file
> Tools/lldb/lldbWebKitTester/Configurations/lldbWebKitTester.xcconfig. I will
> watch the commit-queue for failure.

Nice. I discovered it by building lldbWebKitTester.xcconfig by hand in Xcode.
Comment 47 Chris Dumez 2018-06-28 09:56:02 PDT
This may have broken iOS build:
Ld /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/lldbWebKitTester normal arm64
    cd /Volumes/Data/WebKit/OpenSource/Tools/lldb/lldbWebKitTester
    /Applications/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/usr/bin/clang++ -arch arm64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.0.Internal.sdk -L/Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos -F/Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos -iframework /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.0.Internal.sdk/System/Library/PrivateFrameworks -filelist /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester.LinkFileList -miphoneos-version-min=12.0 -Xlinker -object_path_lto -Xlinker /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester_lto.o -Xlinker -no_deduplicate -stdlib=libc++ -licucore -lWTF -Xlinker -dependency_info -Xlinker /Volumes/Data/WebKit/OpenSource/WebKitBuild/lldbWebKitTester.build/Release-iphoneos/lldbWebKitTester.build/Objects-normal/arm64/lldbWebKitTester_dependency_info.dat -o /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/lldbWebKitTester
Undefined symbols for architecture arm64:
  "_CFAllocatorCreate", referenced from:
      WTF::StringImpl::createCFString() in libWTF.a(StringImplCF.o)
  "_CFArrayGetCount", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFArrayGetValueAtIndex", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFGetTypeID", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFLocaleCopyPreferredLanguages", referenced from:
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFLocaleCreateCanonicalLanguageIdentifierFromString", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFPreferencesCopyValue", referenced from:
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFRelease", referenced from:
      WTF::createWithFormatAndArguments(char const*, char*) in libWTF.a(WTFString.o)
      vprintf_stderr_common(char const*, char*) in libWTF.a(Assertions.o)
      WTF::timerFired(__CFRunLoopTimer*, void*) in libWTF.a(MainThreadMac.o)
      WTF::currentSearchLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
      WTF::currentTextBreakLocaleID() in libWTF.a(TextBreakIteratorInternalICUMac.o)
  "_CFRetain", referenced from:


At least I cannot link anymore.
Comment 48 Dawei Fenton (:realdawei) 2018-06-28 10:13:21 PDT
This is causing timeouts on Sierra

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-test/logs/stdio
Comment 49 Daniel Bates 2018-06-28 10:41:40 PDT
(In reply to Chris Dumez from comment #47)
> This may have broken iOS build:
> Ld
> /Volumes/Data/WebKit/OpenSource/WebKitBuild/Release-iphoneos/
> lldbWebKitTester normal arm64
>     cd /Volumes/Data/WebKit/OpenSource/Tools/lldb/lldbWebKitTester
>    
> /Applications/Xcode.app/Contents/Developer/Toolchains/iOS12.0.xctoolchain/
> [...]
> libWTF.a(TextBreakIteratorInternalICUMac.o)
>   "_CFRetain", referenced from:
> 
> 
> At least I cannot link anymore.

We should only build lldbWebKitTester for Mac. Committed fix in <https://trac.webkit.org/changeset/233313>.
Comment 50 Daniel Bates 2018-06-28 10:44:25 PDT
(In reply to David Fenton (:realdawei) from comment #48)
> This is causing timeouts on Sierra
> 
> https://build.webkit.org/builders/
> Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> test/logs/stdio

Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164, 199}, which services this queue, to allow debugging. See comment 45 for more details.
Comment 51 Daniel Bates 2018-06-28 10:45:48 PDT
(In reply to Daniel Bates from comment #50)
> (In reply to David Fenton (:realdawei) from comment #48)
> > This is causing timeouts on Sierra
> > 
> > https://build.webkit.org/builders/
> > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> > test/logs/stdio
> 
> Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164,
> 199}, which services this queue, to allow debugging. See comment 45 for more
> details.

We will need to enable Developer Tool Access on all Mac and iOS bots that run test-webkitpy.
Comment 52 Daniel Bates 2018-06-28 10:47:56 PDT
(In reply to Daniel Bates from comment #51)
> (In reply to Daniel Bates from comment #50)
> > (In reply to David Fenton (:realdawei) from comment #48)
> > > This is causing timeouts on Sierra
> > > 
> > > https://build.webkit.org/builders/
> > > Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/10215/steps/webkitpy-
> > > test/logs/stdio
> > 
> > Needed to run `sudo /usr/sbin/DevToolsSecurity -enable` on the bot{164,
> > 199}, which services this queue, to allow debugging. See comment 45 for more
> > details.
> 
> We will need to enable Developer Tool Access on all Mac and iOS bots that
> run test-webkitpy.

err, I meant to write:

We will need to enable Developer Tool Access on all Mac test bots that run test-webkitpy.