Make Element inherit from CanMakeWeakPtr and deploy it in HTMLFormElement and FormAssociatedElement code.
<rdar://problem/36082053>
Created attachment 366759 [details] Patch
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.
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
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
I don't think the test failure on Windows in IndexedDB is related to this.
Comment on attachment 366759 [details] Patch Nice! Please check perf bots after landing.
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()?
(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.
Created attachment 366795 [details] Patch for landing
Comment on attachment 366795 [details] Patch for landing Wait for EWS.
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.
(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()?
> > I like my name better. > > computeEmpty() doesn't sound right though. Maybe computeEmptiness()? Mine was 'computesEmpty()': if (foo.computesEmpty()) doBar();
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.
(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.
Created attachment 366801 [details] Patch for landing
Comment on attachment 366801 [details] Patch for landing Wait for EWS once more.
(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.
Maybe Darin or Sam has some idea.
Comment on attachment 366801 [details] Patch for landing Clearing flags on attachment: 366801 Committed r243929: <https://trac.webkit.org/changeset/243929>
All reviewed patches have been landed. Closing bug.
One option is to simply call it isEmpty() and assume people who use this type understand the performance characteristics.
This broke GTK and WPE builds
(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?
(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.
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.
(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?
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.
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.
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 tried to replace the #pragma once with an ifdef earlier, it didn't work either, but I can give it another try...
(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?
(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__
(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?
Committed r243941: <https://trac.webkit.org/changeset/243941>
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.
Created attachment 366831 [details] Patch
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.
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.
Comment on attachment 366831 [details] Patch We can't land this patch. Disabling unified builds has a serious perf impact.
(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)?
(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.
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.
(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.
(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....
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.
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.
Hm... I wonder if gcc is getting confused about friend declaration we have in WeakPtr.h.
Created attachment 366838 [details] Forward declare WeakHashSet in WeakPtr
(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.
Nice! That did the trick!
Created attachment 366840 [details] Patch for landing
Comment on attachment 366840 [details] Patch for landing Wait for EWS.
Committed r243954: <https://trac.webkit.org/changeset/243954>
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".
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.
Doing that cleanup & adding unit tests for computesEmpty & computeSize in https://bugs.webkit.org/show_bug.cgi?id=196669