Bug 171800 – [Win] StaticStringImpl in HTMLNames.cpp aren't constructed

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Fujii Hironori]

Since Bug 171586, when I start MiniBrowser of WinCairo port, the following assertion fails:

> ASSERTION FAILED: hasHash()
> C:\webkit\ga\WebKitBuild\Debug\DerivedSources\ForwardingHeaders\wtf/text/StringImpl.h(883) : WTF::StringImpl::assertHashIsCorrect

Callstack:

> WTF.dll!WTFCrash() Line 292	C++
> WebKit.dll!WTF::StringImpl::assertHashIsCorrect() Line 883	C++
> WebKit.dll!WebCore::HTMLNames::init() Line 1666	C++
> WebKit.dll!WebCore::Frame::Frame(WebCore::Page & page, WebCore::HTMLFrameOwnerElement * ownerElement, WebCore::FrameLoaderClient & frameLoaderClient) Line 175	C++
> WebKit.dll!WebCore::MainFrame::MainFrame(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 56	C++
> WebKit.dll!WebCore::MainFrame::create(WebCore::Page & page, WebCore::PageConfiguration & configuration) Line 68	C++
> WebKit.dll!WebCore::Page::Page(WebCore::PageConfiguration && pageConfiguration) Line 271	C++
> WebKit.dll!WebView::initWithFrame(tagRECT frame, wchar_t * frameName, wchar_t * groupName) Line 3122	C++
> MiniBrowserLib.dll!MiniBrowser::prepareViews(HWND__ * mainWnd, const tagRECT & clientRect, wchar_t * const & requestedURL, HWND__ * & viewHwnd) Line 100	C++
> MiniBrowserLib.dll!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 160	C++
> MiniBrowserLib.dll!dllLauncherEntryPoint(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 857	C++
> MiniBrowser.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * lpstrCmdLine, int nCmdShow) Line 249	C++
> [External Code]	

All layout test of AppleWin port also fail:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/593

[Per Arne Vollan]

Created attachment 309479 [details]
Patch

[Per Arne Vollan]

Created attachment 309480 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 309480 [details]
Patch

Hmm, I think we should fix this issue by ensuring StaticStringImpl is correctly initialized at compile time in Windows even in it is a global variable.
Current fix is moving it to static variables in functions. It does not work well for StringImpl::empty()'s static string.
And we don't want to make it static string inside function because we cannot make StringImpl::empty() inline-function.

Since assertHashIsCorrect() exists before Bug 171586, I guess Bug 171586 change makes these strings non-compile-time-initialized in Windows.
Can you ensure that this `assertHashIsCorrect()` does not fire before Bug 171586?
We can guarantee that they are compile-time-initialized in OSX since we fail to compile if the code includes global constructor invocations.

If we can ensure that this `assertHashIsCorrect()` does not fire before Bug 171586, we can assume that Bug 171586 changes these strings non-compile-time-initialized thing in Windows. And I think the right fix is changing them compile-time-string again (as is in OSX).
I guess non-constexpr StringImplShape is called from StaticStringImpl constructor.

[Per Arne Vollan]

Created attachment 309486 [details]
Patch

[Per Arne Vollan]

(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 309480 [details]
> Patch
> 
> Hmm, I think we should fix this issue by ensuring StaticStringImpl is
> correctly initialized at compile time in Windows even in it is a global
> variable.
> Current fix is moving it to static variables in functions. It does not work
> well for StringImpl::empty()'s static string.
> And we don't want to make it static string inside function because we cannot
> make StringImpl::empty() inline-function.
> 
> Since assertHashIsCorrect() exists before Bug 171586, I guess Bug 171586
> change makes these strings non-compile-time-initialized in Windows.
> Can you ensure that this `assertHashIsCorrect()` does not fire before Bug
> 171586?
> We can guarantee that they are compile-time-initialized in OSX since we fail
> to compile if the code includes global constructor invocations.
> 
> If we can ensure that this `assertHashIsCorrect()` does not fire before Bug
> 171586, we can assume that Bug 171586 changes these strings
> non-compile-time-initialized thing in Windows. And I think the right fix is
> changing them compile-time-string again (as is in OSX).
> I guess non-constexpr StringImplShape is called from StaticStringImpl
> constructor.

Thanks for reviewing! Updated patch.

[Yusuke Suzuki]

Comment on attachment 309486 [details]
Patch

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

> Source/WebCore/dom/make_names.pl:577
> +    print F "#undef SKIP_STATIC_CONSTRUCTORS_ON_MSVC\n\n";

Oops, sorry for my misleading comment.

We should not allow static constructors here.
I mean that we should ensure that Windows works before Bug 171586.
Not invoking static constructors is fine. (so above change is not correct).

But these variables should be initialized because these variables invokes `constexpr` constructors.
I suspect that Bug 171586 change makes these variables uninitialized.

[Per Arne Vollan]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 309486 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309486&action=review
> 
> > Source/WebCore/dom/make_names.pl:577
> > +    print F "#undef SKIP_STATIC_CONSTRUCTORS_ON_MSVC\n\n";
> 
> Oops, sorry for my misleading comment.
> 
> We should not allow static constructors here.
> I mean that we should ensure that Windows works before Bug 171586.
> Not invoking static constructors is fine. (so above change is not correct).
> 
> But these variables should be initialized because these variables invokes
> `constexpr` constructors.
> I suspect that Bug 171586 change makes these variables uninitialized.

Yes, I believe this was working before Bug 171586.
When SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined on Windows, not only static constructors are skipped, but also compile time initialization of objects is incorrect. When not defining SKIP_STATIC_CONSTRUCTORS_ON_MSVC, the compile time initialization of these objects seem to be correct.

From StaticConstructors.h:

#ifdef SKIP_STATIC_CONSTRUCTORS_ON_MSVC
// - Assume that all includes of this header want ALL of their static
//   initializers ignored. This is currently the case. This means that if
//   a .cc includes this header (or it somehow gets included), all static
//   initializers after the include will not be executed.
// - We do this with a pragma, so that all of the static initializer pointers
//   go into our own section, and the CRT won't call them. Eventually it would
//   be nice if the section was discarded, because we don't want the pointers.
//   See: http://msdn.microsoft.com/en-us/library/7977wcck(VS.80).aspx
#pragma warning(disable:4075)
#pragma init_seg(".unwantedstaticinits")
#endif

This will skip static constructors as expected, but also seem to have the side effect of not initializing the objects in HTMLNames.cpp at compile time. Please correct me if I am wrong. Perhaps we should investigate whether  this pragma should be used at all?

[Yusuke Suzuki]

Comment on attachment 309486 [details]
Patch

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

>>> Source/WebCore/dom/make_names.pl:577
>>> +    print F "#undef SKIP_STATIC_CONSTRUCTORS_ON_MSVC\n\n";
>> 
>> Oops, sorry for my misleading comment.
>> 
>> We should not allow static constructors here.
>> I mean that we should ensure that Windows works before Bug 171586.
>> Not invoking static constructors is fine. (so above change is not correct).
>> 
>> But these variables should be initialized because these variables invokes `constexpr` constructors.
>> I suspect that Bug 171586 change makes these variables uninitialized.
> 
> Yes, I believe this was working before Bug 171586.
> When SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined on Windows, not only static constructors are skipped, but also compile time initialization of objects is incorrect. When not defining SKIP_STATIC_CONSTRUCTORS_ON_MSVC, the compile time initialization of these objects seem to be correct.
> 
> From StaticConstructors.h:
> 
> #ifdef SKIP_STATIC_CONSTRUCTORS_ON_MSVC
> // - Assume that all includes of this header want ALL of their static
> //   initializers ignored. This is currently the case. This means that if
> //   a .cc includes this header (or it somehow gets included), all static
> //   initializers after the include will not be executed.
> // - We do this with a pragma, so that all of the static initializer pointers
> //   go into our own section, and the CRT won't call them. Eventually it would
> //   be nice if the section was discarded, because we don't want the pointers.
> //   See: http://msdn.microsoft.com/en-us/library/7977wcck(VS.80).aspx
> #pragma warning(disable:4075)
> #pragma init_seg(".unwantedstaticinits")
> #endif
> 
> This will skip static constructors as expected, but also seem to have the side effect of not initializing the objects in HTMLNames.cpp at compile time. Please correct me if I am wrong. Perhaps we should investigate whether  this pragma should be used at all?

If this was working before Bug 171586, should we fix StaticStringImpl side instead of adding the above undef?
This is because our StaticStringImpl work fine even if SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined before Bug 171586.

[Yusuke Suzuki]

Comment on attachment 309486 [details]
Patch

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

>>>> Source/WebCore/dom/make_names.pl:577
>>>> +    print F "#undef SKIP_STATIC_CONSTRUCTORS_ON_MSVC\n\n";
>>> 
>>> Oops, sorry for my misleading comment.
>>> 
>>> We should not allow static constructors here.
>>> I mean that we should ensure that Windows works before Bug 171586.
>>> Not invoking static constructors is fine. (so above change is not correct).
>>> 
>>> But these variables should be initialized because these variables invokes `constexpr` constructors.
>>> I suspect that Bug 171586 change makes these variables uninitialized.
>> 
>> Yes, I believe this was working before Bug 171586.
>> When SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined on Windows, not only static constructors are skipped, but also compile time initialization of objects is incorrect. When not defining SKIP_STATIC_CONSTRUCTORS_ON_MSVC, the compile time initialization of these objects seem to be correct.
>> 
>> From StaticConstructors.h:
>> 
>> #ifdef SKIP_STATIC_CONSTRUCTORS_ON_MSVC
>> // - Assume that all includes of this header want ALL of their static
>> //   initializers ignored. This is currently the case. This means that if
>> //   a .cc includes this header (or it somehow gets included), all static
>> //   initializers after the include will not be executed.
>> // - We do this with a pragma, so that all of the static initializer pointers
>> //   go into our own section, and the CRT won't call them. Eventually it would
>> //   be nice if the section was discarded, because we don't want the pointers.
>> //   See: http://msdn.microsoft.com/en-us/library/7977wcck(VS.80).aspx
>> #pragma warning(disable:4075)
>> #pragma init_seg(".unwantedstaticinits")
>> #endif
>> 
>> This will skip static constructors as expected, but also seem to have the side effect of not initializing the objects in HTMLNames.cpp at compile time. Please correct me if I am wrong. Perhaps we should investigate whether  this pragma should be used at all?
> 
> If this was working before Bug 171586, should we fix StaticStringImpl side instead of adding the above undef?
> This is because our StaticStringImpl work fine even if SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined before Bug 171586.

I guess, StaticStringImpl should call StringImplShape base constructor with the form like `StringImplShape<charactersCount>(...)`. And maybe, constexpr StringImplShape constructor needs changes.

[Fujii Hironori]

I tested a following code with VS2015Update3.

> #include "stdio.h"
> 
> class Foo {
> public:
>     Foo(int x, int y) : m_x(x), m_y(y) { }
>     constexpr Foo(int x) : m_x(x), m_y(0) { }
>     int m_x;
>     int m_y;
> };
> 
> Foo f1(1, 2);
> Foo f2(2);
> 
> #pragma init_seg(".hoge")
> 
> Foo f3(1, 2);
> Foo f4(2);
> 
> int main()
> {
>     printf("%d\n", f1 .m_x);
>     printf("%d\n", f2 .m_x);
>     printf("%d\n", f3 .m_x);
>     printf("%d\n", f4 .m_x);
>     return 0;
> }

Result:

> 1
> 2
> 0
> 2

The init_seg trick skips only non-constexpr constructor.

[Per Arne Vollan]

(In reply to Yusuke Suzuki from comment #8)
> Comment on attachment 309486 [details]
> Patch
> 
> If this was working before Bug 171586, should we fix StaticStringImpl side
> instead of adding the above undef?
> This is because our StaticStringImpl work fine even if
> SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined before Bug 171586.

Sorry, I might be misunderstanding here :)

Instead of adding the undef, perhaps we should look into removing #pragma init_seg(".unwantedstaticinits") from StaticConstructors.h, since it seems to break compile time initialization?

Thanks for reviewing!

[Yusuke Suzuki]

(In reply to Per Arne Vollan from comment #11)
> (In reply to Yusuke Suzuki from comment #8)
> > Comment on attachment 309486 [details]
> > Patch
> > 
> > If this was working before Bug 171586, should we fix StaticStringImpl side
> > instead of adding the above undef?
> > This is because our StaticStringImpl work fine even if
> > SKIP_STATIC_CONSTRUCTORS_ON_MSVC is defined before Bug 171586.
> 
> Sorry, I might be misunderstanding here :)
> 
> Instead of adding the undef, perhaps we should look into removing #pragma
> init_seg(".unwantedstaticinits") from StaticConstructors.h, since it seems
> to break compile time initialization?

Ah, no. Instead of adding undef / changing pragma init_seg, we should fix the current implementation of StaticStringImpl. If this was working before Bug 171586, the patch introduced in Bug 171586 breaks the StaticStringImpl.

> 
> Thanks for reviewing!

[Mark Lam]

@Per, thanks for investigating and identifying what is going wrong here.  I agree with Yusuke's point that StaticStringImpl should use the constexpr forms of StringImplShape's constructors.  However, I'm not 100% certain that that's all there is to it.

So, I'll file a separate bug to implement Yusuke's solution, and we'll see how the bots fare thereafter.  If the issue is resolved, we can close this bug as a dupe.  If not, we'll use this bug to investigate the next issue.

[Per Arne Vollan]

(In reply to Mark Lam from comment #13)
> @Per, thanks for investigating and identifying what is going wrong here.  I
> agree with Yusuke's point that StaticStringImpl should use the constexpr
> forms of StringImplShape's constructors.  However, I'm not 100% certain that
> that's all there is to it.
> 
> So, I'll file a separate bug to implement Yusuke's solution, and we'll see
> how the bots fare thereafter.  If the issue is resolved, we can close this
> bug as a dupe.  If not, we'll use this bug to investigate the next issue.

Sounds good, thanks guys!

[Mark Lam]

@Per, the fix for https://bugs.webkit.org/show_bug.cgi?id=171861 has landed in r216512, but from the looks of the bot, I suspect that it didn't work.  Can you investigate further and see whether VC++ is honoring constexprs at all?  Thanks.

[Fujii Hironori]

A lot of compilation warnings are output while compiling HTMLNames.cpp of WinCairo port:

>------ Build started: Project: WebCoreDerivedSources, Configuration: Debug x64 ------
>  HTMLNames.cpp
>C:\webkit\ga\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp(54): warning C4592: 'WebCore::HTMLNames::aData': symbol will be dynamically initialized (implementation limitation)
>C:\webkit\ga\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp(55): warning C4592: 'WebCore::HTMLNames::abbrData': symbol will be dynamically initialized (implementation limitation)
>C:\webkit\ga\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp(56): warning C4592: 'WebCore::HTMLNames::acceptData': symbol will be dynamically initialized (implementation limitation)
>C:\webkit\ga\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp(57): warning C4592: 'WebCore::HTMLNames::accept_charsetData': symbol will be dynamically initialized (implementation limitation)
>C:\webkit\ga\WebKitBuild\Debug\DerivedSources\WebCore\HTMLNames.cpp(58): warning C4592: 'WebCore::HTMLNames::accesskeyData': symbol will be dynamically initialized (implementation limitation)
>(...)

Supprisingly, these warnings had been output even before Bug 171586 change.

[Fujii Hironori]

Created attachment 309578 [details]
WIP patch

>    constexpr StringImplShape(unsigned refCount, unsigned length, const char (&characters)[charactersCount], unsigned hashAndFlags, ConstructWithConstExprTag)
>        : m_refCount(refCount)
>        , m_length(length)
>        , m_data8(reinterpret_cast<const LChar*>(characters))
>        , m_hashAndFlags(hashAndFlags)
>    { }

It seems that this reinterpret_cast prevents the compile-time-initialization.
This reinterpret_cast has been introduced in Bug 171586.

I removed the reinterpret_cast in the WIP patch. It works fine.

[Fujii Hironori]

Created attachment 309579 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 309579 [details]
Patch

r=me. Right, reinterpret_cast is not constexpr.

[Per Arne Vollan]

(In reply to Fujii Hironori from comment #18)
> Created attachment 309579 [details]
> Patch

Great work!

[WebKit Commit Bot]

Comment on attachment 309579 [details]
Patch

Clearing flags on attachment: 309579

Committed r216566: <http://trac.webkit.org/changeset/216566>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.