-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Avoid crash when adopting nodes with event listeners #39901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
🔨 Triggering try run (#18533841452) for Linux (WPT) |
479ade1 to
cc0f6c1
Compare
|
🔨 Triggering try run (#18533884164) for Linux (WPT) |
|
🤖 Opened new upstream WPT pull request (web-platform-tests/wpt#55454) with upstreamable changes. |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55454). |
cc0f6c1 to
92fcc1c
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55454). |
|
Test results for linux-wpt from try job (#18533884164): Flaky unexpected result (32)
Stable unexpected results that are known to be intermittent (32)
Stable unexpected results (14)
|
|
|
| document.body.appendChild(img); | ||
| iframe.remove(); | ||
| img.addEventListener('load', () => { | ||
| assert_equals(window.imageLoaded, false, "Window should point to iframe contentWindow"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message here is confusing, since it seems to contradict what the test is checking. Doesn't the iframe content window have imageLoaded=true because the element's onload handler executes before the iframe's load event is fired?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the expected behavior should be and opened whatwg/html#11795 to discuss that. Seems like Chrome and Firefox disagree as well on what this should be.
92fcc1c to
61399b8
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55454). |
61399b8 to
0b2a5ee
Compare
|
🔨 Triggering try run (#18780227858) for Linux (WPT) |
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55454). |
|
Test results for linux-wpt from try job (#18780227858): Flaky unexpected result (21)
Stable unexpected results that are known to be intermittent (24)
|
|
✨ Try run (#18780227858) succeeded. |
Previously we would crash when the event listener runs in the new document, since it was compiled against the global from the old document. Instead, we need to mark all event listeners as uncompiled, so that we reset the global to the new document. It also adds a new WPT test, but it is named tentative since Chrome and Firefox differ in behavior. Fixes servo#36397 Signed-off-by: Tim van der Lippe <tvanderlippe@gmail.com>
0b2a5ee to
fab673d
Compare
|
📝 Transplanted new upstreamable changes to existing upstream WPT pull request (web-platform-tests/wpt#55454). |
Previously we would crash when the event listener runs in the new document, since it was compiled against the global from the old document. Instead, we need to mark all event listeners as uncompiled, so that we reset the global to the new document.
It also adds a new WPT test, but it is named tentative since Chrome and Firefox differ in behavior.
Fixes #36397