REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()
Created attachment 451953 [details] Patch
<rdar://problem/88696673>
Comment on attachment 451953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451953&action=review > Source/WebCore/bindings/js/JSEventListener.cpp:49 > +JSEventListener::JSEventListener(JSObject* function, JSObject* wrapper, bool isAttribute, bool wasCreatedFromMarkup, DOMWrapperWorld& isolatedWorld) nit: Can we make an enum for this instead of using a boolean? That way, the call sites will be more readable Maybe: "enum class CreatedFromMarkup { Yes, No }" > Source/WebCore/bindings/js/JSEventListener.h:63 > + static bool wasCreatedFromMarkup(Ref<EventListener>&& eventListener) Taking an rvalue and not taking ownership over it is an anti pattern. Can this just be "const EventListener&" or "EventListener&"? > LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html:19 > + console.log(`PASS: clicked is 1`); nit: why not use something like testPassed here and testFailed instead of console.log.
Comment on attachment 451953 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451953&action=review > Source/WebCore/ChangeLog:12 > + This change fixes inline event handler check by introducing m_wasCreatedFromMarkup what exactly does "m_wasCreatedFromMarkup" mean in this context?
Created attachment 452080 [details] Patch Fix ASSERT in SVGElement, replace bool with enum, remove rvalue parameter, fix condition in EventTarget.cpp, and clarify ChangeLog.
(In reply to Saam Barati from comment #3) > Comment on attachment 451953 [details] > Patch Appreciate your feedback, Saam! > > Source/WebCore/bindings/js/JSEventListener.cpp:49 > > +JSEventListener::JSEventListener(JSObject* function, JSObject* wrapper, bool isAttribute, bool wasCreatedFromMarkup, DOMWrapperWorld& isolatedWorld) > > nit: Can we make an enum for this instead of using a boolean? That way, the > call sites will be more readable > > Maybe: "enum class CreatedFromMarkup { Yes, No }" Done, we should probably do the same for `bool isAttribute`. > > Source/WebCore/bindings/js/JSEventListener.h:63 > > + static bool wasCreatedFromMarkup(Ref<EventListener>&& eventListener) > > Taking an rvalue and not taking ownership over it is an anti pattern. Can > this just be "const EventListener&" or "EventListener&"? Nice, changed. No reason to worry about performance of EventListener&& vs const EventListener&, especially on M1. > > LayoutTests/http/tests/security/contentSecurityPolicy/inline-event-handler-allowed-after-being-replaced.html:19 > > + console.log(`PASS: clicked is 1`); > > nit: why not use something like testPassed here and testFailed instead of > console.log. Sadly, we can't do that in tests where CSP is enabled, since testPassed() harness does a lot evals even on initialization. (In reply to Saam Barati from comment #4) > > Source/WebCore/ChangeLog:12 > > + This change fixes inline event handler check by introducing m_wasCreatedFromMarkup > > what exactly does "m_wasCreatedFromMarkup" mean in this context? created from markup = inline event handler. Basically, <body onload="foo()"> stuff. Clarified ChangeLog on that.
Comment on attachment 452080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452080&action=review > Source/WebCore/bindings/js/JSEventListener.h:77 > + enum class CreatedFromMarkup : bool { Yes, No }; Please reverse so that No=0 and Yes=1. It would make more sense.
Comment on attachment 452080 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452080&action=review > Source/WebCore/bindings/js/JSEventListener.cpp:53 > + , m_isInitialized(false) Can this use inline initialization in the header instead?
(In reply to Chris Dumez from comment #8) > Comment on attachment 452080 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452080&action=review > > > Source/WebCore/bindings/js/JSEventListener.cpp:53 > > + , m_isInitialized(false) > > Can this use inline initialization in the header instead? Not when we do bit field packing I guess.
Created attachment 452082 [details] Patch CreatedFromMarkup: reverse so that No=0 and Yes=1.
(In reply to Alexey Shvayka from comment #9) > (In reply to Chris Dumez from comment #8) > > Comment on attachment 452080 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=452080&action=review > > > > > Source/WebCore/bindings/js/JSEventListener.cpp:53 > > > + , m_isInitialized(false) > > > > Can this use inline initialization in the header instead? > > Not when we do bit field packing I guess. I thought this was fixed in C++20? I could be wrong though, I haven't personally tested it.
(In reply to Chris Dumez from comment #11) > (In reply to Alexey Shvayka from comment #9) > > (In reply to Chris Dumez from comment #8) > > > Comment on attachment 452080 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=452080&action=review > > > > > > > Source/WebCore/bindings/js/JSEventListener.cpp:53 > > > > + , m_isInitialized(false) > > > > > > Can this use inline initialization in the header instead? > > > > Not when we do bit field packing I guess. > > I thought this was fixed in C++20? I could be wrong though, I haven't > personally tested it. https://en.cppreference.com/w/cpp/language/bit_field does say that we can provide an initializer since C++20. As far as I know, Alex turned on C++20 for WebKit recently.
Created attachment 452083 [details] Patch Initialize m_isInitialized in header.
(In reply to Chris Dumez from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Alexey Shvayka from comment #9) > > > (In reply to Chris Dumez from comment #8) > > > > Comment on attachment 452080 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=452080&action=review > > > > > > > > > Source/WebCore/bindings/js/JSEventListener.cpp:53 > > > > > + , m_isInitialized(false) > > > > > > > > Can this use inline initialization in the header instead? > > > > > > Not when we do bit field packing I guess. > > > > I thought this was fixed in C++20? I could be wrong though, I haven't > > personally tested it. > > https://en.cppreference.com/w/cpp/language/bit_field does say that we can > provide an initializer since C++20. As far as I know, Alex turned on C++20 > for WebKit recently. Thanks, it works, neat, and there is no chance we would have to cherry-pick this patch, so we can totally use it!
Comment on attachment 452083 [details] Patch r=me
Committed r289892 (247330@trunk): <https://commits.webkit.org/247330@trunk>
(In reply to Chris Dumez from comment #15) > Comment on attachment 452083 [details] > Patch > > r=me Thanks Chris! (In reply to Chris Dumez from comment #12) > (In reply to Chris Dumez from comment #11) > > (In reply to Alexey Shvayka from comment #9) > > > (In reply to Chris Dumez from comment #8) > > > > Comment on attachment 452080 [details] > > > > Patch > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=452080&action=review > > > > > > > > > Source/WebCore/bindings/js/JSEventListener.cpp:53 > > > > > + , m_isInitialized(false) > > > > > > > > Can this use inline initialization in the header instead? > > > > > > Not when we do bit field packing I guess. > > > > I thought this was fixed in C++20? I could be wrong though, I haven't > > personally tested it. > > https://en.cppreference.com/w/cpp/language/bit_field does say that we can > provide an initializer since C++20. As far as I know, Alex turned on C++20 > for WebKit recently. Unfortunately, `mutable bool m_isInitialized : 1 { false };` doesn't build on Windows EWS, and I can't grep any other usages in Source/ (using "m_\w+ : \d+ {" and "m_\w+ : \d+ =" patterns).