Bug 236618

Summary:

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

Product:

WebKit

Reporter:

Alexey Shvayka <ashvayka>

Component:

New Bugs

Assignee:

Alexey Shvayka <ashvayka>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, dino, esprehn+autocc, ews-watchlist, fmalita, ggaren, gyuyoung.kim, kangil.han, mkwst, pdr, saam, sabouhallawa, schenney, sergio, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

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

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	cdumez: review+, ews-feeder: commit-queue-

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.