Bug 213224

Summary: REGRESSION (r262994): Web Inspector is killed when context-clicking anywhere
Product: WebKit Reporter: David Kilzer (:ddkilzer) <ddkilzer>
Component: WebKit2Assignee: 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 Flags
Patch v1 (WIP: help with SFINAE)
none
Patch v2
none
Patch v3
darin: review+
Patch for landing
none
Patch for landing v2 none

Description David Kilzer (:ddkilzer) 2020-06-15 18:07:03 PDT
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>
Comment 1 David Kilzer (:ddkilzer) 2020-06-15 21:10:53 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.)
Comment 2 David Kilzer (:ddkilzer) 2020-06-15 21:26:36 PDT
(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
Comment 3 David Kilzer (:ddkilzer) 2020-06-15 21:47:17 PDT
Created attachment 401979 [details]
Patch v2
Comment 4 David Kilzer (:ddkilzer) 2020-06-15 21:48:23 PDT
(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 5 David Kilzer (:ddkilzer) 2020-06-15 21:51:43 PDT
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 6 David Kilzer (:ddkilzer) 2020-06-15 21:52:59 PDT
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: '...'
Comment 7 David Kilzer (:ddkilzer) 2020-06-15 21:55:18 PDT
Created attachment 401980 [details]
Patch v3
Comment 8 Darin Adler 2020-06-16 10:58:16 PDT
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 9 David Kilzer (:ddkilzer) 2020-06-16 11:03:31 PDT
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 10 Darin Adler 2020-06-16 11:04:50 PDT
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?
Comment 11 David Kilzer (:ddkilzer) 2020-06-16 11:30:27 PDT
Created attachment 402026 [details]
Patch for landing
Comment 12 David Kilzer (:ddkilzer) 2020-06-16 11:41:37 PDT
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 13 David Kilzer (:ddkilzer) 2020-06-16 11:45:05 PDT
Comment on attachment 402026 [details]
Patch for landing

Oops, forgot to use #if ENABLE(CONTEXT_MENUS)/#endif in Tools/TestWebKitAPI/Tests/WebCore/ContextMenuAction.cpp.
Comment 14 David Kilzer (:ddkilzer) 2020-06-16 11:45:29 PDT
Created attachment 402031 [details]
Patch for landing v2
Comment 15 Darin Adler 2020-06-16 12:07:17 PDT
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 16 David Kilzer (:ddkilzer) 2020-06-16 13:34:51 PDT
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
Comment 17 EWS 2020-06-16 13:42:18 PDT
Committed r263112: <https://trac.webkit.org/changeset/263112>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402031 [details].
Comment 18 David Kilzer (:ddkilzer) 2020-06-16 18:14:42 PDT
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
Comment 19 Alexey Proskuryakov 2020-06-29 18:13:27 PDT
*** Bug 213651 has been marked as a duplicate of this bug. ***