Skip to content

Do not reparse the same URL repeatedly for HTMLImageElement src attribute setter#25003

Merged
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Constellation:eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter
Feb 24, 2024
Merged

Do not reparse the same URL repeatedly for HTMLImageElement src attribute setter#25003
webkit-commit-queue merged 1 commit intoWebKit:mainfrom
Constellation:eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter

Conversation

@Constellation
Copy link
Member

@Constellation Constellation commented Feb 23, 2024

88f5d64

Do not reparse the same URL repeatedly for HTMLImageElement src attribute setter
https://bugs.webkit.org/show_bug.cgi?id=269975
rdar://123492927

Reviewed by Ryosuke Niwa.

This patch cleans up a lot of HTMLImageElement src attribute setter path.

1. HTMLImageElement should not make m_currentSrc AtomString eagerly. This is rarely accessed. So we should defer it.
2. HTMLImageElement should have super fast path for no `sizes` attribute case since this is common. We should not invoke SizesAttributeParser.
3. ImageCandidate should carry underlying AtomString if possible. Attributes are AtomString. So by carrying it,
   we can avoid AtomString creation for `m_bestFitImageURL = candidate.string.toAtomString()`.
4. We should use HTMLImageElement::currentURL if possible in ImageLoader, avoiding repeated parsing of the same URL string.
5. FrameLoader should keep m_outgoingReferrerURL. Then subsequent code can use this URL instead of parsing it repeatedly.
   We enhance URL::strippedForUseAsReferrer to further avoid reparsing URL from stripped string.

* Source/WTF/wtf/URL.cpp:
(WTF::URL::strippedForUseAsReferrer const):
(WTF::URL::strippedForUseAsReferrerWithExplicitPort const):
* Source/WTF/wtf/URL.h:
* Source/WTF/wtf/text/StringView.h:
(WTF::StringViewWithUnderlyingString::toAtomString const):
* Source/WebCore/Modules/fetch/FetchLoader.cpp:
(WebCore::FetchLoader::start):
* Source/WebCore/Modules/reporting/ReportingScope.cpp:
(WebCore::ReportingScope::generateTestReport):
* Source/WebCore/html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::currentSrc):
(WebCore::HTMLImageElement::setBestFitURLAndDPRFromImageCandidate):
(WebCore::HTMLImageElement::selectImageSource):
* Source/WebCore/html/HTMLImageElement.h:
(WebCore::HTMLImageElement::currentSrc const): Deleted.
* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::mediaPlayerReferrer const):
* Source/WebCore/html/parser/HTMLPreloadScanner.cpp:
(WebCore::TokenPreloadScanner::StartTagScanner::processAttributes):
* Source/WebCore/html/parser/HTMLSrcsetParser.cpp:
(WebCore::parseImageCandidatesFromSrcsetAttribute):
(WebCore::pickBestImageCandidate):
(WebCore::bestFitSourceForImageAttributes):
* Source/WebCore/html/parser/HTMLSrcsetParser.h:
(WebCore::ImageCandidate::ImageCandidate):
(WebCore::ImageCandidate::isEmpty const):
* Source/WebCore/loader/CrossOriginAccessControl.cpp:
(WebCore::updateRequestReferrer):
* Source/WebCore/loader/CrossOriginAccessControl.h:
* Source/WebCore/loader/CrossOriginPreflightChecker.cpp:
(WebCore::CrossOriginPreflightChecker::doPreflight):
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::setOutgoingReferrer):
(WebCore::FrameLoader::outgoingReferrerURL):
(WebCore::FrameLoader::loadFrameRequest):
(WebCore::FrameLoader::loadResourceSynchronously):
(WebCore::createWindow):
* Source/WebCore/loader/FrameLoader.h:
* Source/WebCore/loader/ImageLoader.cpp:
(WebCore::ImageLoader::updateFromElement):
* Source/WebCore/loader/LinkLoader.cpp:
(WebCore::LinkLoader::preloadIfNeeded):
* Source/WebCore/loader/PingLoader.cpp:
(WebCore::PingLoader::loadImage):
(WebCore::PingLoader::sendViolationReport):
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe):
(WebCore::FrameLoader::SubframeLoader::loadSubframe):
* Source/WebCore/loader/SubframeLoader.h:
* Source/WebCore/loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::checkRedirectionCrossOriginAccessControl):
* Source/WebCore/loader/WorkerThreadableLoader.cpp:
(WebCore::WorkerThreadableLoader::WorkerThreadableLoader):
* Source/WebCore/loader/cache/CachedResourceRequest.cpp:
(WebCore::CachedResourceRequest::updateReferrerAndOriginHeaders):
* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::setLocation):
(WebCore::LocalDOMWindow::createWindow):
* Source/WebCore/page/SecurityPolicy.cpp:
(WebCore::SecurityPolicy::shouldHideReferrer):
(WebCore::SecurityPolicy::referrerToOriginString):
(WebCore::SecurityPolicy::generateReferrerHeader):
* Source/WebCore/page/SecurityPolicy.h:
* Source/WebCore/page/csp/ContentSecurityPolicy.cpp:
(WebCore::ContentSecurityPolicy::createURLForReporting const):
(WebCore::ContentSecurityPolicy::reportViolation const):
* Source/WebCore/platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::setExistingHTTPReferrerToOriginString):
* Source/WebKit/WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::loadMainResource):

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

