Windows tests broken after r162816

• Component: Tools / Tests
• Status: NEW ---

[Roger Fong]

r162816 is causing all Windows tests to crash.

The reason is because the test infrastructure now accesses the static class variable gAccessibilityEnabled variable defined in AXObjectCache.h and initialized in AXObjectCache.cpp.

However, we are accessing the variable from WebCoreTestSupport not WebCore, which does not include AXObjectCache.cpp and thus does not initialize the variable.

As a result, when we call the method, (from Internals.cpp in the WebCoreTestSupport project) we crash because we have not initialized the gAccessibilityEnabled variable in any cpp in the project.

Solutions:
We could just initialize it somewhere (like in Internals.cpp) but then we have a copy of that initializing in both WebCore/AXObjectCache.cpp and WebCoreTestSupport/Internals.cpp

Or we could just include AXObjectCache.cpp, have WebCore copy the file over to the build directory, etc etc etc, which is kind of strange because we don't do that for any other files in WebCoreTestSupport.

[Alexey Proskuryakov]

gAccessibilityEnabled is exported from WebKit, so I don't see why this doesn't work. The variable should be shared across dynamic libraries.

[Roger Fong]

Created attachment 222356 [details]
patch

[Roger Fong]

(In reply to comment #1)
> gAccessibilityEnabled is exported from WebKit, so I don't see why this doesn't work. The variable should be shared across dynamic libraries.

didn't read this before uploading the patch (which does seem to fix the issue)

That is confusing, and in fact if i'm initializing the variable in WebCoreTestSupport.lib and WebKit.dll shouldn't there be some kinda multiple definition clash? (I did not see this, when running the tests with my patch in place)

[chris fleizach]

Comment on attachment 222356 [details]
patch

Won't the two gAccessibilityEnabled get out of sync now?

[Roger Fong]

(In reply to comment #4)
> (From update of attachment 222356 [details])
> Won't the two gAccessibilityEnabled get out of sync now?

Right, which is why I was thinking about having WebCoreTestSupport compile AXObjectCache.cpp on its own. Or alternatively maybe just get rid of WebCoreTestSupport and stick it in WebCore (like it is on Mac??). 

I'm not quite sure why we need the separate project.

[Roger Fong]

Ignore all the things I said.

Alexey and I think that there's something odd with how Windows data exporting works.

What we should do is to make the added function non inline and then export the functions.

[Roger Fong]

Created attachment 222374 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/WebKit/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebCore/accessibility/AXObjectCache.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AXObjectCache.h:130:  Tab found; better to use spaces  [whitespace/tab] [1]
ERROR: Source/WebCore/accessibility/AXObjectCache.h:131:  Tab found; better to use spaces  [whitespace/tab] [1]
Total errors found: 4 in 5 files


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

[Alexey Proskuryakov]

Comment on attachment 222374 [details]
patch

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

> Source/WebCore/ChangeLog:21
> [phone]  Roger Fong  <[e-mail]>
> +
> +        [Windows] Tests crashing on Windows after r162816.
> +        https://bugs.webkit.org/show_bug.cgi?id=127703.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * accessibility/AXObjectCache.cpp:
> +        (WebCore::AXObjectCache::enableAccessibility):
> +        (WebCore::AXObjectCache::disableAccessibility):
> +        * accessibility/AXObjectCache.h: Un-inline some methods so that they can be exported.
> +
> [phone]  Roger Fong  <[e-mail]>
> +
> +        [Windows] Tests crashing on Windows after r162816.
> +        https://bugs.webkit.org/show_bug.cgi?id=127703.
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * testing/Internals.cpp: Initialize gAccessibilityEnabled in WebCoreTestSupport.
> +

Double ChangeLog.

> Source/WebCore/accessibility/AXObjectCache.cpp:112
> +void AXObjectCache::enableAccessibility() { gAccessibilityEnabled = true; }
> +void AXObjectCache::disableAccessibility() { gAccessibilityEnabled = false; };

Please format these as normal multi-line functions, and remove the trailing semicolon.

> Source/WebKit/ChangeLog:4
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

Yup.