Bug 213224 – REGRESSION (r262994): Web Inspector is killed when context-clicking anywhere

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Web inspector is killed when context-clicking anywhere.

Caused by:
    Bug 211988: [IPC hardening] Check enum values in IPC::Decoder::decodeEnum() an IPC::Encoder::encodeEnum()
    <https://bugs.webkit.org/show_bug.cgi?id=211988>
    <https://trac.webkit.org/changeset/262994/webkit>

<rdar://problem/64383320>

[David Kilzer (:ddkilzer)]

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.)

[David Kilzer (:ddkilzer)]

(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

[David Kilzer (:ddkilzer)]

Created attachment 401979 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(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.

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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: '...'

[David Kilzer (:ddkilzer)]

Created attachment 401980 [details]
Patch v3

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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?

[Darin Adler]

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?

[David Kilzer (:ddkilzer)]

Created attachment 402026 [details]
Patch for landing

[David Kilzer (:ddkilzer)]

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)
     ^

[David Kilzer (:ddkilzer)]

Comment on attachment 402026 [details]
Patch for landing

Oops, forgot to use #if ENABLE(CONTEXT_MENUS)/#endif in Tools/TestWebKitAPI/Tests/WebCore/ContextMenuAction.cpp.

[David Kilzer (:ddkilzer)]

Created attachment 402031 [details]
Patch for landing v2

[Darin Adler]

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.

[David Kilzer (:ddkilzer)]

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

[EWS]

Committed r263112: <https://trac.webkit.org/changeset/263112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402031 [details].

[David Kilzer (:ddkilzer)]

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

[Alexey Proskuryakov]

*** Bug 213651 has been marked as a duplicate of this bug. ***