Bug 70688 – [chromium] Manage webkit_unit_tests TestSuite lifetime explicitly in DLL build

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nat Duca]

[chromium] Manage TestSuite lifetime explicitly, fixing crash in webkit_unit_tests in DLL build."

[Nat Duca]

Created attachment 112103 [details]
Patch

[WebKit Review Bot]

Attachment 112103 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/tests/WebUnitTests.h:46:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Source/WebKit/chromium/tests/WebUnitTests.h:49:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Source/WebKit/chromium/tests/WebUnitTests.h:52:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

On component/dll builds, the existing code çreates two test suites -- main() creates a TestSuite, then WebKit::RunAllTests creates another. This leads to two nasty side effects:
- webkit_unit_tests crashes after it runs. The tests show as passing, but then we get a stack dump in ~CommandLine.
- The "real" testsuite thinks it doesn't have a command line. This makes things like --gtest_filter don't work

This patch replaces the RunAllTests with three functions: InitTestSuite,RunAllTests,DeleteTestSuite. This allows the overall main() function to create the suite, initialize webkit support, run the tests, then tear everything down again in the right order.

I'm not sure how to make the style bot happy. Should tests/WebUnitTests.h be in public?

[Tony Chang]

Comment on attachment 112103 [details]
Patch

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

> Source/WebKit/chromium/tests/RunAllTests.cpp:47
> +// and run inside awebkit.dll.

Nit: awebkit typo

> Source/WebKit/chromium/tests/WebUnitTests.cpp:38
> +namespace {
> +static TestSuite* testSuite = 0;

Nit: No namespace (it's redundant with static and WebKit style prefers static).

>> Source/WebKit/chromium/tests/WebUnitTests.h:46
>> +WEBKIT_EXPORT void InitTestSuite(int argc, char** argv);
> 
> WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]

We could add a WEBKIT_EXPORT_PRIVATE for these.  See src/net/base/net_export.h for an example of this.

[Nat Duca]

Created attachment 112208 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tony Chang]

LGTM, but let's let Darin review the addition of WEBKIT_EXPORT_PRIVATE.

[Darin Fisher (:fishd, Google)]

Comment on attachment 112208 [details]
Patch

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

> Source/WebKit/chromium/public/WebCommon.h:72
> +            #define WEBKIT_EXPORT_PRIVATE __declspec(dllexport)

I don't think WEBKIT_EXPORT_PRIVATE is all that helpful.  I'm also don't
think it is that helpful for net/ to have NET_EXPORT_PRIVATE :)

The only reason we have NET_EXPORT_PRIVATE is because Ricardo took the
time to carefully separate net/ entry points used only by tests from
those used by Chrome.  The downside of this approach is that it is really
hard to maintain.  I would not be surprised to see NET_EXPORT_PRIVATE
being used by Chrome now since there is no mechanism to prevent it.

Let's just stick with WEBKIT_EXPORT.  It is easier to understand.  I'm not
too worried about using it outside of the public/ directory for this one
case.

[Nat Duca]

Created attachment 112220 [details]
Patch

[Tony Chang]

Comment on attachment 112220 [details]
Patch

Feel free to ignore the style bot.  We can fix that the style checker in a follow up bug to ignore the tests directory.

[WebKit Review Bot]

Attachment 112220 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/chromium/ChangeLog', u'Sourc..." exit_code: 1

Source/WebKit/chromium/tests/WebUnitTests.h:46:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Source/WebKit/chromium/tests/WebUnitTests.h:49:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Source/WebKit/chromium/tests/WebUnitTests.h:52:  WEBKIT_EXPORT should only appear in the chromium public directory.  [readability/webkit_export] [5]
Total errors found: 3 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nat Duca]

Committed r98269: <http://trac.webkit.org/changeset/98269>