Bug 196626 – Make WeakPtr<Element> possible and deploy it in form associated elements code

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Make Element inherit from CanMakeWeakPtr and deploy it in HTMLFormElement and FormAssociatedElement code.

[Ryosuke Niwa]

<rdar://problem/36082053>

[Ryosuke Niwa]

Created attachment 366759 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 366759 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.h:180
>      Vector<FormAssociatedElement*> m_associatedElements;

We should be using WeakPtr here too but that change is a bit involved so I'll do that in a separate patch.

[EWS Watchlist]

Comment on attachment 366759 [details]
Patch

Attachment 366759 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11770438

New failing tests:
storage/indexeddb/modern/gc-closes-database-private.html

[EWS Watchlist]

Created attachment 366771 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Ryosuke Niwa]

I don't think the test failure on Windows in IndexedDB is related to this.

[Antti Koivisto]

Comment on attachment 366759 [details]
Patch

Nice! Please check perf bots after landing.

[Antti Koivisto]

Comment on attachment 366759 [details]
Patch

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

> Source/WebCore/html/HTMLFormElement.cpp:569
> -    if (m_invalidAssociatedFormControls.isEmpty())
> +    if (!m_invalidAssociatedFormControls.computeSize())

It would be nice to have a version of isEmpty() to avoid reversing existing logic. Maybe computesEmpty()?

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 366759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366759&action=review
> 
> > Source/WebCore/html/HTMLFormElement.cpp:569
> > -    if (m_invalidAssociatedFormControls.isEmpty())
> > +    if (!m_invalidAssociatedFormControls.computeSize())
> 
> It would be nice to have a version of isEmpty() to avoid reversing existing
> logic. Maybe computesEmpty()?

Oh, thanks for reminding me of that. I was thinking of doing it last night and forgot to do.

[Ryosuke Niwa]

Created attachment 366795 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 366795 [details]
Patch for landing

Wait for EWS.

[Antti Koivisto]

Comment on attachment 366795 [details]
Patch for landing

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

> Source/WTF/wtf/WeakHashSet.h:114
> +    bool computeIsEmpty() const

I like my name better.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #12)
> Comment on attachment 366795 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366795&action=review
> 
> > Source/WTF/wtf/WeakHashSet.h:114
> > +    bool computeIsEmpty() const
> 
> I like my name better.

computeEmpty() doesn't sound right though. Maybe computeEmptiness()?

[Antti Koivisto]

> > I like my name better.
> 
> computeEmpty() doesn't sound right though. Maybe computeEmptiness()?

Mine was 'computesEmpty()':

if (foo.computesEmpty())
   doBar();

[Ryosuke Niwa]

Comment on attachment 366795 [details]
Patch for landing

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

> Source/WTF/wtf/WeakHashSet.h:119
> +        const_cast<WeakReferenceSet&>(m_set).removeIf([] (auto& value) { return !value->get(); });
> +        return m_set.isEmpty();

Ugh... not sure what I was thinking. This is no better than just calling computeSize().
I want to find the first non-released value instead.

[Ryosuke Niwa]