b79a10a

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 gtk
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv-sim ✅ 🛠 jsc-armv7
✅ 🛠 watch ✅ 🧪 jsc-armv7-tests
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim

@Constellation Constellation self-assigned this Feb 23, 2024
@Constellation Constellation added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Feb 23, 2024
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from f09fb03 to b7d7c5e Compare February 23, 2024 08:14
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Feb 23, 2024
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from b7d7c5e to 6330c74 Compare February 23, 2024 10:17
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from 6330c74 to 9408ea9 Compare February 23, 2024 21:51
@rniwa
Copy link
Member

rniwa commented Feb 23, 2024

shouldWe should have super fast path

Nit: Typo.

@Constellation
Copy link
Member Author

shouldWe should have super fast path

Nit: Typo.

Oops, fixed!

@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from 9408ea9 to eb2af2d Compare February 23, 2024 23:17
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the boolean signify? Can we use define a struct with a descriptive name instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is indicating whether the strip operation changed the url. I'll change it to struct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use RefPtr here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed.

@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from eb2af2d to 37b392f Compare February 23, 2024 23:47
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from 37b392f to 7e8b4cd Compare February 24, 2024 00:14
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from 7e8b4cd to e8dd48d Compare February 24, 2024 01:21
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Feb 24, 2024
@Constellation Constellation force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from e8dd48d to b79a10a Compare February 24, 2024 02:05
@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 24, 2024
…bute setter

https://bugs.webkit.org/show_bug.cgi?id=269975
rdar://123492927

Reviewed by Ryosuke Niwa.

This patch cleans up a lot of HTMLImageElement src attribute setter path.

1. HTMLImageElement should not make m_currentSrc AtomString eagerly. This is rarely accessed. So we should defer it.
2. HTMLImageElement should have super fast path for no `sizes` attribute case since this is common. We should not invoke SizesAttributeParser.
3. ImageCandidate should carry underlying AtomString if possible. Attributes are AtomString. So by carrying it,
   we can avoid AtomString creation for `m_bestFitImageURL = candidate.string.toAtomString()`.
4. We should use HTMLImageElement::currentURL if possible in ImageLoader, avoiding repeated parsing of the same URL string.
5. FrameLoader should keep m_outgoingReferrerURL. Then subsequent code can use this URL instead of parsing it repeatedly.
   We enhance URL::strippedForUseAsReferrer to further avoid reparsing URL from stripped string.

* Source/WTF/wtf/URL.cpp:
(WTF::URL::strippedForUseAsReferrer const):
(WTF::URL::strippedForUseAsReferrerWithExplicitPort const):
* Source/WTF/wtf/URL.h:
* Source/WTF/wtf/text/StringView.h:
(WTF::StringViewWithUnderlyingString::toAtomString const):
* Source/WebCore/Modules/fetch/FetchLoader.cpp:
(WebCore::FetchLoader::start):
* Source/WebCore/Modules/reporting/ReportingScope.cpp:
(WebCore::ReportingScope::generateTestReport):
* Source/WebCore/html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::currentSrc):
(WebCore::HTMLImageElement::setBestFitURLAndDPRFromImageCandidate):
(WebCore::HTMLImageElement::selectImageSource):
* Source/WebCore/html/HTMLImageElement.h:
(WebCore::HTMLImageElement::currentSrc const): Deleted.
* Source/WebCore/html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::mediaPlayerReferrer const):
* Source/WebCore/html/parser/HTMLPreloadScanner.cpp:
(WebCore::TokenPreloadScanner::StartTagScanner::processAttributes):
* Source/WebCore/html/parser/HTMLSrcsetParser.cpp:
(WebCore::parseImageCandidatesFromSrcsetAttribute):
(WebCore::pickBestImageCandidate):
(WebCore::bestFitSourceForImageAttributes):
* Source/WebCore/html/parser/HTMLSrcsetParser.h:
(WebCore::ImageCandidate::ImageCandidate):
(WebCore::ImageCandidate::isEmpty const):
* Source/WebCore/loader/CrossOriginAccessControl.cpp:
(WebCore::updateRequestReferrer):
* Source/WebCore/loader/CrossOriginAccessControl.h:
* Source/WebCore/loader/CrossOriginPreflightChecker.cpp:
(WebCore::CrossOriginPreflightChecker::doPreflight):
* Source/WebCore/loader/FrameLoader.cpp:
(WebCore::FrameLoader::setOutgoingReferrer):
(WebCore::FrameLoader::outgoingReferrerURL):
(WebCore::FrameLoader::loadFrameRequest):
(WebCore::FrameLoader::loadResourceSynchronously):
(WebCore::createWindow):
* Source/WebCore/loader/FrameLoader.h:
* Source/WebCore/loader/ImageLoader.cpp:
(WebCore::ImageLoader::updateFromElement):
* Source/WebCore/loader/LinkLoader.cpp:
(WebCore::LinkLoader::preloadIfNeeded):
* Source/WebCore/loader/PingLoader.cpp:
(WebCore::PingLoader::loadImage):
(WebCore::PingLoader::sendViolationReport):
* Source/WebCore/loader/SubframeLoader.cpp:
(WebCore::FrameLoader::SubframeLoader::loadOrRedirectSubframe):
(WebCore::FrameLoader::SubframeLoader::loadSubframe):
* Source/WebCore/loader/SubframeLoader.h:
* Source/WebCore/loader/SubresourceLoader.cpp:
(WebCore::SubresourceLoader::checkRedirectionCrossOriginAccessControl):
* Source/WebCore/loader/WorkerThreadableLoader.cpp:
(WebCore::WorkerThreadableLoader::WorkerThreadableLoader):
* Source/WebCore/loader/cache/CachedResourceRequest.cpp:
(WebCore::CachedResourceRequest::updateReferrerAndOriginHeaders):
* Source/WebCore/page/LocalDOMWindow.cpp:
(WebCore::LocalDOMWindow::setLocation):
(WebCore::LocalDOMWindow::createWindow):
* Source/WebCore/page/SecurityPolicy.cpp:
(WebCore::SecurityPolicy::shouldHideReferrer):
(WebCore::SecurityPolicy::referrerToOriginString):
(WebCore::SecurityPolicy::generateReferrerHeader):
* Source/WebCore/page/SecurityPolicy.h:
* Source/WebCore/page/csp/ContentSecurityPolicy.cpp:
(WebCore::ContentSecurityPolicy::createURLForReporting const):
(WebCore::ContentSecurityPolicy::reportViolation const):
* Source/WebCore/platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::setExistingHTTPReferrerToOriginString):
* Source/WebKit/WebProcess/Plugins/PluginView.cpp:
(WebKit::PluginView::loadMainResource):

Canonical link: https://commits.webkit.org/275281@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Do-not-reparse-the-same-URL-repeatedly-for-HTMLImageElement-src-attribute-setter branch from b79a10a to 88f5d64 Compare February 24, 2024 15:47
@webkit-commit-queue
Copy link
Collaborator

Committed 275281@main (88f5d64): https://commits.webkit.org/275281@main

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

@webkit-commit-queue webkit-commit-queue merged commit 88f5d64 into WebKit:main Feb 24, 2024
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DOM For bugs specific to XML/HTML DOM elements (including parsing).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants