Skip to content

Conversation

@cdumez
Copy link
Contributor

@cdumez cdumez commented Feb 21, 2023

b57dc1f

Regression(252852@main) Games on kongregate.com are no longer loading
https://bugs.webkit.org/show_bug.cgi?id=252636
rdar://104392542

Reviewed by Ryosuke Niwa.

Do not lazy load iframes that have no valid URL or "about:blank" as URL. Lazily
loading such iframes has no performance benefits and can actually cause breakage
as JS (like on kongregate.com) may expect them to load synchronously.

Note that we have a more general problem where we fail to create the Frame &
initial empty document when we decide to lazy load an iframe. This is not correct,
and we should only delay the navigation to the frame URL *after* the creation of
the initial empty document. However, fixing this is a larger change and this is
not a regression from 252852@main. As a result, I am merely adding a FIXME comment
in this patch and will address separately.

* LayoutTests/fast/frames/about-blank-frame-no-lazy-loading-expected.txt: Added.
* LayoutTests/fast/frames/about-blank-frame-no-lazy-loading.html: Added.
Add layout test coverage.

* Source/WebCore/html/HTMLFrameElementBase.cpp:
(WebCore::HTMLFrameElementBase::openURL):
* Source/WebCore/html/HTMLIFrameElement.cpp:
(WebCore::isFrameLazyLoadable):
(WebCore::HTMLIFrameElement::shouldLoadFrameLazily):

Canonical link: https://commits.webkit.org/260612@main

9b506f4

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ❌ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🛠 gtk
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk1 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 watch ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch-sim

@cdumez cdumez requested a review from rniwa as a code owner February 21, 2023 01:04
@cdumez cdumez self-assigned this Feb 21, 2023
@cdumez cdumez added the Page Loading For bugs in page loading, including handling of network callbacks. label Feb 21, 2023
@cdumez cdumez requested review from rwlbuis and smfr February 21, 2023 01:05
@rniwa
Copy link
Member

rniwa commented Feb 21, 2023

Should we also file a spec bug?

@cdumez
Copy link
Contributor Author

cdumez commented Feb 21, 2023

Should we also file a spec bug?

I don’t think a spec bug is needed. I think that if we fixed our real bug (the fact that we delay the construction of the initial empty document) then we wouldn’t have broken the site.

i will look into fixing this in a follow up. However, I went with the simpler partial revert here since this was a recent regression from my change and this will go to a branch.

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Feb 21, 2023
https://bugs.webkit.org/show_bug.cgi?id=252636
rdar://104392542

Reviewed by Ryosuke Niwa.

Do not lazy load iframes that have no valid URL or "about:blank" as URL. Lazily
loading such iframes has no performance benefits and can actually cause breakage
as JS (like on kongregate.com) may expect them to load synchronously.

Note that we have a more general problem where we fail to create the Frame &
initial empty document when we decide to lazy load an iframe. This is not correct,
and we should only delay the navigation to the frame URL *after* the creation of
the initial empty document. However, fixing this is a larger change and this is
not a regression from 252852@main. As a result, I am merely adding a FIXME comment
in this patch and will address separately.

* LayoutTests/fast/frames/about-blank-frame-no-lazy-loading-expected.txt: Added.
* LayoutTests/fast/frames/about-blank-frame-no-lazy-loading.html: Added.
Add layout test coverage.

* Source/WebCore/html/HTMLFrameElementBase.cpp:
(WebCore::HTMLFrameElementBase::openURL):
* Source/WebCore/html/HTMLIFrameElement.cpp:
(WebCore::isFrameLazyLoadable):
(WebCore::HTMLIFrameElement::shouldLoadFrameLazily):

Canonical link: https://commits.webkit.org/260612@main
@webkit-commit-queue
Copy link
Collaborator

Committed 260612@main (b57dc1f): https://commits.webkit.org/260612@main

Reviewed commits have been landed. Closing PR #10414 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit b57dc1f into WebKit:main Feb 21, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Page Loading For bugs in page loading, including handling of network callbacks.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants