Bug 195920 – Build cleanly with GCC 9

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

WebKit triggers three new GCC 9 warnings:

-Wdeprecated-copy, implied by -Wextra, warns about the C++11 deprecation of implicitly declared copy constructor and assignment operator if one of them is user-provided. -Wdeprecated-copy-dtor also warns if the destructor is user-provided, as specified in C++11.

Solution is to either add a copy constructor or copy assignment operator, if required, or else remove one if it is redundant. This is the highest regression potential; I messed up ScriptDisallowedScope in an earlier version of this patch.

-Wredundant-move, implied by -Wextra, warns about redundant calls to std::move.
-Wpessimizing-move, implied by -Wall, warns when a call to std::move prevents copy elision.

These account for most of this patch. Solution is to just remove the bad WTFMove().

-Wclass-memaccess has also been enhanced to catch a few cases that GCC 8 didn't. These are solved by casting nontrivial types to void* before using memcpy. (Of course, it would be safer to not use memcpy on nontrivial types, but that's too complex for this patch.)

[Michael Catanzaro]

Created attachment 365090 [details]
Patch

[Michael Catanzaro]

I'll rebase the patch momentarily.

There's also this lovely warning that I don't know what to do with:

[1470/4720] Building C object Source/ThirdParty/libwebrtc...rty/usrsctp/usrsctplib/usrsctplib/netinet/sctp_output.c.o
In file included from ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_output.c:46:
../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_output.c: In function ‘send_forward_tsn’:
../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h:407:8: note: defined here
  407 | struct sctp_forward_tsn_chunk {
      |        ^~~~~~~~~~~~~~~~~~~~~~
../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h:418:8: note: defined here
  418 | struct sctp_strseq_mid {
      |        ^~~~~~~~~~~~~~~
../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_output.c: In function ‘sctp_send_sack’:
../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h:265:8: note: defined here
  265 | struct sctp_gap_ack_block {
      |        ^~~~~~~~~~~~~~~~~~

I guess it's a GCC bug, because there is absolutely no warning there, only notes.

[Michael Catanzaro]

Created attachment 365094 [details]
Patch

[EWS Watchlist]

Comment on attachment 365094 [details]
Patch

Attachment 365094 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/11556601

New failing tests:
(JS) JSTestCallbackFunction.cpp
(JS) JSTestCallbackFunctionRethrow.cpp
(JS) JSTestCallbackInterface.cpp

[Michael Catanzaro]

The bindings tests are checking for the presence of the WTFMove() in the generated code. I'll need to update the tests.

[EWS Watchlist]

Comment on attachment 365094 [details]
Patch

Attachment 365094 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11558063

New failing tests:
imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-constructor.html

[EWS Watchlist]

Created attachment 365119 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Michael Catanzaro]

Created attachment 365156 [details]
Patch

[EWS Watchlist]

Comment on attachment 365156 [details]
Patch

Attachment 365156 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/11565936

New failing tests:
(JS) JSTestCallbackFunction.cpp
(JS) JSTestCallbackFunctionRethrow.cpp

[Michael Catanzaro]

Created attachment 365161 [details]
Patch

[Xan Lopez]

Seems we need to be a bit careful with older compilers at least, though, see bug #195947.

[Michael Catanzaro]

Any WK2 owners want to approve this?

I'd have to install a really old distro to get access to clang 3.8, and there is no EWS testing it, so if this breaks clang 3.8 then it will have to be handled by someone with clang 3.8 in a follow-up. It's just not practical to handle that in a patch this big without EWS. I did find and address a couple cases that broke whatever old GCC is running on the WPE EWS (probably GCC 6?), though.

[Chris Dumez]

Comment on attachment 365161 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:234
> +            return reinterpret_cast<Ref<${objectType}>*>(&result)->copyRef();

Ideally, we'd find a way to write this which does not involve a copyRef(), to maintain the same performance characteristics.

> Source/WebCore/Modules/fetch/FetchBody.cpp:246
> +        return body.copyRef();

This seems worse, would the compiler accept:
return FetchBody { WTFMove(body) }; ?

> Source/WebCore/Modules/fetch/FetchBody.cpp:256
> +        return body.copyRef();

ditto.

> Source/WebCore/Modules/fetch/FetchBody.cpp:277
> +        return body.copyRef();

ditto.

> Source/WebCore/dom/ScriptDisallowedScope.h:58
> +    ScriptDisallowedScope& operator=(const ScriptDisallowedScope&)

would "= default;" work?

[Michael Catanzaro]

Thanks.

(In reply to Chris Dumez from comment #13)
> > Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:234
> > +            return reinterpret_cast<Ref<${objectType}>*>(&result)->copyRef();
> 
> Ideally, we'd find a way to write this which does not involve a copyRef(),
> to maintain the same performance characteristics.

I don't think it's possible, it's the same case as FetchBody:

> > Source/WebCore/Modules/fetch/FetchBody.cpp:246
> > +        return body.copyRef();
> 
> This seems worse, would the compiler accept:
> return FetchBody { WTFMove(body) }; ?

Nope, the return value is Ref<FetchBody>, that's the problem. Ref is move-only so if we can't move it, we have to copyRef().

I'm not sure, but I think GCC might be wrong to warn here, because the WTFMove() seems required, not redundant, unless I misunderstand.

We could suppress the warnings here using IGNORE_WARNINGS(), but I don't think it's worth it just to avoid constructing the extra Ref, destroying the original, and the extra refcount increment/decrement.

> > Source/WebCore/dom/ScriptDisallowedScope.h:58
> > +    ScriptDisallowedScope& operator=(const ScriptDisallowedScope&)
> 
> would "= default;" work?

This one is weird. On my first try, I removed the empty copy constructor because I didn't realize there was a member variable that needed to NOT be copied. The copy constructor is intentionally empty to prevent that, so this new assignment operator should be too.

[Michael Catanzaro]

Committed r243163: <https://trac.webkit.org/changeset/243163>

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> I don't think it's possible, it's the same case as FetchBody:

Well, it's not. My explanation for FetchBody works only for this case.

> > > Source/WebCore/Modules/fetch/FetchBody.cpp:246
> > > +        return body.copyRef();
> > 
> > This seems worse, would the compiler accept:
> > return FetchBody { WTFMove(body) }; ?
> 
> Nope, the return value is Ref<FetchBody>, that's the problem.

Wrong, the return value is RefPtr<FetchBody>. And that is copyable. This works fine for me:

  return body;

as it should. But it breaks the WPE EWS.

[Chris Dumez]

Comment on attachment 365161 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchBody.cpp:246
>> +        return body.copyRef();
> 
> This seems worse, would the compiler accept:
> return FetchBody { WTFMove(body) }; ?

Should be .releaseNonNull() I believe instead of copyRef() to avoid ref counting churn.

>> Source/WebCore/Modules/fetch/FetchBody.cpp:256
>> +        return body.copyRef();
> 
> ditto.

ditto.

>> Source/WebCore/Modules/fetch/FetchBody.cpp:277
>> +        return body.copyRef();
> 
> ditto.

ditto.

[Chris Dumez]

Comment on attachment 365161 [details]
Patch

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

>>> Source/WebCore/Modules/fetch/FetchBody.cpp:246
>>> +        return body.copyRef();
>> 
>> This seems worse, would the compiler accept:
>> return FetchBody { WTFMove(body) }; ?
> 
> Should be .releaseNonNull() I believe instead of copyRef() to avoid ref counting churn.

Although I am personally surprised we need to do anything there. I have a Ref<FormData> and the return type is RefPtr<FormData>, it is a common case where we normally return WTFMove().

[Chris Dumez]

Comment on attachment 365161 [details]
Patch

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

>>>> Source/WebCore/Modules/fetch/FetchBody.cpp:246
>>>> +        return body.copyRef();
>>> 
>>> This seems worse, would the compiler accept:
>>> return FetchBody { WTFMove(body) }; ?
>> 
>> Should be .releaseNonNull() I believe instead of copyRef() to avoid ref counting churn.
> 
> Although I am personally surprised we need to do anything there. I have a Ref<FormData> and the return type is RefPtr<FormData>, it is a common case where we normally return WTFMove().

Actually, I am wrong about releaseNonNull(), it is the other way around, we want to go from RefPtr to Ref, not from RefPtr to Ref.
I think the WTFMove() was the right way here and I do not understand why GCC was complaining about it.

[Michael Catanzaro]

(In reply to Chris Dumez from comment #19)
> I think the WTFMove() was the right way here and I do not understand why GCC
> was complaining about it.

WTFMove() would be right for returning a Ref, since Ref is noncopyable.

RefPtrs should not be returned through WTFMove() because they are copyable and copy elision is guaranteed.

Let's use bug #195962 to remove the copyRefs; you had some good suggestions on IRC.

[Chris Dumez]

(In reply to Michael Catanzaro from comment #20)
> (In reply to Chris Dumez from comment #19)
> > I think the WTFMove() was the right way here and I do not understand why GCC
> > was complaining about it.
> 
> WTFMove() would be right for returning a Ref, since Ref is noncopyable.
> 
> RefPtrs should not be returned through WTFMove() because they are copyable
> and copy elision is guaranteed.
> 
> Let's use bug #195962 to remove the copyRefs; you had some good suggestions
> on IRC.

When we have a local Ref<T> and the method return a Ref<T>, we already return without WTFMove() and it works. This only case we've needed return WTFMove() so far is when we have a local Ref<T> and the return is a RefPtr<T> because they are not the same type.

[Truitt Savell]

Looks like the changes in https://trac.webkit.org/changeset/243163/webkit

caused 4 api failures on Mac WK1:
WKNavigation.ProcessCrashDuringCallback 
WebKit.ApplicationManifestBasic 
WebKit.ApplicationManifestDisplayMode 
WKWebView.GetContentsShouldReturnString

Build:
https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/builds/2656

TestWebKitAPI.WebKit.ApplicationManifestBasic
        _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
        ASSERTION FAILED: HashTraits<uint64_t>::emptyValue() != m_id && !HashTraits<uint64_t>::isDeletedValue(m_id)
        /Volumes/Data/slave/mojave-debug/build/Source/WebKit/Shared/CallbackID.h(55) : WebKit::CallbackID &WebKit::CallbackID::operator=(const WebKit::CallbackID &)
        1   0x1105ae2c9 WTFCrash
        2   0x1157f2fab WTFCrashWithInfo(int, char const*, char const*, int)

[Truitt Savell]

They are crashing from the new assert added.

[Darin Adler]

Comment on attachment 365161 [details]
Patch

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

> Source/WebKit/Shared/CallbackID.h:53
> +        if (this == &otherID)
> +            return *this;

I suggest omitting this. It’s not needed for correctness nor does it improve performance.

> Source/WebKit/Shared/CallbackID.h:55
> +        ASSERT(HashTraits<uint64_t>::emptyValue() != m_id && !HashTraits<uint64_t>::isDeletedValue(m_id));

Why is the assertion correct?

> Source/WebKit/Shared/OptionalCallbackID.h:52
> +        if (this == &otherID)
> +            return *this;

Ditto.

> Source/WebKit/Shared/OptionalCallbackID.h:54
> +        ASSERT(!HashTraits<uint64_t>::isDeletedValue(m_id));

Ditto.

[Chris Dumez]

Follow-up landed in <https://trac.webkit.org/changeset/243203> to address the crashes and include suggestions from Darin.

[Michael Catanzaro]

> I suggest omitting this. It’s not needed for correctness nor does it improve performance.

It's good form for assignment operators IMO, but yes, it's not necessary in these cases.

(In reply to Darin Adler from comment #24)
> > Source/WebKit/Shared/CallbackID.h:55
> > +        ASSERT(HashTraits<uint64_t>::emptyValue() != m_id && !HashTraits<uint64_t>::isDeletedValue(m_id));
> 
> Why is the assertion correct?

Hm, it's weird. The assertion is copied from the copy constructor. (That's what the GCC warning is there for, to catch that the copy constructor and the copy assignment operator are mismatched.) The assertion seems to be there to make it illegal to copy an empty/default-constructed CallbackID. But, due to the missing assignment operator, we failed to actually enforce that as expected.

Anyway, it sure looks like empty CallbackIDs are not supposed to be used except as placeholders in containers, e.g. HashTables. So my suspicion is that the ASSERTs should be there? But I wouldn't feel confident saying that without being able to see more of the backtrace to see where it's coming from.

(In reply to Chris Dumez from comment #25)
> Follow-up landed in <https://trac.webkit.org/changeset/243203> to address
> the crashes and include suggestions from Darin.

Thanks for the follow-up!

(In reply to Truitt Savell from comment #22)
> Looks like the changes in https://trac.webkit.org/changeset/243163/webkit
> 
> caused 4 api failures on Mac WK1:
> WKNavigation.ProcessCrashDuringCallback 
> WebKit.ApplicationManifestBasic 
> WebKit.ApplicationManifestDisplayMode 
> WKWebView.GetContentsShouldReturnString
> 
> Build:
> https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/
> builds/2656

OK, but this is WK2-specific code. Why is the WK1 bot even building this code?

[Michael Catanzaro]

Full backtrace is:

_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
    ASSERTION FAILED: HashTraits<uint64_t>::emptyValue() != m_id && !HashTraits<uint64_t>::isDeletedValue(m_id)
    /Volumes/Data/slave/mojave-debug/build/Source/WebKit/Shared/CallbackID.h(55) : WebKit::CallbackID &WebKit::CallbackID::operator=(const WebKit::CallbackID &)
    1   0x113da92c9 WTFCrash
    2   0x118febfab WTFCrashWithInfo(int, char const*, char const*, int)
    3   0x11906a0ce WebKit::CallbackID::operator=(WebKit::CallbackID const&)
    4   0x1199f2e8c WTF::HashTraits<WebKit::CallbackID>::constructDeletedValue(WebKit::CallbackID&)
    5   0x1199f2b99 void WTF::HashTable<WebKit::CallbackID, WebKit::CallbackID, WTF::IdentityExtractor, WTF::CallbackIDHash, WTF::HashTraits<WebKit::CallbackID>, WTF::HashTraits<WebKit::CallbackID> >::checkKey<WTF::IdentityHashTranslator<WTF::HashTraits<WebKit::CallbackID>, WTF::CallbackIDHash>, WebKit::CallbackID>(WebKit::CallbackID const&)
    6   0x1199f287f WTF::HashTableAddResult<WTF::HashTableIterator<WebKit::CallbackID, WebKit::CallbackID, WTF::IdentityExtractor, WTF::CallbackIDHash, WTF::HashTraits<WebKit::CallbackID>, WTF::HashTraits<WebKit::CallbackID> > > WTF::HashTable<WebKit::CallbackID, WebKit::CallbackID, WTF::IdentityExtractor, WTF::CallbackIDHash, WTF::HashTraits<WebKit::CallbackID>, WTF::HashTraits<WebKit::CallbackID> >::add<WTF::IdentityHashTranslator<WTF::HashTraits<WebKit::CallbackID>, WTF::CallbackIDHash>, WebKit::CallbackID const&, WebKit::CallbackID const&>(WebKit::CallbackID const&&&, WebKit::CallbackID const&&&)
    7   0x1199f2833 WTF::HashTable<WebKit::CallbackID, WebKit::CallbackID, WTF::IdentityExtractor, WTF::CallbackIDHash, WTF::HashTraits<WebKit::CallbackID>, WTF::HashTraits<WebKit::CallbackID> >::add(WebKit::CallbackID const&)
    8   0x11996a564 WTF::HashSet<WebKit::CallbackID, WTF::CallbackIDHash, WTF::HashTraits<WebKit::CallbackID> >::add(WebKit::CallbackID const&)
    9   0x11996ac9b WebKit::WebPageProxy::getContentsAsString(WTF::Function<void (WTF::String const&, WebKit::CallbackBase::Error)>&&)
    10  0x119723711 -[WKWebView(WKPrivate) _getContentsAsStringWithCompletionHandler:]
    11  0x10ebdf8d4 WKNavigation_ProcessCrashDuringCallback_Test::TestBody()
    12  0x10ed91dbe void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
    13  0x10ed3977b void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
    14  0x10ed396a6 testing::Test::Run()
    15  0x10ed3b342 testing::TestInfo::Run()
    16  0x10ed3c9cc testing::TestCase::Run()
    17  0x10ed55d35 testing::internal::UnitTestImpl::RunAllTests()
    18  0x10ed95fbe bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
    19  0x10ed5575b bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
    20  0x10ed5561c testing::UnitTest::Run()
    21  0x10eb50071 RUN_ALL_TESTS()
    22  0x10eb50004 TestWebKitAPI::TestsController::run(int, char**)
    23  0x10ecf3c3e main
    24  0x7fff62d1eed9 start
TestWebKitAPI.WKNavigation.ProcessCrashDuringCallback Crashed

I guess it's intentional that the WK1 bot is running WK2 API tests?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #27)
>     1   0x113da92c9 WTFCrash
>     2   0x118febfab WTFCrashWithInfo(int, char const*, char const*, int)
>     3   0x11906a0ce WebKit::CallbackID::operator=(WebKit::CallbackID const&)
>     4   0x1199f2e8c
> WTF::HashTraits<WebKit::CallbackID>::constructDeletedValue(WebKit::
> CallbackID&)

Anyway, that's proof enough the assertions can't go in the assignment operator, so removing them was correct.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #2)
> I'll rebase the patch momentarily.
> 
> There's also this lovely warning that I don't know what to do with:
> 
> [1470/4720] Building C object
> Source/ThirdParty/libwebrtc...rty/usrsctp/usrsctplib/usrsctplib/netinet/
> sctp_output.c.o
> In file included from
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_output.c:46:
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_output.c: In function ‘send_forward_tsn’:
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_header.h:407:8: note: defined here
>   407 | struct sctp_forward_tsn_chunk {
>       |        ^~~~~~~~~~~~~~~~~~~~~~
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_header.h:418:8: note: defined here
>   418 | struct sctp_strseq_mid {
>       |        ^~~~~~~~~~~~~~~
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_output.c: In function ‘sctp_send_sack’:
> ../../Source/ThirdParty/libwebrtc/Source/third_party/usrsctp/usrsctplib/
> usrsctplib/netinet/sctp_header.h:265:8: note: defined here
>   265 | struct sctp_gap_ack_block {
>       |        ^~~~~~~~~~~~~~~~~~
> 
> I guess it's a GCC bug, because there is absolutely no warning there, only
> notes.

This one is fixed in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89985.