(In reply to Antti Koivisto from comment #14)
> > > I like my name better.
> > 
> > computeEmpty() doesn't sound right though. Maybe computeEmptiness()?
> 
> Mine was 'computesEmpty()':
> 
> if (foo.computesEmpty())
>    doBar();

That still doesn't read right to me but I guess we can land it as is, and change it later if someone comes up with a better name.

[Ryosuke Niwa]

Created attachment 366801 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 366801 [details]
Patch for landing

Wait for EWS once more.

[Ryosuke Niwa]

(In reply to Ryosuke Niwa from comment #16)
> (In reply to Antti Koivisto from comment #14)
> > > > I like my name better.
> > > 
> > > computeEmpty() doesn't sound right though. Maybe computeEmptiness()?
> > 
> > Mine was 'computesEmpty()':
> > 
> > if (foo.computesEmpty())
> >    doBar();
> 
> That still doesn't read right to me but I guess we can land it as is, and
> change it later if someone comes up with a better name.

By "land it as is", I mean just call it computesEmpty for now.

[Ryosuke Niwa]

Maybe Darin or Sam has some idea.

[Ryosuke Niwa]

Comment on attachment 366801 [details]
Patch for landing

Clearing flags on attachment: 366801

Committed r243929: <https://trac.webkit.org/changeset/243929>

[Ryosuke Niwa]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

One option is to simply call it isEmpty() and assume people who use this type understand the performance characteristics.

[Sergio Villar Senin]

This broke GTK and WPE builds

[Darin Adler]

(In reply to Sergio Villar Senin from comment #24)
> This broke GTK and WPE builds

Am I correct in understanding the problem you are mentioning was actually the patch from bug 366655, not this change? Or is there still a problem with this change?

[Ryosuke Niwa]

(In reply to Sergio Villar Senin from comment #24)
> This broke GTK and WPE builds

Looks like HTMLFormElement.h needs to include WeakPtr.h. The unified compilation strikes again. In my defense, GTK+ wasn't building without this patch either when I landed this place patch, so I didn't know if this patch would have introduced a new failure or not.

[Philippe Normand]

FAILED: Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp.o 
/usr/lib/ccache/c++   -DBUILDING_GTK__=1 -DBUILDING_WEBKIT -DBUILDING_WITH_CMAKE=1 -DBUILDING_WebCore -DBWRAP_EXECUTABLE=\"/usr/bin/bwrap\" -DDBUS_PROXY_EXECUTABLE=\"/home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/DependenciesGTK/Root/bin/xdg-dbus-proxy\" -DGETTEXT_PACKAGE=\"WebKit2GTK-4.0\" -DHAVE_CONFIG_H=1 -DJSC_GLIB_API_ENABLED -DSTATICALLY_LINKED_WITH_PAL=1 -DWEBKITGTK_API_VERSION_STRING=\"4.0\" -I. -I../../Source/WebCore -I../../Source/WebCore/Modules/airplay -I../../Source/WebCore/Modules/applepay -I../../Source/WebCore/Modules/applepay/paymentrequest -I../../Source/WebCore/Modules/applicationmanifest -I../../Source/WebCore/Modules/beacon -I../../Source/WebCore/Modules/cache -I../../Source/WebCore/Modules/credentialmanagement -I../../Source/WebCore/Modules/encryptedmedia -I../../Source/WebCore/Modules/encryptedmedia/legacy -I../../Source/WebCore/Modules/entriesapi -I../../Source/WebCore/Modules/fetch -I../../Source/WebCore/Modules/geolocation -I../../Source/WebCore/Modules/indexeddb -I../../Source/WebCore/Modules/indexeddb/client -I../../Source/WebCore/Modules/indexeddb/server -I../../Source/WebCore/Modules/indexeddb/shared -I../../Source/WebCore/Modules/mediacapabilities -I../../Source/WebCore/Modules/mediacontrols -I../../Source/WebCore/Modules/mediarecorder -I../../Source/WebCore/Modules/mediasession -I../../Source/WebCore/Modules/mediasource -I../../Source/WebCore/Modules/mediastream -I../../Source/WebCore/Modules/mediastream/libwebrtc -I../../Source/WebCore/Modules/navigatorcontentutils -I../../Source/WebCore/Modules/notifications -I../../Source/WebCore/Modules/paymentrequest -I../../Source/WebCore/Modules/plugins -I../../Source/WebCore/Modules/quota -I../../Source/WebCore/Modules/speech -I../../Source/WebCore/Modules/streams -I../../Source/WebCore/Modules/webaudio -I../../Source/WebCore/Modules/webauthn -I../../Source/WebCore/Modules/webauthn/cbor -I../../Source/WebCore/Modules/webauthn/fido -I../../Source/WebCore/Modules/webdatabase -I../../Source/WebCore/Modules/webdriver -I../../Source/WebCore/Modules/webgpu -I../../Source/WebCore/Modules/websockets -I../../Source/WebCore/Modules/webvr -I../../Source/WebCore/accessibility -I../../Source/WebCore/accessibility/isolatedtree -I../../Source/WebCore/animation -I../../Source/WebCore/bindings -I../../Source/WebCore/bindings/js -I../../Source/WebCore/bridge -I../../Source/WebCore/bridge/c -I../../Source/WebCore/bridge/jsc -I../../Source/WebCore/contentextensions -I../../Source/WebCore/crypto -I../../Source/WebCore/crypto/algorithms -I../../Source/WebCore/crypto/keys -I../../Source/WebCore/crypto/parameters -I../../Source/WebCore/css -I../../Source/WebCore/css/parser -I../../Source/WebCore/css/typedom -I../../Source/WebCore/cssjit -I../../Source/WebCore/dom -I../../Source/WebCore/dom/messageports -I../../Source/WebCore/domjit -I../../Source/WebCore/editing -I../../Source/WebCore/fileapi -I../../Source/WebCore/history -I../../Source/WebCore/html -I../../Source/WebCore/html/canvas -I../../Source/WebCore/html/forms -I../../Source/WebCore/html/parser -I../../Source/WebCore/html/shadow -I../../Source/WebCore/html/track -I../../Source/WebCore/inspector -I../../Source/WebCore/inspector/agents -I../../Source/WebCore/inspector/agents/page -I../../Source/WebCore/inspector/agents/worker -I../../Source/WebCore/layout -I../../Source/WebCore/layout/blockformatting -I../../Source/WebCore/layout/displaytree -I../../Source/WebCore/layout/floats -I../../Source/WebCore/layout/inlineformatting -I../../Source/WebCore/layout/inlineformatting/text -I../../Source/WebCore/layout/layouttree -I../../Source/WebCore/loader -I../../Source/WebCore/loader/appcache -I../../Source/WebCore/loader/archive -I../../Source/WebCore/loader/archive/mhtml -I../../Source/WebCore/loader/cache -I../../Source/WebCore/loader/icon -I../../Source/WebCore/mathml -I../../Source/WebCore/page -I../../Source/WebCore/page/animation -I../../Source/WebCore/page/csp -I../../Source/WebCore/page/scrolling -I../../Source/WebCore/platform -I../../Source/WebCore/platform/animation -I../../Source/WebCore/platform/audio -I../../Source/WebCore/platform/encryptedmedia -I../../Source/WebCore/platform/gamepad -I../../Source/WebCore/platform/graphics -I../../Source/WebCore/platform/graphics/cpu/arm -I../../Source/WebCore/platform/graphics/cpu/arm/filters -I../../Source/WebCore/platform/graphics/displaylists -I../../Source/WebCore/platform/graphics/filters -I../../Source/WebCore/platform/graphics/iso -I../../Source/WebCore/platform/graphics/opentype -I../../Source/WebCore/platform/graphics/transforms -I../../Source/WebCore/platform/mediacapabilities -I../../Source/WebCore/platform/mediarecorder -I../../Source/WebCore/platform/mediasession -I../../Source/WebCore/platform/mediastream -I../../Source/WebCore/platform/mediastream/libwebrtc -I../../Source/WebCore/platform/mock -I../../Source/WebCore/platform/mock/mediasource -I../../Source/WebCore/platform/network -I../../Source/WebCore/platform/sql -I../../Source/WebCore/platform/text -I../../Source/WebCore/platform/vr -I../../Source/WebCore/plugins -I../../Source/WebCore/rendering -I../../Source/WebCore/rendering/line -I../../Source/WebCore/rendering/mathml -I../../Source/WebCore/rendering/shapes -I../../Source/WebCore/rendering/style -I../../Source/WebCore/rendering/svg -I../../Source/WebCore/rendering/updating -I../../Source/WebCore/replay -I../../Source/WebCore/storage -I../../Source/WebCore/style -I../../Source/WebCore/svg -I../../Source/WebCore/svg/animation -I../../Source/WebCore/svg/graphics -I../../Source/WebCore/svg/graphics/filters -I../../Source/WebCore/svg/properties -I../../Source/WebCore/websockets -I../../Source/WebCore/workers -I../../Source/WebCore/workers/service -I../../Source/WebCore/workers/service/context -I../../Source/WebCore/workers/service/server -I../../Source/WebCore/worklets -I../../Source/WebCore/xml -I../../Source/WebCore/xml/parser -IDerivedSources/WebCore -IDerivedSources/ForwardingHeaders/ANGLE -I../../Source/WebCore/platform/graphics/gpu -I../../Source/ThirdParty/xdgmime/src -I../../Source/WebCore/platform/graphics/cairo -I../../Source/WebCore/platform/graphics/freetype -I../../Source/WebCore/platform/graphics/harfbuzz -I../../Source/WebCore/platform/graphics/harfbuzz/ng -I../../Source/WebCore/platform/graphics/gstreamer -I../../Source/WebCore/platform/graphics/gstreamer/mse -I../../Source/WebCore/platform/graphics/gstreamer/eme -I../../Source/WebCore/platform/audio/gstreamer -I../../Source/WebCore/platform/encryptedmedia/clearkey -I../../Source/WebCore/platform/image-decoders -I../../Source/WebCore/platform/image-decoders/bmp -I../../Source/WebCore/platform/image-decoders/gif -I../../Source/WebCore/platform/image-decoders/ico -I../../Source/WebCore/platform/image-decoders/jpeg -I../../Source/WebCore/platform/image-decoders/jpeg2000 -I../../Source/WebCore/platform/image-decoders/png -I../../Source/WebCore/platform/image-decoders/webp -I../../Source/WebCore/platform/graphics/texmap -I../../Source/WebCore/page/scrolling/nicosia -I../../Source/WebCore/platform/graphics/texmap/coordinated -I../../Source/WebCore/platform/graphics/nicosia -I../../Source/WebCore/platform/graphics/nicosia/cairo -I../../Source/WebCore/platform/graphics/nicosia/texmap -I../../Source/ThirdParty/ANGLE -I../../Source/ThirdParty/ANGLE/include/KHR -I../../Source/WebCore/accessibility/atk -I../../Source/WebCore/editing/atk -I../../Source/WebCore/page/gtk -I../../Source/WebCore/platform/generic -I../../Source/WebCore/platform/gtk -I../../Source/WebCore/platform/graphics/egl -I../../Source/WebCore/platform/graphics/glx -I../../Source/WebCore/platform/graphics/gtk -I../../Source/WebCore/platform/graphics/opengl -I../../Source/WebCore/platform/graphics/wayland -I../../Source/WebCore/platform/graphics/x11 -I../../Source/WebCore/platform/mediastream/gtk -I../../Source/WebCore/platform/mediastream/gstreamer -I../../Source/WebCore/platform/network/gtk -I../../Source/WebCore/platform/network/soup -I../../Source/WebCore/platform/text/gtk -I../../Source/WebCore/bindings/gobject -isystem ../DependenciesGTK/Root/include/libxml2 -isystem ../../Source/ThirdParty/libwebrtc/Source -isystem ../../Source/ThirdParty/libwebrtc/Source/webrtc -isystem ../../Source/ThirdParty/libwebrtc/Source/third_party/abseil-cpp -isystem ../DependenciesGTK/Root/include/cairo -isystem ../DependenciesGTK/Root/include/freetype2 -isystem ../DependenciesGTK/Root/include/harfbuzz -isystem ../DependenciesGTK/Root/include/gstreamer-1.0 -isystem ../DependenciesGTK/Root/include/glib-2.0 -isystem ../DependenciesGTK/Root/lib/glib-2.0/include -isystem ../DependenciesGTK/Root/include/orc-0.4 -isystem ../DependenciesGTK/Root/lib/gstreamer-1.0/include -isystem /usr/include/libdrm -isystem ../DependenciesGTK/Root/include/openjpeg-2.3 -isystem ../DependenciesGTK/Root/include/atk-1.0 -isystem /usr/include/enchant -isystem ../DependenciesGTK/Root/include/gio-unix-2.0 -isystem ../DependenciesGTK/Root/include/libsecret-1 -isystem ../DependenciesGTK/Root/include/libsoup-2.4 -IDerivedSources/ForwardingHeaders -I../../Source/bmalloc -IDerivedSources -I../../Source/ThirdParty -fdiagnostics-color=always -Wextra -Wall -Wno-attributes -Wno-psabi -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align  -fno-strict-aliasing -fno-exceptions -fno-rtti -std=c++14 -O3 -DNDEBUG -fPIC -MD -MT Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp.o -MF Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp.o.d -o Source/WebCore/CMakeFiles/WebCore.dir/__/__/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp.o -c /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp
In file included from ../../Source/WebCore/html/HTMLFormElement.h:32:0,
                 from DerivedSources/WebCore/JSHTMLFormElement.h:23,
                 from DerivedSources/WebCore/JSDOMFormData.cpp:39,
                 from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp:3:
DerivedSources/ForwardingHeaders/wtf/WeakHashSet.h:151:12: error: ‘WeakHashSet’ is already declared in this scope
 using WTF::WeakHashSet;
            ^~~~~~~~~~~

It seems the compiler gets confused between WeakHashSet.h in Source/WTF/wtf and the one in ForwardingHeaders.

[Ryosuke Niwa]

(In reply to Philippe Normand from comment #27)
> DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp
> In file included from ../../Source/WebCore/html/HTMLFormElement.h:32:0,
>                  from DerivedSources/WebCore/JSHTMLFormElement.h:23,
>                  from DerivedSources/WebCore/JSDOMFormData.cpp:39,
>                  from
> /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/
> DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp:3:
> DerivedSources/ForwardingHeaders/wtf/WeakHashSet.h:151:12: error:
> ‘WeakHashSet’ is already declared in this scope
>  using WTF::WeakHashSet;
>             ^~~~~~~~~~~
> 
> It seems the compiler gets confused between WeakHashSet.h in Source/WTF/wtf
> and the one in ForwardingHeaders.

Yeah, just saw that. r243931 added WeakHashSet already. Not sure why this only happens to WeakHashSet... Maybe it's forwarded differently from other headers?

[Ryosuke Niwa]

I guess one band aid we can land is to do a manual if-def check instead of pragma once until someone who has access to GTK+ / WPE can figure out what's up.

[Philippe Normand]

I'm quite puzzled :(

I've got a local WPE build fix, by adding a forward declaration of WeakHashSet in wtf/Forward.h, moving the real include to the FormElement.cpp file and disabling unity build for that file. I'm checking the GTK build with this change now.

[Ryosuke Niwa]

Disabling unified build for FormElements.cpp would be strictly worse than the workaround I suggested. We'd also need to redo all perf qualification if we did that.

[Philippe Normand]

I tried to replace the #pragma once with an ifdef earlier, it didn't work either, but I can give it another try...

[Ryosuke Niwa]

(In reply to Philippe Normand from comment #32)
> I tried to replace the #pragma once with an ifdef earlier, it didn't work
> either, but I can give it another try...

Also get rid of it in Forward.h?

[Philippe Normand]

(In reply to Ryosuke Niwa from comment #33)
> (In reply to Philippe Normand from comment #32)
> > I tried to replace the #pragma once with an ifdef earlier, it didn't work
> > either, but I can give it another try...
> 
> Also get rid of it in Forward.h?

Yes, same error:
In file included from ../../Source/WebCore/html/HTMLFormElement.h:32:0,
                 from DerivedSources/WebCore/JSHTMLFormElement.h:23,
                 from DerivedSources/WebCore/JSHTMLElementWrapperFactory.cpp:54,
                 from /app/webkit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-38.cpp:2:
DerivedSources/ForwardingHeaders/wtf/WeakHashSet.h:152:12: error: ‘WeakHashSet’ is already declared in this scope
 using WTF::WeakHashSet;
            ^~~~~~~~~~~


diff --git a/Source/WTF/wtf/WeakHashSet.h b/Source/WTF/wtf/WeakHashSet.h
index c3902329ad1..ef8c682726d 100644
--- a/Source/WTF/wtf/WeakHashSet.h
+++ b/Source/WTF/wtf/WeakHashSet.h
@@ -23,7 +23,8 @@
  * THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#pragma once
+#ifndef __WTF_WEAK_HASH_SET__
+#define __WTF_WEAK_HASH_SET__
 
 #include <wtf/HashSet.h>
 #include <wtf/HashTraits.h>
@@ -149,3 +150,5 @@ template<typename T> struct HashTraits<Ref<WeakReference<T>>> : RefHashTraits<We
 } // namespace WTF
 
 using WTF::WeakHashSet;
+
+#endif // __WTF_WEAK_HASH_SET__

[Philippe Normand]

(In reply to Ryosuke Niwa from comment #31)
> Disabling unified build for FormElements.cpp would be strictly worse than
> the workaround I suggested. We'd also need to redo all perf qualification if
> we did that.

I don't know what you mean with "perf qualification".
I have a build fix for WPE and GTK, as described in comment 30. If you don't want this fix, can you please rollout the patch?

[Michael Catanzaro]

Committed r243941: <https://trac.webkit.org/changeset/243941>

[Michael Catanzaro]

Folks, please roll out sooner next time when the proper fix is unclear or difficult. Please don't leave the bots broken for 12 revisions; that makes it really hard to discover regressions introduced in that range. I've done this rollout now to make sure we don't stay broken over the weekend.

Phil has a proposed fix but it involves @no-unify for HTMLInputElement.cpp, so I'm nervous that could break Mac. We'll need to test on EWS. I'll upload it momentarily.

[Michael Catanzaro]

Created attachment 366831 [details]
Patch

[Darin Adler]

I’m not thrilled with any solution that requires @no-unify or that requires we use header guards rather than #pragma once. Long term both are not the directions we want to go for the WebKit project.

I worry that we are making changes without understanding what’s going wrong.

> It seems the compiler gets confused between WeakHashSet.h in Source/WTF/wtf
> and the one in ForwardingHeaders.

We need to make sure that those two are never both included in the same compilation unit. In which compilation does that occur? How do we end up including both? What’s the tree of "what includes what"? This investigation is best done by someone who can compile on their own computer, not in EWS. Generally when compiling with forwarding headers, we can’t have a mix of the forwarding headers and other "real" headers from the same project, and if we do, we need to figure out exactly why.

When I originally created ForwardingHeaders, those forwarding headers contained only include statements. But I think the modern version involves copying the headers. That is risky because then #pragma once definitely won’t work if we ever include both. But despite that, prefer not to solve this by adding old fashioned header guards.

[Michael Catanzaro]

BTW I'm pretty sure we've lost a lot more developer time thinking about and trying to fix unified build issues than we've actually gained in cycles saved. At least when WebKit is building we can do other things and not waste manpower. I'd be willing to wait 40% longer for builds if we could once again easily land patches that add or remove a #include (or, worse, an entire file). We really underestimated how hard unified builds would make simple things. :(

Anyway, Darin, I haven't looked into this yet -- just uploaded Ryosuke's original patch with Phil's suggested changes -- but I'll see if I can build Ryosuke's original patch on its own later today to see what exactly is causing the WeakHashSet.h and Forward.h to be included in the same place. FWIW I think we've changed the forwarding headers to be copies in the CMake build just to parallel XCode, which has used copies for a long time; we used to instead #include the real header from the forwarding header.

[Ryosuke Niwa]

Comment on attachment 366831 [details]
Patch

We can't land this patch. Disabling unified builds has a serious perf impact.

[Michael Catanzaro]

(In reply to Ryosuke Niwa from comment #41)
> Comment on attachment 366831 [details]
> Patch
> 
> We can't land this patch. Disabling unified builds has a serious perf impact.

Do you mean at runtime (surely not?) or at build time (true, but often very difficult to avoid)?

[Ryosuke Niwa]

(In reply to Michael Catanzaro from comment #42)
> (In reply to Ryosuke Niwa from comment #41)
> > Comment on attachment 366831 [details]
> > Patch
> > 
> > We can't land this patch. Disabling unified builds has a serious perf impact.
> 
> Do you mean at runtime (surely not?) or at build time (true, but often very
> difficult to avoid)?

Runtime. When a file is excluded from unified builds, some functions would no longer get inlined or de-virtualized. As a result, things can get slower.

[Michael Catanzaro]

And that's significant enough to show up in perf?

Anyway, Phil, your patch doesn't link on the Mac EWS. So that's not good.

I can't reproduce the build failure locally (without the rollout), although the rollout did fix the GTK bots (but the WPE release bot is still broken). Unfortunately the error message from the compiler on the bots is insufficient to debug:

In file included from ../../Source/WebCore/html/HTMLFormElement.h:32:0,
                 from DerivedSources/WebCore/JSHTMLFormElement.h:23,
                 from DerivedSources/WebCore/JSDOMFormData.cpp:39,
                 from /home/slave/webkitgtk/gtk-linux-64-release/build/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource-3a52ce78-18.cpp:3:
DerivedSources/ForwardingHeaders/wtf/WeakHashSet.h:151:12: error: ‘WeakHashSet’ is already declared in this scope
 using WTF::WeakHashSet;
            ^~~~~~~~~~~

That shows us the other declaration of WeakHashSet in the forwarding header, as we would expect, but it doesn't show us where the other declaration is coming from, as I would have expected. Unless the compiler tells us, it's really impossible to know. Since Phil was able to reproduce this locally, we'll have to wait for him to report back on Monday.

[Ryosuke Niwa]

(In reply to Michael Catanzaro from comment #44)
> And that's significant enough to show up in perf?

Yes, in some cases.

(In reply to Darin Adler from comment #39)
>
> We need to make sure that those two are never both included in the same
> compilation unit. In which compilation does that occur? How do we end up
> including both? What’s the tree of "what includes what"? This investigation
> is best done by someone who can compile on their own computer, not in EWS.
> Generally when compiling with forwarding headers, we can’t have a mix of the
> forwarding headers and other "real" headers from the same project, and if we
> do, we need to figure out exactly why.
> 
> When I originally created ForwardingHeaders, those forwarding headers
> contained only include statements. But I think the modern version involves
> copying the headers. That is risky because then #pragma once definitely
> won’t work if we ever include both. But despite that, prefer not to solve
> this by adding old fashioned header guards.

It appears to be happening in DerivedSources/WebCore/HTMLElementFactory.cpp and DerivedSources/WebCore/JSDOMFormData.cpp so things in derived sources.

[Michael Catanzaro]

(In reply to Ryosuke Niwa from comment #45)
> It appears to be happening in DerivedSources/WebCore/HTMLElementFactory.cpp
> and DerivedSources/WebCore/JSDOMFormData.cpp so things in derived sources.

They should both be seeing the forwarding header DerivedSources/ForwardingHeaders/wtf/WeakHashSet.h though, which is guarded by #pragma once. The actual WTF source directories are not in include dirs either, which you can verify in comment #27, so nothing should be able to pull in the original header. So without being able to reproduce, I don't think I can do more here....

[Ryosuke Niwa]

I also don't understand what's so special about WeakHashSet. HTMLFormElement.h pulls in other wtf header files like WeakPtr.h I don't see why WeakHashSet.h is so special.

[Ryosuke Niwa]

Created attachment 366837 [details]
WeakHashSet.h in HTMLFormElement.h

From the error, it looks like just adding WeakHashSet.h to HTMLFormElement.h is enough to reproduce the issue.
Let's try that.

[Ryosuke Niwa]

Hm... I wonder if gcc is getting confused about friend declaration we have in WeakPtr.h.

[Ryosuke Niwa]

Created attachment 366838 [details]
Forward declare WeakHashSet in WeakPtr

[Michael Catanzaro]

(In reply to Ryosuke Niwa from comment #49)
> Hm... I wonder if gcc is getting confused about friend declaration we have
> in WeakPtr.h.

Oooh, maybe; let's hope you're right. And thanks for investigating.

I tested locally and adding WeakHashSet.h to HTMLFormElement.h didn't break for me, but as you just saw it did break the EWS. So a GCC version difference seems pretty plausible.

[Ryosuke Niwa]

Nice! That did the trick!

[Ryosuke Niwa]

Created attachment 366840 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 366840 [details]
Patch for landing

Wait for EWS.

[Ryosuke Niwa]

Committed r243954: <https://trac.webkit.org/changeset/243954>

[Darin Adler]

Comment on attachment 366840 [details]
Patch for landing

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

> Source/WTF/wtf/WeakPtr.h:36
> +template<typename U> class WeakHashSet;

For future reference, this can be written without a name:

    template<typename> class WeakHashSet;

So can the ones below and the template friend cases below too. Basically, an unused template argument name can be omitted, and should be if the name is not meaningful like "T" or "U".

[Ryosuke Niwa]

Comment on attachment 366840 [details]
Patch for landing

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

>> Source/WTF/wtf/WeakPtr.h:36
>> +template<typename U> class WeakHashSet;
> 
> For future reference, this can be written without a name:
> 
>     template<typename> class WeakHashSet;
> 
> So can the ones below and the template friend cases below too. Basically, an unused template argument name can be omitted, and should be if the name is not meaningful like "T" or "U".

That's a good point. We should probably clean it up elsewhere too.
Looks like most of template forward declarations are naming types right now.

[Ryosuke Niwa]

Doing that cleanup & adding unit tests for computesEmpty & computeSize in https://bugs.webkit.org/show_bug.cgi?id=196669