Summary: | REGRESSION (r262994): Web Inspector is killed when context-clicking anywhere | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||
Component: | WebKit2 | Assignee: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | annulen, beidson, darin, ews-watchlist, gyuyoung.kim, hi, ryuan.choi, sergio, simon.fraser, thorton, w0nka, webkit-bug-importer, wenson_hsieh | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | Other | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=213278 | ||||||||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2020-06-15 18:07:03 PDT
Created attachment 401976 [details]
Patch v1 (WIP: help with SFINAE)
I've been trying to figure out the correct way to do SFINAE for the WTF::isValueEnum() template specialization for WebCore::ContextMenuAction, but it always conflicts with the specialization for IPC::MessageName (or the new specialization can't be found).
If feels like something silly I'm missing (like a #include <WebCore/ContextMenuItem.h>), but I can't figure it out.
Will sleep on it and look tomorrow morning. (Or I'll figure it out soon after I post this patch.)
(In reply to David Kilzer (:ddkilzer) from comment #1) > Created attachment 401976 [details] > Patch v1 (WIP: help with SFINAE) > > I've been trying to figure out the correct way to do SFINAE for the > WTF::isValueEnum() template specialization for WebCore::ContextMenuAction, > but it always conflicts with the specialization for IPC::MessageName (or the > new specialization can't be found). > > If feels like something silly I'm missing (like a #include > <WebCore/ContextMenuItem.h>), but I can't figure it out. > > Will sleep on it and look tomorrow morning. (Or I'll figure it out soon > after I post this patch.) Yep. I was missing #include <WebCore/ContextMenuItem.h> includes from: Source/WebKit/Platform/IPC/Decoder.h Source/WebKit/Platform/IPC/Encoder.h Created attachment 401979 [details]
Patch v2
(In reply to David Kilzer (:ddkilzer) from comment #3) > Created attachment 401979 [details] > Patch v2 Will write a test tomorrow to validate WebCore::ContextMenuAction values. Wanted to get this posted so EWS could chew on it, and give folks the option of landing this before the patch to unbreak Web Inspector. Comment on attachment 401979 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=401979&action=review > Source/WebKit/ChangeLog:13 > + * Platform/IPC/Decoder.h: > + * Platform/IPC/Encoder.h: > + - Include <WebCore/ContextMenuItem.h> since the definition of > + the custom WTF::isValidEnum<WebCore::ContextMenuAction> > + function is needed. Note that "MessageNames.h" is included in both of these headers for the same reason: for the WTF::isValidEnum<IPC::MessageName>() specialization. Comment on attachment 401979 [details] Patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=401979&action=review > Source/WebCore/platform/ContextMenuItem.cpp:245 > + case ContextMenuAction::ContextMenuItemBaseCustomTag ... ContextMenuAction::ContextMenuItemLastCustomTag: LOL...of course this won't work on MSVC++ since it's a gcc extension: C:\Buildbot\WinCairo-EWS\build\Source\WebCore\platform/ContextMenuItem.cpp(245): error C2143: syntax error: missing ':' before '...' C:\Buildbot\WinCairo-EWS\build\Source\WebCore\platform/ContextMenuItem.cpp(245): error C2059: syntax error: '...' Created attachment 401980 [details]
Patch v3
Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review > Source/WebCore/platform/ContextMenuItem.h:209 > +template<typename E, typename T, std::enable_if_t<std::is_same<E, WebCore::ContextMenuAction>::value>* = nullptr> Could use std::is_same_v. > Source/WebKit/Platform/IPC/Decoder.h:32 > +#include <WebCore/ContextMenuItem.h> Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. > Source/WebKit/Platform/IPC/Encoder.h:32 > +#include <WebCore/ContextMenuItem.h> Ditto. Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review >> Source/WebKit/Platform/IPC/Decoder.h:32 >> +#include <WebCore/ContextMenuItem.h> > > Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. What would you suggest? A separate header file? Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review >>> Source/WebKit/Platform/IPC/Decoder.h:32 >>> +#include <WebCore/ContextMenuItem.h> >> >> Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. > > What would you suggest? A separate header file? Before I can suggest a solution, I need to understand why this include is needed. Seems very strange that we need to include this *everywhere* Decoder is included. What does ContextMenuItem have to do with other uses of decoder? Created attachment 402026 [details]
Patch for landing
Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review >>>> Source/WebKit/Platform/IPC/Decoder.h:32 >>>> +#include <WebCore/ContextMenuItem.h> >>> >>> Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. >> >> What would you suggest? A separate header file? > > Before I can suggest a solution, I need to understand why this include is needed. Seems very strange that we need to include this *everywhere* Decoder is included. What does ContextMenuItem have to do with other uses of decoder? Ah, Source/WebKit/Shared/WebContextMenuItemData.cpp uses IPC::Decoder/Encoder, but the WTF::isValidEnum<WebCore::ContextMenuAction>() method must be declared before the Decoder.h/Encoder.h header is included, otherwise the compiler can't find the correct template specialization (this is for the decoder; there's another error for the encoder): In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: In file included from Source/WebKit/Shared/API/APINumber.h:30: Source/WebKit/Platform/IPC/Decoder.h:107:14: error: no matching function for call to 'isValidEnum' if (!WTF::isValidEnum<E>(value)) ^~~~~~~~~~~~~~~~~~~ In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:3: Source/WebKit/Shared/WebContextMenuItemData.cpp:118:18: note: in instantiation of function template specialization 'IPC::Decoder::decode<WebCore::ContextMenuAction, nullptr>' requested here if (!decoder.decode(action)) ^ In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: In file included from Source/WebKit/Shared/WebConnectionClient.cpp:29: In file included from Source/WebKit/Shared/API/APIObject.h:28: Debug/usr/local/include/wtf/EnumTraits.h:57:16: note: candidate template ignored: requirement '!HasCustomIsValidEnum<ContextMenuAction>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] constexpr bool isValidEnum(T t) ^ Debug/usr/local/include/wtf/EnumTraits.h:65:16: note: candidate template ignored: requirement 'std::is_same<std::underlying_type_t<ContextMenuAction>, bool>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] constexpr bool isValidEnum(T t) ^ In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: In file included from Source/WebKit/Shared/API/APINumber.h:30: In file included from Source/WebKit/Platform/IPC/Decoder.h:30: Debug/DerivedSources/WebKit2/MessageNames.h:3513:6: note: candidate template ignored: requirement 'std::is_same_v<WebCore::ContextMenuAction, IPC::MessageName>' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] bool isValidEnum(T messageName) ^ Comment on attachment 402026 [details]
Patch for landing
Oops, forgot to use #if ENABLE(CONTEXT_MENUS)/#endif in Tools/TestWebKitAPI/Tests/WebCore/ContextMenuAction.cpp.
Created attachment 402031 [details]
Patch for landing v2
Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review >>>>> Source/WebKit/Platform/IPC/Decoder.h:32 >>>>> +#include <WebCore/ContextMenuItem.h> >>>> >>>> Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. >>> >>> What would you suggest? A separate header file? >> >> Before I can suggest a solution, I need to understand why this include is needed. Seems very strange that we need to include this *everywhere* Decoder is included. What does ContextMenuItem have to do with other uses of decoder? > > Ah, Source/WebKit/Shared/WebContextMenuItemData.cpp uses IPC::Decoder/Encoder, but the WTF::isValidEnum<WebCore::ContextMenuAction>() method must be declared before the Decoder.h/Encoder.h header is included, otherwise the compiler can't find the correct template specialization (this is for the decoder; there's another error for the encoder): > > In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: > In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: > In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: > In file included from Source/WebKit/Shared/API/APINumber.h:30: > Source/WebKit/Platform/IPC/Decoder.h:107:14: error: no matching function for call to 'isValidEnum' > if (!WTF::isValidEnum<E>(value)) > ^~~~~~~~~~~~~~~~~~~ > In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:3: > Source/WebKit/Shared/WebContextMenuItemData.cpp:118:18: note: in instantiation of function template specialization 'IPC::Decoder::decode<WebCore::ContextMenuAction, nullptr>' requested here > if (!decoder.decode(action)) > ^ > In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: > In file included from Source/WebKit/Shared/WebConnectionClient.cpp:29: > In file included from Source/WebKit/Shared/API/APIObject.h:28: > Debug/usr/local/include/wtf/EnumTraits.h:57:16: note: candidate template ignored: requirement '!HasCustomIsValidEnum<ContextMenuAction>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] > constexpr bool isValidEnum(T t) > ^ > Debug/usr/local/include/wtf/EnumTraits.h:65:16: note: candidate template ignored: requirement 'std::is_same<std::underlying_type_t<ContextMenuAction>, bool>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] > constexpr bool isValidEnum(T t) > ^ > In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: > In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: > In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: > In file included from Source/WebKit/Shared/API/APINumber.h:30: > In file included from Source/WebKit/Platform/IPC/Decoder.h:30: > Debug/DerivedSources/WebKit2/MessageNames.h:3513:6: note: candidate template ignored: requirement 'std::is_same_v<WebCore::ContextMenuAction, IPC::MessageName>' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] > bool isValidEnum(T messageName) > ^ I think we can definitely find a way to make this work without including the header — will take some thought though. Comment on attachment 402031 [details]
Patch for landing v2
Setting cq+ since the layout tests failing on Windows aren't related to this change:
http/tests/security/cross-origin-clean-css-resource-timing.html
http/tests/security/cross-origin-css-resource-timing.html
Committed r263112: <https://trac.webkit.org/changeset/263112> All reviewed patches have been landed. Closing bug and clearing flags on attachment 402031 [details]. Comment on attachment 401980 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=401980&action=review >>>>>> Source/WebKit/Platform/IPC/Decoder.h:32 >>>>>> +#include <WebCore/ContextMenuItem.h> >>>>> >>>>> Including the entire header seems not good. Do we really need to do that? Would be good I think to follow up to make this unnecessary. >>>> >>>> What would you suggest? A separate header file? >>> >>> Before I can suggest a solution, I need to understand why this include is needed. Seems very strange that we need to include this *everywhere* Decoder is included. What does ContextMenuItem have to do with other uses of decoder? >> >> Ah, Source/WebKit/Shared/WebContextMenuItemData.cpp uses IPC::Decoder/Encoder, but the WTF::isValidEnum<WebCore::ContextMenuAction>() method must be declared before the Decoder.h/Encoder.h header is included, otherwise the compiler can't find the correct template specialization (this is for the decoder; there's another error for the encoder): >> >> In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: >> In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: >> In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: >> In file included from Source/WebKit/Shared/API/APINumber.h:30: >> Source/WebKit/Platform/IPC/Decoder.h:107:14: error: no matching function for call to 'isValidEnum' >> if (!WTF::isValidEnum<E>(value)) >> ^~~~~~~~~~~~~~~~~~~ >> In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:3: >> Source/WebKit/Shared/WebContextMenuItemData.cpp:118:18: note: in instantiation of function template specialization 'IPC::Decoder::decode<WebCore::ContextMenuAction, nullptr>' requested here >> if (!decoder.decode(action)) >> ^ >> In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: >> In file included from Source/WebKit/Shared/WebConnectionClient.cpp:29: >> In file included from Source/WebKit/Shared/API/APIObject.h:28: >> Debug/usr/local/include/wtf/EnumTraits.h:57:16: note: candidate template ignored: requirement '!HasCustomIsValidEnum<ContextMenuAction>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] >> constexpr bool isValidEnum(T t) >> ^ >> Debug/usr/local/include/wtf/EnumTraits.h:65:16: note: candidate template ignored: requirement 'std::is_same<std::underlying_type_t<ContextMenuAction>, bool>::value' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] >> constexpr bool isValidEnum(T t) >> ^ >> In file included from Debug/DerivedSources/WebKit2/unified-sources/UnifiedSource24.cpp:1: >> In file included from Source/WebKit/Shared/WebConnectionClient.cpp:30: >> In file included from Source/WebKit/Shared/API/c/WKSharedAPICast.h:30: >> In file included from Source/WebKit/Shared/API/APINumber.h:30: >> In file included from Source/WebKit/Platform/IPC/Decoder.h:30: >> Debug/DerivedSources/WebKit2/MessageNames.h:3513:6: note: candidate template ignored: requirement 'std::is_same_v<WebCore::ContextMenuAction, IPC::MessageName>' was not satisfied [with E = WebCore::ContextMenuAction, T = unsigned int] >> bool isValidEnum(T messageName) >> ^ > > I think we can definitely find a way to make this work without including the header — will take some thought though. Filed: Bug 213278: IPC/Decoder.h and IPC/Encoder.h need every WTF::EnumTraits<> specialization included before they are included *** Bug 213651 has been marked as a duplicate of this bug. *** |