Bug 175209 – [Win] When built with VS2017, MiniBrowser crashes on startup.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

WTF.dll!WTFCrash() Line 292	C++
 	WebKit.dll!WTF::StringImpl::assertHashIsCorrect() Line 889	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 270	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++

[Per Arne Vollan]

Created attachment 317286 [details]
Patch

[Brent Fulgham]

Comment on attachment 317286 [details]
Patch

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

> Source/WebCore/bindings/scripts/StaticString.pm:46
> +        push(@result, "constexpr StringImpl::StaticStringImpl ${name}Data(\"${value}\");\n");

I think we should have JF Bastien look at this. I'm not sure if this is the correct way to fix this, especially given the casting we need to do in the rest of this patch.

[Brent Fulgham]

JF: Can you look at this patch? I'm not sure if this is a Visual Studio 2017 bug, or a change that makes the code more correct via constexpr.

[JF Bastien]

It seems like keeping static would be good.

Are the extra const casts needed because we don't have const overloads for some StaticString methods? Adding the overloads would be a nicer fix.

[Per Arne Vollan]

(In reply to JF Bastien from comment #4)
> It seems like keeping static would be good.
> 
> Are the extra const casts needed because we don't have const overloads for
> some StaticString methods? Adding the overloads would be a nicer fix.

Thanks for reviewing! I will update the patch.

[Per Arne Vollan]

Created attachment 317444 [details]
Patch

[Per Arne Vollan]

(In reply to JF Bastien from comment #4)
> It seems like keeping static would be good.
> 
> Are the extra const casts needed because we don't have const overloads for
> some StaticString methods? Adding the overloads would be a nicer fix.

I added the const casts because we don't have AtomicString(const StringImpl*) (we have AtomicString(StringImpl*)), but adding this lead to a chain of const changes, which I think is outside the scope of this bug, so I kept the const casts.

[Per Arne Vollan]

Created attachment 317446 [details]
Patch

[Fujii Hironori]

Just out of curiosity.
Is it safe to remove const from constant global variables?
https://stackoverflow.com/q/27595839/2691131

[Per Arne Vollan]

Created attachment 317652 [details]
Patch

[Per Arne Vollan]

(In reply to Fujii Hironori from comment #9)
> Just out of curiosity.
> Is it safe to remove const from constant global variables?
> https://stackoverflow.com/q/27595839/2691131

This is a good point. I guess it is depending highly on which compiler is used, whether it is a debug or release build, and what type of object const is removed from. I have removed the const casts in the latest patch.

[Brent Fulgham]

Comment on attachment 317652 [details]
Patch

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

> Source/WTF/wtf/text/AtomicString.h:61
> +    AtomicString(const StaticStringImpl*);

I don't understand why this has to be a "const StaticStringImpl", rather than a "const StringImpl*". Are StringImpl objects always mutable?

It seems like we work with StringImpl at most other layers of the code, not StaticStringImpl, so I'm not sure if it's appropriate to do some of the casting elsewhere in this patch.

> Source/WTF/wtf/text/AtomicStringImpl.cpp:422
> +    auto s = reinterpret_cast<const StringImpl*>(string);

Why do we take a "const StaticStringImpl" and convert to "const StringImpl" here? It seems like we could just start of with "const StringImpl*" and avoid the reinterpret-cast?

> Source/WebCore/dom/QualifiedName.cpp:72
> +void createQualifiedName(void* targetAddress, const StaticStringImpl* name, const AtomicString& nameNamespace)

Here we go from a "StringImpl*" to a "const StaticStringImpl*", which will get reinterpret_cast to "const StringImpl*". Is that really necessary?

> Source/WebCore/dom/QualifiedName.cpp:77
> +void createQualifiedName(void* targetAddress, const StaticStringImpl* name)

Ditto.

> Source/WebCore/dom/make_names.pl:143
> +        print F "    new (NotNull, (void*)&$name) AtomicString(&${name}Data);\n";

Maybe this is why? Does &${name}Data map to "StaticStringImpl", and was being case here previously?

> Source/WebCore/dom/make_names.pl:915
> +        print F "        { (void*)&$name$shortCamelType, *(&${name}Data) },\n";

Ditto.

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #12)
> Comment on attachment 317652 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317652&action=review
> 
> > Source/WTF/wtf/text/AtomicString.h:61
> > +    AtomicString(const StaticStringImpl*);
> 
> I don't understand why this has to be a "const StaticStringImpl", rather
> than a "const StringImpl*". Are StringImpl objects always mutable?
> 
> It seems like we work with StringImpl at most other layers of the code, not
> StaticStringImpl, so I'm not sure if it's appropriate to do some of the
> casting elsewhere in this patch.
> 

That's a good point. I have tried adding the constructor AtomicString(const StringImpl*) instead, but unfortunately this turns out to be more difficult, mainly because of the following code:

    ALWAYS_INLINE static Ref<AtomicStringImpl> add(StringImpl& string)
    {
        if (string.isAtomic()) {
            ASSERT_WITH_MESSAGE(!string.length() || isInAtomicStringTable(&string), "The atomic string comes from an other thread!");
            return static_cast<AtomicStringImpl&>(string);
        }
        return addSlowCase(string);
    }

When the string is atomic we just return the string input parameter, but we cannot do this if the input parameter is const, since we cannot change the ref count of a const object.

Given this, it seems to be easier to add the AtomicString(const StaticStringImpl*) constructor. Or are there alternative solutions we should consider?

> > Source/WTF/wtf/text/AtomicStringImpl.cpp:422
> > +    auto s = reinterpret_cast<const StringImpl*>(string);
> 
> Why do we take a "const StaticStringImpl" and convert to "const StringImpl"
> here? It seems like we could just start of with "const StringImpl*" and
> avoid the reinterpret-cast?
> 
> > Source/WebCore/dom/QualifiedName.cpp:72
> > +void createQualifiedName(void* targetAddress, const StaticStringImpl* name, const AtomicString& nameNamespace)
> 
> Here we go from a "StringImpl*" to a "const StaticStringImpl*", which will
> get reinterpret_cast to "const StringImpl*". Is that really necessary?
> 
> > Source/WebCore/dom/QualifiedName.cpp:77
> > +void createQualifiedName(void* targetAddress, const StaticStringImpl* name)
> 
> Ditto.
> 
> > Source/WebCore/dom/make_names.pl:143
> > +        print F "    new (NotNull, (void*)&$name) AtomicString(&${name}Data);\n";
> 
> Maybe this is why? Does &${name}Data map to "StaticStringImpl", and was
> being case here previously?
> 
> > Source/WebCore/dom/make_names.pl:915
> > +        print F "        { (void*)&$name$shortCamelType, *(&${name}Data) },\n";
> 
> Ditto.

We start off with static global const StaticStringImpl objects, and at one point we need to reinterpret cast it to a StringImpl object in order to create a AtomicString object from it. This patch has moved the reinterpret cast from generated code into the AtomicString implementation.

Thanks for reviewing, and sorry for the long response time!

[Per Arne Vollan]

Created attachment 321205 [details]
Patch

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #14)
> Created attachment 321205 [details]
> Patch

This patch takes a little different approach, in that it moves the definition of the static strings inside the init() method, which makes sure they will be initialized at runtime if the compiler refuses to initialize the objects at compile time. Only Windows is affected by this patch.

[Alex Christensen]

Comment on attachment 321205 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        VS2017 is not able to compile-time initialize the static strings. On Windows, we move the definition

Was VS2015 able to?  Why can't VS2017?

[Per Arne Vollan]

(In reply to Alex Christensen from comment #16)
> Comment on attachment 321205 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=321205&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        VS2017 is not able to compile-time initialize the static strings. On Windows, we move the definition
> 
> Was VS2015 able to?  Why can't VS2017?

Yes, VS2015 was able to. This might be a regression in VS2017. If we add constexpr to the definition of the static string variable itself, it also works in VS2017. But that implies quite a few const changes in AtomicString code. This was also not fixed in the first VS2017 update ...

[Alex Christensen]

I think we ought to be able to generate code that works on all compilers.  If we can't, then we should understand more about why we can't.

[Per Arne Vollan]

Created attachment 321836 [details]
Patch

[Per Arne Vollan]

(In reply to Alex Christensen from comment #18)
> I think we ought to be able to generate code that works on all compilers. 
> If we can't, then we should understand more about why we can't.

Thanks for reviewing! The current patch adds 'constexpr' to the StaticStringImpl objects for all platforms.

Here's a short example of why VS2017 needs 'constexpr' in the definition of the global StaticStringImpl objects:

class A {
public:
    constexpr A(int i)
    : m_i(i)
    {
    }

private:
    int m_i;
};

A a1(1); // a1 will not be compile-time initialized with VS2017.

constexpr A a2(2); // a2 will be compile-time initialized with VS2017.

[Daniel Bates]

Comment on attachment 321836 [details]
Patch

Is this patch working around a compiler quirk of VS2017? If so, we should file a bug against VS2017 and consider guarding this code behind COMPILER_QUIRK().

[Per Arne Vollan]

(In reply to Daniel Bates from comment #21)
> Comment on attachment 321836 [details]
> Patch
> 
> Is this patch working around a compiler quirk of VS2017? If so, we should
> file a bug against VS2017 and consider guarding this code behind
> COMPILER_QUIRK().

I will file a bug against VS2017. Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 321836 [details]
Patch

Clearing flags on attachment: 321836

Committed r223314: <https://trac.webkit.org/changeset/223314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35622210>