236618 – REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()

RESOLVED FIXED 236618

REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()

https://bugs.webkit.org/show_bug.cgi?id=236618

Summary REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after...

Alexey Shvayka

Reported 2022-02-14 15:52:07 PST

REGRESSION(r287293): EventListener::wasCreatedFromMarkup() is incorrect after replaceJSFunctionForAttributeListener()

Attachments
Patch (13.87 KB, patch) 2022-02-14 15:59 PST, Alexey Shvayka	no flags	Details Formatted Diff Diff
Patch (14.75 KB, patch) 2022-02-15 13:15 PST, Alexey Shvayka	no flags	Details Formatted Diff Diff
Patch (14.75 KB, patch) 2022-02-15 13:23 PST, Alexey Shvayka	no flags	Details Formatted Diff Diff
Patch (14.73 KB, patch) 2022-02-15 13:32 PST, Alexey Shvayka	cdumez: review+ ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Alexey Shvayka

Comment 1 2022-02-14 15:59:51 PST

Created attachment 451953 [details] Patch

Alexey Shvayka

Comment 2 2022-02-14 15:59:59 PST

<rdar://problem/88696673>

Saam Barati

Comment 3 2022-02-14 17:43:26 PST

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.

Saam Barati

Comment 4 2022-02-14 17:44:44 PST

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?

Alexey Shvayka

Comment 5 2022-02-15 13:15:56 PST

Created attachment 452080 [details] Patch Fix ASSERT in SVGElement, replace bool with enum, remove rvalue parameter, fix condition in EventTarget.cpp, and clarify ChangeLog.

Alexey Shvayka

Comment 6 2022-02-15 13:18:44 PST

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

Chris Dumez

Comment 7 2022-02-15 13:20:04 PST

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.

Chris Dumez

Comment 8 2022-02-15 13:21:40 PST

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?

Alexey Shvayka

Comment 9 2022-02-15 13:22:28 PST

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

Alexey Shvayka

Comment 10 2022-02-15 13:23:05 PST

Created attachment 452082 [details] Patch CreatedFromMarkup: reverse so that No=0 and Yes=1.

Chris Dumez

Comment 11 2022-02-15 13:24:51 PST

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

Chris Dumez

Comment 12 2022-02-15 13:26:46 PST

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

Alexey Shvayka

Comment 13 2022-02-15 13:32:19 PST

Created attachment 452083 [details] Patch Initialize m_isInitialized in header.

Alexey Shvayka

Comment 14 2022-02-15 13:33:22 PST

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

Chris Dumez

Comment 15 2022-02-15 13:36:03 PST

Comment on attachment 452083 [details] Patch r=me

Alexey Shvayka

Comment 16 2022-02-16 08:58:56 PST

Committed r289892 (247330@trunk): <https://commits.webkit.org/247330@trunk>

Alexey Shvayka

Comment 17 2022-02-16 09:40:02 PST

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

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component New Bugs

Assignee

Alexey Shvayka

Reported

2022-02-14 15:52 PST

Modified

2022-02-16 09:40 PST History

CC List

16 users Show

URL

Keywords InRadar

Depends on

Blocks