From 57ffc5e22d842039749b5d6c48f4a40832bedffa Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Sat, 25 Jan 2025 18:36:32 -0800 Subject: [PATCH 01/10] Drop use of WTF_ALLOW_UNSAFE_BUFFER_USAGE in ShorthandSerializer.cpp https://bugs.webkit.org/show_bug.cgi?id=286523 Reviewed by Geoffrey Garen. * Source/WebCore/css/ShorthandSerializer.cpp: (WebCore::LayerValues::serialize const): Canonical link: https://commits.webkit.org/289398@main --- Source/WebCore/css/ShorthandSerializer.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/Source/WebCore/css/ShorthandSerializer.cpp b/Source/WebCore/css/ShorthandSerializer.cpp index 90ade6050bf2d..1360bbeb1db15 100644 --- a/Source/WebCore/css/ShorthandSerializer.cpp +++ b/Source/WebCore/css/ShorthandSerializer.cpp @@ -564,19 +564,17 @@ class LayerValues { return value && value->isPair(); } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN void serialize(StringBuilder& builder) const { // If all are skipped, then serialize the first. - auto begin = std::begin(m_skipSerializing); - auto end = begin + m_shorthand.length(); - bool allSkipped = std::find(begin, end, false) == end; + auto range = std::span { m_skipSerializing }.first(m_shorthand.length()); + bool allSkipped = std::ranges::find(range, false) == range.end(); auto separator = builder.isEmpty() ? ""_s : ", "_s; - for (unsigned j = 0; j < m_shorthand.length(); j++) { + auto longhands = m_shorthand.properties(); + for (auto [j, longhand] : indexedRange(longhands)) { if (allSkipped ? j : m_skipSerializing[j]) continue; - auto longhand = m_shorthand.properties()[j]; if (longhand == CSSPropertyBackgroundSize || longhand == CSSPropertyMaskSize) separator = " / "_s; if (auto& value = m_values[j]) @@ -586,7 +584,6 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN separator = " "_s; } } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END private: const StylePropertyShorthand& m_shorthand; From cd22602acd1a62f94e7f60245d6332f2f15f1078 Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Sat, 25 Jan 2025 19:38:01 -0800 Subject: [PATCH 02/10] Reduce use of strcmp() / strncmp() in the codebase https://bugs.webkit.org/show_bug.cgi?id=286519 Reviewed by Geoffrey Garen. Reduce use of strcmp() / strncmp() in the codebase as they are considered unsafe. * Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h: (CGPDFDictionaryGetNameString): * Source/WebCore/PAL/pal/text/TextCodecICU.cpp: (PAL::TextCodecICU::createICUConverter const): * Source/WebCore/css/typedom/CSSNumericValue.cpp: (WebCore::CSSNumericValue::toSum): * Source/WebCore/dom/TextDecoder.cpp: (WebCore::TextDecoder::create): * Source/WebCore/page/PrintContext.cpp: (WebCore::PrintContext::pageProperty): * Source/WebCore/page/PrintContext.h: * Source/WebCore/platform/SharedMemory.cpp: (WebCore::isMemoryAttributionDisabled): * Source/WebCore/platform/cocoa/AGXCompilerService.cpp: (WebCore::deviceHasAGXCompilerService): * Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm: (WebCore::parseOpusPrivateData): * Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp: (GIFImageReader::parse): * Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp: (WebCore::decodingWarning): * Source/WebCore/platform/mac/CursorMac.mm: (WebCore::cursor): (WebCore::Cursor::ensurePlatformCursor const): * Source/WebCore/platform/sql/SQLiteFileSystem.cpp: (WebCore::SQLiteFileSystem::setCanSuspendLockedFileAttribute): * Source/WebCore/testing/Internals.cpp: (WebCore::Internals::pageProperty const): * Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp: (WebCore::attributesStartElementNsHandler): * Source/WebDriver/WebDriverService.cpp: (WebDriver::WebDriverService::run): * Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm: (-[_WKRemoteObjectRegistry _sendInvocation:interface:]): (-[_WKRemoteObjectRegistry _invokeMethod:]): * Source/WebKit/WebProcess/Plugins/PDF/PDFScriptEvaluation.mm: (WebKit::PDFScriptEvaluation::pdfDocumentContainsPrintScript): * Source/WebKitLegacy/mac/WebView/WebPDFDocumentExtras.mm: (allScriptsInPDFDocument): * Tools/Scripts/webkitpy/style/checkers/cpp.py: (check_safer_cpp): (CppChecker): * Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py: (WebKitStyleTest.test_safer_cpp): Canonical link: https://commits.webkit.org/289399@main --- .../WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h | 8 ++ Source/WebCore/PAL/pal/text/TextCodecICU.cpp | 4 +- .../WebCore/css/typedom/CSSNumericValue.cpp | 3 +- Source/WebCore/dom/TextDecoder.cpp | 4 +- Source/WebCore/page/PrintContext.cpp | 18 ++-- Source/WebCore/page/PrintContext.h | 2 +- Source/WebCore/platform/SharedMemory.cpp | 8 +- .../platform/cocoa/AGXCompilerService.cpp | 4 +- .../graphics/cocoa/WebMAudioUtilitiesCocoa.mm | 4 +- .../image-decoders/gif/GIFImageReader.cpp | 7 +- .../image-decoders/png/PNGImageDecoder.cpp | 2 +- Source/WebCore/platform/mac/CursorMac.mm | 84 +++++++++---------- .../WebCore/platform/sql/SQLiteFileSystem.cpp | 4 +- Source/WebCore/testing/Internals.cpp | 2 +- .../xml/parser/XMLDocumentParserLibxml2.cpp | 2 +- Source/WebDriver/WebDriverService.cpp | 32 +++---- .../API/Cocoa/_WKRemoteObjectRegistry.mm | 12 +-- .../Plugins/PDF/PDFScriptEvaluation.mm | 6 +- .../mac/WebView/WebPDFDocumentExtras.mm | 4 +- Tools/Scripts/webkitpy/style/checkers/cpp.py | 10 +++ .../webkitpy/style/checkers/cpp_unittest.py | 10 +++ 21 files changed, 118 insertions(+), 112 deletions(-) diff --git a/Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h b/Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h index e09ef1c79623d..d25e411d6698f 100644 --- a/Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h +++ b/Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h @@ -27,6 +27,7 @@ #include #include +#include #if HAVE(IOSURFACE) #include @@ -449,3 +450,10 @@ CG_EXTERN void CGEnterLockdownModeForFonts(); #endif WTF_EXTERN_C_END + +inline String CGPDFDictionaryGetNameString(CGPDFDictionaryRef dictionary, ASCIILiteral key) +{ + const char* value = nullptr; + CGPDFDictionaryGetName(dictionary, key.characters(), &value); + return value ? String::fromUTF8(value) : String(); +} diff --git a/Source/WebCore/PAL/pal/text/TextCodecICU.cpp b/Source/WebCore/PAL/pal/text/TextCodecICU.cpp index 80b8ddedffc27..597cd9a007879 100644 --- a/Source/WebCore/PAL/pal/text/TextCodecICU.cpp +++ b/Source/WebCore/PAL/pal/text/TextCodecICU.cpp @@ -162,12 +162,10 @@ void TextCodecICU::createICUConverter() const if (cachedConverter) { UErrorCode error = U_ZERO_ERROR; const char* cachedConverterName = ucnv_getName(cachedConverter.get(), &error); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (U_SUCCESS(error) && !strcmp(m_canonicalConverterName, cachedConverterName)) { + if (U_SUCCESS(error) && m_canonicalConverterName == cachedConverterName) { m_converter = WTFMove(cachedConverter); return; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END } UErrorCode error = U_ZERO_ERROR; diff --git a/Source/WebCore/css/typedom/CSSNumericValue.cpp b/Source/WebCore/css/typedom/CSSNumericValue.cpp index f6bf029a2e1e0..a8e36fed16d94 100644 --- a/Source/WebCore/css/typedom/CSSNumericValue.cpp +++ b/Source/WebCore/css/typedom/CSSNumericValue.cpp @@ -52,6 +52,7 @@ #include "ExceptionOr.h" #include #include +#include #include namespace WebCore { @@ -403,7 +404,7 @@ ExceptionOr> CSSNumericValue::toSum(FixedVector&& units) if (parsedUnits.isEmpty()) { std::sort(values.begin(), values.end(), [](auto& a, auto& b) { - return strcmp(downcast(a)->unitSerialization().characters(), downcast(b)->unitSerialization().characters()) < 0; + return compareSpans(downcast(a)->unitSerialization().span(), downcast(b)->unitSerialization().span()) < 0; }); return CSSMathSum::create(WTFMove(values)); } diff --git a/Source/WebCore/dom/TextDecoder.cpp b/Source/WebCore/dom/TextDecoder.cpp index 7b3bca21244fd..d6a7f0afe006c 100644 --- a/Source/WebCore/dom/TextDecoder.cpp +++ b/Source/WebCore/dom/TextDecoder.cpp @@ -46,10 +46,8 @@ ExceptionOr> TextDecoder::create(const String& label, Options o if (trimmedLabel.contains(nullCharacter)) return Exception { ExceptionCode::RangeError }; auto decoder = adoptRef(*new TextDecoder(trimmedLabel, options)); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (!decoder->m_textEncoding.isValid() || !strcmp(decoder->m_textEncoding.name(), "replacement")) + if (!decoder->m_textEncoding.isValid() || decoder->m_textEncoding.name() == "replacement"_s) return Exception { ExceptionCode::RangeError }; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END return decoder; } diff --git a/Source/WebCore/page/PrintContext.cpp b/Source/WebCore/page/PrintContext.cpp index 108b30ad2e9b2..17aedef88dcb7 100644 --- a/Source/WebCore/page/PrintContext.cpp +++ b/Source/WebCore/page/PrintContext.cpp @@ -360,7 +360,7 @@ void PrintContext::outputLinkedDestinations(GraphicsContext& graphicsContext, Do } } -String PrintContext::pageProperty(LocalFrame* frame, const char* propertyName, int pageNumber) +String PrintContext::pageProperty(LocalFrame* frame, const String& propertyName, int pageNumber) { ASSERT(frame); ASSERT(frame->document()); @@ -373,26 +373,22 @@ String PrintContext::pageProperty(LocalFrame* frame, const char* propertyName, i document->updateLayout(); auto style = document->styleScope().resolver().styleForPage(pageNumber); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - // Implement formatters for properties we care about. - if (!strcmp(propertyName, "margin-left")) { + if (propertyName == "margin-left"_s) { if (style->marginLeft().isAuto()) return autoAtom(); return String::number(style->marginLeft().value()); } - if (!strcmp(propertyName, "line-height")) + if (propertyName == "line-height"_s) return String::number(style->lineHeight().value()); - if (!strcmp(propertyName, "font-size")) + if (propertyName == "font-size"_s) return String::number(style->fontDescription().computedSize()); - if (!strcmp(propertyName, "font-family")) + if (propertyName == "font-family"_s) return style->fontDescription().firstFamily(); - if (!strcmp(propertyName, "size")) + if (propertyName == "size"_s) return makeString(style->pageSize().width.value(), ' ', style->pageSize().height.value()); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END - - return makeString("pageProperty() unimplemented for: "_s, unsafeSpan(propertyName)); + return makeString("pageProperty() unimplemented for: "_s, propertyName); } bool PrintContext::isPageBoxVisible(LocalFrame* frame, int pageNumber) diff --git a/Source/WebCore/page/PrintContext.h b/Source/WebCore/page/PrintContext.h index 8e36ea39125df..9f8f5a8d0d8df 100644 --- a/Source/WebCore/page/PrintContext.h +++ b/Source/WebCore/page/PrintContext.h @@ -77,7 +77,7 @@ class PrintContext : public FrameDestructionObserver { // Used by layout tests. WEBCORE_EXPORT static int pageNumberForElement(Element*, const FloatSize& pageSizeInPixels); // Returns -1 if page isn't found. - WEBCORE_EXPORT static String pageProperty(LocalFrame*, const char* propertyName, int pageNumber); + WEBCORE_EXPORT static String pageProperty(LocalFrame*, const String& propertyName, int pageNumber); WEBCORE_EXPORT static bool isPageBoxVisible(LocalFrame*, int pageNumber); WEBCORE_EXPORT static String pageSizeAndMarginsInPixels(LocalFrame*, int pageNumber, int width, int height, int marginTop, int marginRight, int marginBottom, int marginLeft); WEBCORE_EXPORT static int numberOfPages(LocalFrame&, const FloatSize& pageSizeInPixels); diff --git a/Source/WebCore/platform/SharedMemory.cpp b/Source/WebCore/platform/SharedMemory.cpp index f658b01f49bbc..b3460a3de98ac 100644 --- a/Source/WebCore/platform/SharedMemory.cpp +++ b/Source/WebCore/platform/SharedMemory.cpp @@ -35,12 +35,10 @@ namespace WebCore { bool isMemoryAttributionDisabled() { static bool result = []() { - const char* value = getenv("WEBKIT_DISABLE_MEMORY_ATTRIBUTION"); - if (!value) + auto value = unsafeSpan(getenv("WEBKIT_DISABLE_MEMORY_ATTRIBUTION")); + if (!value.data()) return false; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - return !strcmp(value, "1"); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END + return equalSpans(value, "1"_span); }(); return result; } diff --git a/Source/WebCore/platform/cocoa/AGXCompilerService.cpp b/Source/WebCore/platform/cocoa/AGXCompilerService.cpp index 7551c614ad155..7b557ff720dbc 100644 --- a/Source/WebCore/platform/cocoa/AGXCompilerService.cpp +++ b/Source/WebCore/platform/cocoa/AGXCompilerService.cpp @@ -49,8 +49,8 @@ bool deviceHasAGXCompilerService() hasAGXCompilerService = false; return *hasAGXCompilerService; } - const char* machine = systemInfo.machine; - if (!strcmp(machine, "iPad5,1") || !strcmp(machine, "iPad5,2") || !strcmp(machine, "iPad5,3") || !strcmp(machine, "iPad5,4")) + auto machine = unsafeSpan(systemInfo.machine); + if (equalSpans(machine, "iPad5,1"_span) || equalSpans(machine, "iPad5,2"_span) || equalSpans(machine, "iPad5,3"_span) || equalSpans(machine, "iPad5,4"_span)) hasAGXCompilerService = true; else hasAGXCompilerService = false; diff --git a/Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm b/Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm index e452bd5661b48..a7fa98788df18 100644 --- a/Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm +++ b/Source/WebCore/platform/graphics/cocoa/WebMAudioUtilitiesCocoa.mm @@ -291,10 +291,8 @@ bool parseOpusTOCData(std::span frameData, OpusCookieContents& co // // This is an 8-octet (64-bit) field that allows codec // identification and is human readable. -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (strncmp("OpusHead", byteCast(codecPrivateData.data()), 8)) + if (!spanHasPrefix(codecPrivateData, "OpusHead"_span8)) return { }; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END OpusCookieContents cookie; diff --git a/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp b/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp index 3575f40baefb4..4b9b1c9b5b512 100644 --- a/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp +++ b/Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp @@ -435,9 +435,9 @@ bool GIFImageReader::parse(size_t dataPosition, size_t len, bool parseSizeOnly) case GIFType: { // All GIF files begin with "GIF87a" or "GIF89a". - if (!strncmp((char*)currentComponent.data(), "GIF89a", 6)) + if (spanHasPrefix(currentComponent, "GIF89a"_span)) m_version = 89; - else if (!strncmp((char*)currentComponent.data(), "GIF87a", 6)) + else if (spanHasPrefix(currentComponent, "GIF87a"_span)) m_version = 87; else return false; @@ -608,8 +608,7 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_END case GIFApplicationExtension: { // Check for netscape application extension. - if (m_bytesToConsume == 11 - && (!strncmp(reinterpret_cast(currentComponent.data()), "NETSCAPE2.0", 11) || !strncmp(reinterpret_cast(currentComponent.data()), "ANIMEXTS1.0", 11))) + if (m_bytesToConsume == 11 && (spanHasPrefix(currentComponent, "NETSCAPE2.0"_span) || spanHasPrefix(currentComponent, "ANIMEXTS1.0"_span))) GETN(1, GIFNetscapeExtensionBlock); else GETN(1, GIFConsumeBlock); diff --git a/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp b/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp index c0011d4e6a7e7..df5c4e2349c4a 100644 --- a/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp +++ b/Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp @@ -83,7 +83,7 @@ static void PNGAPI decodingWarning(png_structp png, png_const_charp warningMsg) // Mozilla did this, so we will too. // Convert a tRNS warning to be an error (see // http://bugzilla.mozilla.org/show_bug.cgi?id=251381 ) - if (!strncmp(warningMsg, "Missing PLTE before tRNS", 24)) + if (spanHasPrefix(unsafeSpan(warningMsg), "Missing PLTE before tRNS"_span)) png_error(png, warningMsg); } diff --git a/Source/WebCore/platform/mac/CursorMac.mm b/Source/WebCore/platform/mac/CursorMac.mm index f5a943d0aad43..8c51008fd84bb 100644 --- a/Source/WebCore/platform/mac/CursorMac.mm +++ b/Source/WebCore/platform/mac/CursorMac.mm @@ -129,48 +129,47 @@ static Class coreCursorClass() return coreCursorClass; } -static NSCursor *cursor(const char *name) +static NSCursor *cursor(ASCIILiteral name) { -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN __strong NSCursor **slot = nullptr; - if (!strcmp(name, "BusyButClickable")) + if (name == "BusyButClickable"_s) slot = &busyButClickableNSCursor; - else if (!strcmp(name, "MakeAlias")) + else if (name == "MakeAlias"_s) slot = &makeAliasNSCursor; - else if (!strcmp(name, "Move")) + else if (name == "Move"_s) slot = &moveNSCursor; - else if (!strcmp(name, "ResizeEast")) + else if (name == "ResizeEast"_s) slot = &resizeEastNSCursor; - else if (!strcmp(name, "ResizeEastWest")) + else if (name == "ResizeEastWest"_s) slot = &resizeEastWestNSCursor; - else if (!strcmp(name, "ResizeNorth")) + else if (name == "ResizeNorth"_s) slot = &resizeNorthNSCursor; - else if (!strcmp(name, "ResizeNorthSouth")) + else if (name == "ResizeNorthSouth"_s) slot = &resizeNorthSouthNSCursor; - else if (!strcmp(name, "ResizeNortheast")) + else if (name == "ResizeNortheast"_s) slot = &resizeNortheastNSCursor; - else if (!strcmp(name, "ResizeNortheastSouthwest")) + else if (name == "ResizeNortheastSouthwest"_s) slot = &resizeNortheastSouthwestNSCursor; - else if (!strcmp(name, "ResizeNorthwest")) + else if (name == "ResizeNorthwest"_s) slot = &resizeNorthwestNSCursor; - else if (!strcmp(name, "ResizeNorthwestSoutheast")) + else if (name == "ResizeNorthwestSoutheast"_s) slot = &resizeNorthwestSoutheastNSCursor; - else if (!strcmp(name, "ResizeSouth")) + else if (name == "ResizeSouth"_s) slot = &resizeSouthNSCursor; - else if (!strcmp(name, "ResizeSoutheast")) + else if (name == "ResizeSoutheast"_s) slot = &resizeSoutheastNSCursor; - else if (!strcmp(name, "ResizeSouthwest")) + else if (name == "ResizeSouthwest"_s) slot = &resizeSouthwestNSCursor; - else if (!strcmp(name, "ResizeWest")) + else if (name == "ResizeWest"_s) slot = &resizeWestNSCursor; - else if (!strcmp(name, "Cell")) + else if (name == "Cell"_s) slot = &cellNSCursor; - else if (!strcmp(name, "Help")) + else if (name == "Help"_s) slot = &helpNSCursor; - else if (!strcmp(name, "ZoomIn")) + else if (name == "ZoomIn"_s) slot = &zoomInNSCursor; - else if (!strcmp(name, "ZoomOut")) + else if (name == "ZoomOut"_s) slot = &zoomOutNSCursor; if (!slot) @@ -179,12 +178,11 @@ static Class coreCursorClass() if (!*slot) *slot = [[coreCursorClass() alloc] init]; return *slot; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END } #else -static NSCursor *cursor(const char *) +static NSCursor *cursor(ASCIILiteral) { return [NSCursor arrowCursor]; } @@ -258,72 +256,72 @@ static Class coreCursorClass() break; case Type::Wait: - m_platformCursor = cursor("BusyButClickable"); + m_platformCursor = cursor("BusyButClickable"_s); break; case Type::Help: - m_platformCursor = cursor("Help"); + m_platformCursor = cursor("Help"_s); break; case Type::Move: case Type::MiddlePanning: - m_platformCursor = cursor("Move"); + m_platformCursor = cursor("Move"_s); break; case Type::EastResize: case Type::EastPanning: - m_platformCursor = cursor("ResizeEast"); + m_platformCursor = cursor("ResizeEast"_s); break; case Type::NorthResize: case Type::NorthPanning: - m_platformCursor = cursor("ResizeNorth"); + m_platformCursor = cursor("ResizeNorth"_s); break; case Type::NorthEastResize: case Type::NorthEastPanning: - m_platformCursor = cursor("ResizeNortheast"); + m_platformCursor = cursor("ResizeNortheast"_s); break; case Type::NorthWestResize: case Type::NorthWestPanning: - m_platformCursor = cursor("ResizeNorthwest"); + m_platformCursor = cursor("ResizeNorthwest"_s); break; case Type::SouthResize: case Type::SouthPanning: - m_platformCursor = cursor("ResizeSouth"); + m_platformCursor = cursor("ResizeSouth"_s); break; case Type::SouthEastResize: case Type::SouthEastPanning: - m_platformCursor = cursor("ResizeSoutheast"); + m_platformCursor = cursor("ResizeSoutheast"_s); break; case Type::SouthWestResize: case Type::SouthWestPanning: - m_platformCursor = cursor("ResizeSouthwest"); + m_platformCursor = cursor("ResizeSouthwest"_s); break; case Type::WestResize: case Type::WestPanning: - m_platformCursor = cursor("ResizeWest"); + m_platformCursor = cursor("ResizeWest"_s); break; case Type::NorthSouthResize: - m_platformCursor = cursor("ResizeNorthSouth"); + m_platformCursor = cursor("ResizeNorthSouth"_s); break; case Type::EastWestResize: - m_platformCursor = cursor("ResizeEastWest"); + m_platformCursor = cursor("ResizeEastWest"_s); break; case Type::NorthEastSouthWestResize: - m_platformCursor = cursor("ResizeNortheastSouthwest"); + m_platformCursor = cursor("ResizeNortheastSouthwest"_s); break; case Type::NorthWestSouthEastResize: - m_platformCursor = cursor("ResizeNorthwestSoutheast"); + m_platformCursor = cursor("ResizeNorthwestSoutheast"_s); break; case Type::ColumnResize: @@ -339,7 +337,7 @@ static Class coreCursorClass() break; case Type::Cell: - m_platformCursor = cursor("Cell"); + m_platformCursor = cursor("Cell"_s); break; case Type::ContextMenu: @@ -347,11 +345,11 @@ static Class coreCursorClass() break; case Type::Alias: - m_platformCursor = cursor("MakeAlias"); + m_platformCursor = cursor("MakeAlias"_s); break; case Type::Progress: - m_platformCursor = cursor("BusyButClickable"); + m_platformCursor = cursor("BusyButClickable"_s); break; case Type::NoDrop: @@ -375,11 +373,11 @@ static Class coreCursorClass() break; case Type::ZoomIn: - m_platformCursor = cursor("ZoomIn"); + m_platformCursor = cursor("ZoomIn"_s); break; case Type::ZoomOut: - m_platformCursor = cursor("ZoomOut"); + m_platformCursor = cursor("ZoomOut"_s); break; case Type::Grab: diff --git a/Source/WebCore/platform/sql/SQLiteFileSystem.cpp b/Source/WebCore/platform/sql/SQLiteFileSystem.cpp index 7ab90be6a03d9..271a1ddf530f4 100644 --- a/Source/WebCore/platform/sql/SQLiteFileSystem.cpp +++ b/Source/WebCore/platform/sql/SQLiteFileSystem.cpp @@ -103,9 +103,7 @@ void SQLiteFileSystem::setCanSuspendLockedFileAttribute(const String& filePath) auto path = makeString(filePath, suffix); char excluded = 0xff; auto result = setxattr(FileSystem::fileSystemRepresentation(path).data(), "com.apple.runningboard.can-suspend-locked", &excluded, sizeof(excluded), 0, 0); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (result < 0 && !strcmp(suffix, "")) -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END + if (result < 0 && suffix == ""_s) RELEASE_LOG_ERROR(SQLDatabase, "SQLiteFileSystem::setCanSuspendLockedFileAttribute: setxattr failed: %" PUBLIC_LOG_STRING, strerror(errno)); } } diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp index 809157d0d0e09..78e2e0d87c71e 100644 --- a/Source/WebCore/testing/Internals.cpp +++ b/Source/WebCore/testing/Internals.cpp @@ -3816,7 +3816,7 @@ ExceptionOr Internals::pageProperty(const String& propertyName, int page if (!frame()) return Exception { ExceptionCode::InvalidAccessError }; - return PrintContext::pageProperty(frame(), propertyName.utf8().data(), pageNumber); + return PrintContext::pageProperty(frame(), propertyName, pageNumber); } ExceptionOr Internals::pageSizeAndMarginsInPixels(int pageNumber, int width, int height, int marginTop, int marginRight, int marginBottom, int marginLeft) const diff --git a/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp b/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp index e4d522c4b512d..3fd4bd3791487 100644 --- a/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp +++ b/Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp @@ -1502,7 +1502,7 @@ using AttributeParseState = std::optional>; static void attributesStartElementNsHandler(void* closure, const xmlChar* xmlLocalName, const xmlChar* /*xmlPrefix*/, const xmlChar* /*xmlURI*/, int /*numNamespaces*/, const xmlChar** /*namespaces*/, int numAttributes, int /*numDefaulted*/, const xmlChar** libxmlAttributes) { - if (strcmp(byteCast(xmlLocalName), "attrs")) + if (!equalSpans(unsafeSpan(byteCast(xmlLocalName)), "attrs"_span)) return; auto& state = *static_cast(static_cast(closure)->_private); diff --git a/Source/WebDriver/WebDriverService.cpp b/Source/WebDriver/WebDriverService.cpp index 3c6e09f08d9f4..fb32717426ef1 100644 --- a/Source/WebDriver/WebDriverService.cpp +++ b/Source/WebDriver/WebDriverService.cpp @@ -98,13 +98,13 @@ int WebDriverService::run(int argc, char** argv) if (const char* targetEnvVar = getenv("WEBDRIVER_TARGET_ADDR")) targetString = String::fromLatin1(targetEnvVar); for (int i = 1 ; i < argc; ++i) { - const char* arg = argv[i]; - if (!strcmp(arg, "-h") || !strcmp(arg, "--help")) { + auto arg = unsafeSpan(argv[i]); + if (equalSpans(arg, "-h"_span) || equalSpans(arg, "--help"_span)) { printUsageStatement(argv[0]); return EXIT_SUCCESS; } - if (!strcmp(arg, "-p") && portString.isNull()) { + if (equalSpans(arg, "-p"_span) && portString.isNull()) { if (++i == argc) { printUsageStatement(argv[0]); return EXIT_FAILURE; @@ -113,27 +113,27 @@ int WebDriverService::run(int argc, char** argv) continue; } - static const unsigned portStrLength = strlen("--port="); - if (!strncmp(arg, "--port=", portStrLength) && portString.isNull()) { - portString = String::fromLatin1(arg + portStrLength); + static constexpr auto portArgument = "--port="_span; + if (spanHasPrefix(arg, portArgument) && portString.isNull()) { + portString = arg.subspan(portArgument.size()); continue; } - static const unsigned hostStrLength = strlen("--host="); - if (!strncmp(arg, "--host=", hostStrLength) && !host) { - host = String::fromLatin1(arg + hostStrLength); + static constexpr auto hostArgument = "--host="_span; + if (spanHasPrefix(arg, hostArgument) && !host) { + host = arg.subspan(hostArgument.size()); continue; } #if ENABLE(WEBDRIVER_BIDI) - static const unsigned bidiPortStrLength = strlen("--bidi-port="); - if (!strncmp(arg, "--bidi-port=", bidiPortStrLength) && bidiPortString.isNull()) { - bidiPortString = String::fromLatin1(arg + bidiPortStrLength); + static constexpr auto bidiPortArgument = "--bidi-port="_span; + if (spanHasPrefix(arg, bidiPortArgument) && bidiPortString.isNull()) { + bidiPortString = arg.subspan(bidiPortArgument.size()); continue; } #endif - if (!strcmp(arg, "-t") && targetString.isNull()) { + if (equalSpans(arg, "-t"_span) && targetString.isNull()) { if (++i == argc) { printUsageStatement(argv[0]); return EXIT_FAILURE; @@ -142,9 +142,9 @@ int WebDriverService::run(int argc, char** argv) continue; } - static const unsigned targetStrLength = strlen("--target="); - if (!strncmp(arg, "--target=", targetStrLength) && targetString.isNull()) { - targetString = String::fromLatin1(arg + targetStrLength); + static constexpr auto targetArgument = "--target="_span; + if (spanHasPrefix(arg, targetArgument) && targetString.isNull()) { + targetString = arg.subspan(targetArgument.size()); continue; } } diff --git a/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm b/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm index 7ca43c4adf917..74a5d18038323 100644 --- a/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm +++ b/Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectRegistry.mm @@ -143,12 +143,10 @@ - (void)_sendInvocation:(NSInvocation *)invocation interface:(_WKRemoteObjectInt NSMethodSignature *methodSignature = invocation.methodSignature; for (NSUInteger i = 0, count = methodSignature.numberOfArguments; i < count; ++i) { - const char *type = [methodSignature getArgumentTypeAtIndex:i]; + auto type = unsafeSpan([methodSignature getArgumentTypeAtIndex:i]); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (strcmp(type, "@?")) + if (!equalSpans(type, "@?"_span)) continue; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END if (replyInfo) [NSException raise:NSInvalidArgumentException format:@"Only one reply block is allowed per message send. (%s)", sel_getName(invocation.selector)]; @@ -233,12 +231,10 @@ - (void)_invokeMethod:(const WebKit::RemoteObjectInvocation&)remoteObjectInvocat // Look for the block argument (if any). for (NSUInteger i = 0, count = methodSignature.numberOfArguments; i < count; ++i) { - const char *type = [methodSignature getArgumentTypeAtIndex:i]; + auto type = unsafeSpan([methodSignature getArgumentTypeAtIndex:i]); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (strcmp(type, "@?")) + if (!equalSpans(type, "@?"_span)) continue; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END // We found the block. // If the wire had no block signature but we expect one, we drop the message. diff --git a/Source/WebKit/WebProcess/Plugins/PDF/PDFScriptEvaluation.mm b/Source/WebKit/WebProcess/Plugins/PDF/PDFScriptEvaluation.mm index 8b8e8c04d92be..919d14f6a660c 100644 --- a/Source/WebKit/WebProcess/Plugins/PDF/PDFScriptEvaluation.mm +++ b/Source/WebKit/WebProcess/Plugins/PDF/PDFScriptEvaluation.mm @@ -30,6 +30,7 @@ #import #import +#import #import #import #import @@ -100,11 +101,8 @@ static bool pdfDocumentContainsPrintScript(RetainPtr pdfDocume continue; // A JavaScript action must have an action type of "JavaScript". - const char* actionType = nullptr; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (!CGPDFDictionaryGetName(javaScriptAction, "S", &actionType) || strcmp(actionType, "JavaScript")) + if (CGPDFDictionaryGetNameString(javaScriptAction, "S"_s) != "JavaScript"_s) continue; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END auto scriptFromBytes = [](std::span bytes) { CFStringEncoding encoding = (bytes.size() > 1 && bytes[0] == 0xFE && bytes[1] == 0xFF) ? kCFStringEncodingUnicode : kCFStringEncodingUTF8; diff --git a/Source/WebKitLegacy/mac/WebView/WebPDFDocumentExtras.mm b/Source/WebKitLegacy/mac/WebView/WebPDFDocumentExtras.mm index 48a57eaf816b3..d5d03be147f07 100644 --- a/Source/WebKitLegacy/mac/WebView/WebPDFDocumentExtras.mm +++ b/Source/WebKitLegacy/mac/WebView/WebPDFDocumentExtras.mm @@ -25,6 +25,7 @@ #import "WebPDFDocumentExtras.h" +#import #import #import @@ -94,8 +95,7 @@ static void getAllValuesInPDFNameTree(CGPDFDictionaryRef tree, Vector Date: Sat, 25 Jan 2025 19:52:26 -0800 Subject: [PATCH 03/10] REGRESSION(285726@main?): [macOS wk2]: http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html is a flaky failure (flaky in EWS) https://bugs.webkit.org/show_bug.cgi?id=283210 rdar://140007072 Reviewed by Youenn Fablet. The test was painting red in a canvas for 0.5s following by green for 1.5. However, due the the MediaRecorder potentially taking times to actually start recording it was possible that we only got a single red frame at the start of the video. The test seeked to 0.05 check the colours and then at the duration-0.05. Due to the delay described above, at 0.05 the frame could actually be green. Instead we should be seeking to 0; however as the video may not have starting playing then, we wouldn't have got the seekend event. So instead we first seek to the end, check that it's green and then seek to the start and check that it's red. We also paint red for 1s instead of 0.5s to give more time for the recording to start on slow machines. Manually testing however still indicated an issue, with the MP4 player (specifically MediaPlayerPrivateAVFObjC) there's no guarantee that when the seeked even is fired, the right frame is actually already displayed (webkit.org/b/236755), and so checking the frame could fail as the old frame is still being displayed. To get around this we now use a MediaSource insted as the MSE player (and WebM player) do guarantee that the right frame is being displayed once the seeked event is fired. Fly-By: VideoMediaSampleRenderer::copyDisplayedPixelBuffer may have returned nullptr when seeking to 0 and the first video frame had a time in the future (such video file can be currently incorrectly generated by the MediaRecorder). * LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html: * LayoutTests/platform/mac-wk2/TestExpectations: * Source/WebCore/platform/graphics/cocoa/VideoMediaSampleRenderer.mm: (WebCore::VideoMediaSampleRenderer::copyDisplayedPixelBuffer): Return the last video frame, even if in the future if it is currently visible. Canonical link: https://commits.webkit.org/289400@main --- ...Recorder-AV-audio-video-dataavailable.html | 87 ++++++++++--------- LayoutTests/platform/mac-wk2/TestExpectations | 3 +- .../cocoa/VideoMediaSampleRenderer.mm | 2 +- 3 files changed, 50 insertions(+), 42 deletions(-) diff --git a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html index 12cadf5a79629..e16f4e97952be 100644 --- a/LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html +++ b/LayoutTests/http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html @@ -32,7 +32,7 @@ if (context) { context.fillStyle = "#ff0000"; context.fillRect(0, 0, 200, 200); - if (Date.now() - drawStartTime < 500) { + if (Date.now() - drawStartTime < 1000) { window.requestAnimationFrame(doRedImageDraw); } else { drawStartTime = Date.now(); @@ -51,7 +51,15 @@ } } - async_test(t => { + function waitForEvent(object, eventName, testName) + { + return new Promise((resolve, reject) => { + object.addEventListener(eventName, (e) => resolve(e), { once: true }); + setTimeout(() => reject("waitForEvent " + (testName ? (testName + " ") : "") + "timed out for " + eventName), 2500); + }); + } + + promise_test(async (test) => { const ac = new AudioContext(); const osc = ac.createOscillator(); const dest = ac.createMediaStreamDestination(); @@ -64,48 +72,47 @@ video.addTrack(audio.getAudioTracks()[0]); assert_equals(video.getAudioTracks().length, 1, "video mediastream starts with one audio track"); const recorder = new MediaRecorder(video); - let mode = 0; - - recorder.ondataavailable = t.step_func(blobEvent => { - assert_true(blobEvent instanceof BlobEvent, 'the type of event should be BlobEvent'); - assert_equals(blobEvent.type, 'dataavailable', 'the event type should be dataavailable'); - assert_true(blobEvent.isTrusted, 'isTrusted should be true when the event is created by C++'); - assert_true(blobEvent.data instanceof Blob, 'the type of data should be Blob'); - assert_true(blobEvent.data.size > 0, 'the blob should contain some buffers'); - player.src = window.URL.createObjectURL(blobEvent.data); - const resFrame = document.getElementById("frame"); - const resContext = resFrame.getContext('2d'); - - player.oncanplay = t.step_func(() => { - assert_greater_than(player.duration, 0.1, 'the duration should be greater than 100ms'); - player.play(); - }); - player.onplay = () => { - player.pause(); - player.currentTime = 0.05; - }; - player.onseeked = t.step_func(() => { - resContext.drawImage(player, 0, 0); - if (!mode) { - _assertPixelApprox(resFrame, 25, 25, 255, 0, 0, 255, "25, 25", "255, 0, 0, 255", 50); - _assertPixelApprox(resFrame, 50, 50, 255, 0, 0, 255, "50, 50", "255, 0, 0, 255", 50); - mode = 1; - player.currentTime = Math.min(1.5, player.duration - 0.05); - } else { - _assertPixelApprox(resFrame, 20, 20, 0, 255, 0, 255, "20, 20", "0, 255, 0, 255", 50); - _assertPixelApprox(resFrame, 199, 199, 0, 255, 0, 255, "199, 199", "0, 255, 0, 255", 50); - t.done(); - } - }); - player.load(); - }); drawStartTime = Date.now(); doRedImageDraw(); recorder.start(); assert_equals(recorder.state, 'recording', 'MediaRecorder has been started successfully'); - setTimeout(() => { - recorder.stop(); - }, 2000); + setTimeout(() => { recorder.stop(); }, 2000); + + const blobEvent = await waitForEvent(recorder, 'dataavailable'); + assert_true(blobEvent instanceof BlobEvent, 'the type of event should be BlobEvent'); + assert_equals(blobEvent.type, 'dataavailable', 'the event type should be dataavailable'); + assert_true(blobEvent.isTrusted, 'isTrusted should be true when the event is created by C++'); + assert_true(blobEvent.data instanceof Blob, 'the type of data should be Blob'); + assert_true(blobEvent.data.size > 0, 'the blob should contain some buffers'); + + const MediaSource = self.ManagedMediaSource || self.MediaSource; + player.disableRemotePlayback = true; + const source = new MediaSource(); + player.src = URL.createObjectURL(source); + await waitForEvent(source, 'sourceopen'); + const sourceBuffer = source.addSourceBuffer("video/mp4"); + sourceBuffer.appendBuffer(await blobEvent.data.arrayBuffer()); + await waitForEvent(sourceBuffer, 'updateend'); + source.endOfStream(); + + const resFrame = document.getElementById("frame"); + const resContext = resFrame.getContext('2d'); + + assert_greater_than(player.duration, 1, 'the duration should be greater than 1s'); + await player.play(); + + player.pause(); + player.currentTime = Math.min(1.5, player.duration - 0.05); + await waitForEvent(player, 'seeked'); + resContext.drawImage(player, 0, 0); + _assertPixelApprox(resFrame, 20, 20, 0, 255, 0, 255, "20, 20", "0, 255, 0, 255", 50); + _assertPixelApprox(resFrame, 199, 199, 0, 255, 0, 255, "199, 199", "0, 255, 0, 255", 50); + + player.currentTime = 0; + await waitForEvent(player, 'seeked'); + resContext.drawImage(player, 0, 0); + _assertPixelApprox(resFrame, 25, 25, 255, 0, 0, 255, "25, 25", "255, 0, 0, 255", 50); + _assertPixelApprox(resFrame, 50, 50, 255, 0, 0, 255, "50, 50", "255, 0, 0, 255", 50); }, 'MediaRecorder can successfully record the video for a audio-video stream'); diff --git a/LayoutTests/platform/mac-wk2/TestExpectations b/LayoutTests/platform/mac-wk2/TestExpectations index 546dd3f3d70ad..81c69202a901c 100644 --- a/LayoutTests/platform/mac-wk2/TestExpectations +++ b/LayoutTests/platform/mac-wk2/TestExpectations @@ -1915,7 +1915,8 @@ webkit.org/b/283144 [ Sequoia ] http/tests/storageAccess/grant-storage-access-un webkit.org/b/283144 [ Sequoia ] http/tests/storageAccess/request-and-grant-access-then-navigate-cross-site-should-not-have-access.https.html [ Failure ] webkit.org/b/283144 [ Sequoia ] http/tests/storageAccess/request-and-grant-access-then-navigate-same-site-should-have-access.https.html [ Failure ] -webkit.org/b/283210 http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html [ Pass Failure ] +# On x86_64 Ventura bots, stream recording doesnt always complete within the 2s. +[ Ventura x86_64 ] http/wpt/mediarecorder/MediaRecorder-AV-audio-video-dataavailable.html [ Pass Failure ] webkit.org/b/283596 [ Sequoia+ Debug ] ipc/cfnetwork-crashes-with-string-to-string-http-headers.html [ Skip ] diff --git a/Source/WebCore/platform/graphics/cocoa/VideoMediaSampleRenderer.mm b/Source/WebCore/platform/graphics/cocoa/VideoMediaSampleRenderer.mm index b807c2f6ba6cc..2e7bb47c7e3a4 100644 --- a/Source/WebCore/platform/graphics/cocoa/VideoMediaSampleRenderer.mm +++ b/Source/WebCore/platform/graphics/cocoa/VideoMediaSampleRenderer.mm @@ -742,7 +742,7 @@ static OSStatus sampleCallback(CMBufferRef buffer, void* refcon) CMTime currentTime = PAL::CMTimebaseGetTime(timebase.get()); CMTime presentationTime = PAL::CMSampleBufferGetOutputPresentationTimeStamp(nextSample.get()); - if (PAL::CMTimeCompare(presentationTime, currentTime) > 0) + if (PAL::CMTimeCompare(presentationTime, currentTime) > 0 && (!m_lastDisplayedSample || PAL::CMTimeCompare(presentationTime, *m_lastDisplayedSample) > 0)) return; imageBuffer = (CVPixelBufferRef)PAL::CMSampleBufferGetImageBuffer(nextSample.get()); From f1c1197adbc419004979c177898bed371a018c56 Mon Sep 17 00:00:00 2001 From: Jean-Yves Avenard Date: Sat, 25 Jan 2025 20:03:12 -0800 Subject: [PATCH 04/10] Instagram video doesn't replay https://bugs.webkit.org/show_bug.cgi?id=286475 rdar://141590278 Reviewed by Eric Carlson. The video's buffered ranges weren't recalculated when the MediaSource was re-opened (its readyState moved from "ended" to "open"). The calculation of the buffered ranges depends on the MediaSource's readyState. Whenever the video's buffered attribute change, a message is sent to the GPU process with the new final value. In parallel, the GPU's MediaSourcePrivate will recalculate the playable buffered ranges whenever new data were added. Instagram when playing a MSE video, append all the data then calls MediaSource.endOfStream() to then totally unnecessarily re-append once again all the data followed by endOfStream() several times. As the video's buffered attribute wasn't updated with the appendBuffer that followed (which reopened the MediaSource) the GPU process buffered range mirror was now incorrectly assuming the MediaSource wasn't ended and kept waiting for more data to come, never reaching the player's ended state. We now correctly recalculate the buffered ranges whenever the MediaSource is closed or re-opened. Added test that ensures: 1- the buffered range gets updated when the MediaSource move back to "open" state 2- we will fire the ended event when playback reaches the end. * LayoutTests/media/media-source/media-source-reopen-expected.txt: Added. * LayoutTests/media/media-source/media-source-reopen.html: Added. * Source/WebCore/Modules/mediasource/MediaSource.cpp: (WebCore::MediaSource::onReadyStateChange): Canonical link: https://commits.webkit.org/289401@main --- .../media-source-reopen-expected.txt | 20 ++++++ .../media-source/media-source-reopen.html | 63 +++++++++++++++++++ .../Modules/mediasource/MediaSource.cpp | 3 +- 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 LayoutTests/media/media-source/media-source-reopen-expected.txt create mode 100644 LayoutTests/media/media-source/media-source-reopen.html diff --git a/LayoutTests/media/media-source/media-source-reopen-expected.txt b/LayoutTests/media/media-source/media-source-reopen-expected.txt new file mode 100644 index 0000000000000..e71cbc0ad4190 --- /dev/null +++ b/LayoutTests/media/media-source/media-source-reopen-expected.txt @@ -0,0 +1,20 @@ + +RUN(video.src = URL.createObjectURL(source)) +EVENT(sourceopen) +RUN(audiosb = source.addSourceBuffer("audio/mp4; codecs=mp4a.40.2")) +RUN(videosb = source.addSourceBuffer("video/mp4; codecs=avc1.4d401e")) +RUN(audiosb.appendWindowEnd = 0.4) +RUN(videosb.appendWindowEnd = 0.5) +EXPECTED (video.buffered.end(video.buffered.length-1) == Math.min(audiosb.buffered.end(audiosb.buffered.length-1), videosb.buffered.end(videosb.buffered.length-1)) == 'true') OK +RUN(source.endOfStream()) +EXPECTED (source.readyState == 'ended') OK +EXPECTED (video.buffered.end(video.buffered.length-1) == Math.max(audiosb.buffered.end(audiosb.buffered.length-1), videosb.buffered.end(videosb.buffered.length-1)) == 'true') OK +EXPECTED (video.duration == Math.max(audiosb.buffered.end(audiosb.buffered.length-1), videosb.buffered.end(videosb.buffered.length-1)) == 'true') OK +EVENT(sourceended) +EXPECTED (source.readyState == 'open') OK +EXPECTED (video.buffered.end(video.buffered.length-1) == Math.min(audiosb.buffered.end(audiosb.buffered.length-1), videosb.buffered.end(videosb.buffered.length-1)) == 'true') OK +EVENT(update) +RUN(source.endOfStream()) +EVENT(ended) +END OF TEST + diff --git a/LayoutTests/media/media-source/media-source-reopen.html b/LayoutTests/media/media-source/media-source-reopen.html new file mode 100644 index 0000000000000..aec61e8e5c585 --- /dev/null +++ b/LayoutTests/media/media-source/media-source-reopen.html @@ -0,0 +1,63 @@ + + + + media-source-reopen + + + + + + + diff --git a/Source/WebCore/Modules/mediasource/MediaSource.cpp b/Source/WebCore/Modules/mediasource/MediaSource.cpp index bcb809ef07c4a..0453ce8534e23 100644 --- a/Source/WebCore/Modules/mediasource/MediaSource.cpp +++ b/Source/WebCore/Modules/mediasource/MediaSource.cpp @@ -1419,8 +1419,9 @@ void MediaSource::onReadyStateChange(ReadyState oldState, ReadyState newState) // https://w3c.github.io/media-source/#htmlmediaelement-extensions-buffered for (auto& sourceBuffer : m_sourceBuffers.get()) sourceBuffer->setMediaSourceEnded(true); - updateBufferedIfNeeded(true /* force */); } + if (newState == ReadyState::Ended || (newState == ReadyState::Open && oldState == ReadyState::Ended)) + updateBufferedIfNeeded(true /* force */); // MediaSource's readyState transitions from "open" to "closed" or "ended" to "closed". if (oldState > ReadyState::Closed && newState == ReadyState::Closed) { From fc04b09d028ce6cb51c6b16e98b994fa81567307 Mon Sep 17 00:00:00 2001 From: Tyler Wilcock Date: Sat, 25 Jan 2025 20:17:18 -0800 Subject: [PATCH 05/10] AX: AXTextMarker::findMarker doesn't move to the right offset when encountering a non-ASCII character https://bugs.webkit.org/show_bug.cgi?id=286522 rdar://143609336 Reviewed by Chris Fleizach. With this commit, we use CachedTextBreakIterator::{preceding, following} to determine how many offsets to move when encountering non-ASCII characters, preventing AXTextMarker::findMarker from moving to an offset in the middle of a non-ASCII character (e.g. an emoji). This partially fixes accessibility/text-marker/text-marker-previous-next.html in ENABLE(AX_THREAD_TEXT_APIS) mode, which includes an emoji testcase. * Source/WebCore/accessibility/AXTextMarker.cpp: (WebCore::AXTextMarker::findMarker const): * Source/WebCore/accessibility/AXTextRun.h: (WebCore::AXTextRuns::AXTextRuns): Canonical link: https://commits.webkit.org/289402@main --- Source/WebCore/accessibility/AXTextMarker.cpp | 15 ++++++++++++--- Source/WebCore/accessibility/AXTextRun.h | 14 +++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/Source/WebCore/accessibility/AXTextMarker.cpp b/Source/WebCore/accessibility/AXTextMarker.cpp index 880a31a7c7304..ec85a39b35e5b 100644 --- a/Source/WebCore/accessibility/AXTextMarker.cpp +++ b/Source/WebCore/accessibility/AXTextMarker.cpp @@ -745,8 +745,17 @@ AXTextMarker AXTextMarker::findMarker(AXDirection direction, CoalesceObjectBreak // If the BR isn't in an editable ancestor, we shouldn't be including it (in most cases of findMarker). bool shouldSkipBR = ignoreBRs == IgnoreBRs::Yes && object && object->roleValue() == AccessibilityRole::LineBreak && !object->editableAncestor(); bool isWithinRunBounds = ((direction == AXDirection::Next && offset() < runs()->totalLength()) || (direction == AXDirection::Previous && offset())); - if (!shouldSkipBR && isWithinRunBounds) - return AXTextMarker { treeID(), objectID(), direction == AXDirection::Next ? offset() + 1 : offset() - 1 }; + if (!shouldSkipBR && isWithinRunBounds) { + if (runs()->containsOnlyASCII) { + // In the common case where the text-runs only contain ASCII, all we need to do is the move the offset by 1, + // which is more efficient than turning the runs into a string and creating a CachedTextBreakIterator. + return AXTextMarker { treeID(), objectID(), direction == AXDirection::Next ? offset() + 1 : offset() - 1 }; + } + + CachedTextBreakIterator iterator(runs()->toString(), { }, TextBreakIterator::CaretMode { }, nullAtom()); + unsigned newOffset = direction == AXDirection::Next ? iterator.following(offset()).value_or(offset() + 1) : iterator.preceding(offset()).value_or(offset() - 1); + return AXTextMarker { treeID(), objectID(), newOffset }; + } // offset() pointed to the last character in the given object's runs, so let's traverse to find the next object with runs. if (RefPtr object = findObjectWithRuns(*isolatedObject(), direction, stopAtID)) { @@ -754,7 +763,7 @@ AXTextMarker AXTextMarker::findMarker(AXDirection direction, CoalesceObjectBreak // The startingOffset is used to advance one position farther when we are coalescing object breaks and skipping positions. unsigned startingOffset = 0; - if ((coalesceObjectBreaks == CoalesceObjectBreaks::Yes || shouldSkipBR) && !isolatedObject()->emitsNewlineAfter()) + if (coalesceObjectBreaks == CoalesceObjectBreaks::Yes || shouldSkipBR) startingOffset = 1; return AXTextMarker { *object, direction == AXDirection::Next ? startingOffset : object->textRuns()->lastRunLength() - startingOffset }; diff --git a/Source/WebCore/accessibility/AXTextRun.h b/Source/WebCore/accessibility/AXTextRun.h index ed364a6aeae85..fb3333cb5cc36 100644 --- a/Source/WebCore/accessibility/AXTextRun.h +++ b/Source/WebCore/accessibility/AXTextRun.h @@ -82,12 +82,20 @@ struct AXTextRuns { // Do not de-reference. Use for comparison purposes only. void* containingBlock { nullptr }; Vector runs; + bool containsOnlyASCII { true }; AXTextRuns() = default; - AXTextRuns(RenderBlock* containingBlock, Vector&& runs) + AXTextRuns(RenderBlock* containingBlock, Vector&& textRuns) : containingBlock(containingBlock) - , runs(WTFMove(runs)) - { } + , runs(WTFMove(textRuns)) + { + for (const auto& run : runs) { + if (!run.text.containsOnlyASCII()) { + containsOnlyASCII = false; + break; + } + } + } String debugDescription() const; size_t size() const { return runs.size(); } From 47b66cb0dc7c2146fa7576555c720080708667f3 Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Sat, 25 Jan 2025 20:21:27 -0800 Subject: [PATCH 06/10] Drop use of WTF_ALLOW_UNSAFE_BUFFER_USAGE in ElementData.cpp https://bugs.webkit.org/show_bug.cgi?id=286521 Reviewed by Per Arne Vollan. * Source/WebCore/dom/ElementData.cpp: (WebCore::UniqueElementData::UniqueElementData): (): Deleted. Canonical link: https://commits.webkit.org/289403@main --- Source/WebCore/dom/ElementData.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Source/WebCore/dom/ElementData.cpp b/Source/WebCore/dom/ElementData.cpp index 0fe8534e82a4d..9572e811497be 100644 --- a/Source/WebCore/dom/ElementData.cpp +++ b/Source/WebCore/dom/ElementData.cpp @@ -142,9 +142,7 @@ UniqueElementData::UniqueElementData(const UniqueElementData& other) UniqueElementData::UniqueElementData(const ShareableElementData& other) : ElementData(other, true) -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - , m_attributeVector(std::span { other.m_attributeArray, other.length() }) -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END + , m_attributeVector(other.attributes()) { // An ShareableElementData should never have a mutable inline StyleProperties attached. ASSERT(!other.m_inlineStyle || !other.m_inlineStyle->isMutable()); From 0cd772a92637d64cf8fa464aa47ae14167657e82 Mon Sep 17 00:00:00 2001 From: Sihui Liu Date: Sun, 26 Jan 2025 09:54:32 -0800 Subject: [PATCH 07/10] CacheStorageRecord::isolatedCopy should make isolated copy for options https://bugs.webkit.org/show_bug.cgi?id=286483 rdar://143576776 Reviewed by Youenn Fablet. WebCore::FetchOptions is a non-trivial type that needs deep copy. * Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h: (WebKit::CacheStorageRecord::isolatedCopy): Canonical link: https://commits.webkit.org/289404@main --- Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h b/Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h index 3a2d8085a5b6d..eae5c15b31fcb 100644 --- a/Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h +++ b/Source/WebKit/NetworkProcess/storage/CacheStorageRecord.h @@ -91,7 +91,7 @@ struct CacheStorageRecord { crossThreadCopy(WTFMove(info)), requestHeadersGuard, crossThreadCopy(WTFMove(request)), - options, + crossThreadCopy(WTFMove(options)), crossThreadCopy(WTFMove(referrer)), responseHeadersGuard, crossThreadCopy(WTFMove(responseData)), From b47b64eb57ec390405396d2888709a7e755d4e08 Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Sun, 26 Jan 2025 10:41:51 -0800 Subject: [PATCH 08/10] Reduce use of WTF_ALLOW_UNSAFE_BUFFER_USAGE in TextCodecCJK.cpp https://bugs.webkit.org/show_bug.cgi?id=286540 Reviewed by Sihui Liu. * Source/WebCore/PAL/pal/text/EncodingTables.h: (PAL::findInSortedPairs): * Source/WebCore/PAL/pal/text/TextCodecCJK.cpp: (PAL::shiftJISEncode): (PAL::big5Encode): (PAL::gbEncodeShared): Canonical link: https://commits.webkit.org/289405@main --- Source/WebCore/PAL/pal/text/EncodingTables.h | 8 +++--- Source/WebCore/PAL/pal/text/TextCodecCJK.cpp | 29 ++++++++------------ 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/Source/WebCore/PAL/pal/text/EncodingTables.h b/Source/WebCore/PAL/pal/text/EncodingTables.h index d8cd2b8a01d69..212cb51b7ccac 100644 --- a/Source/WebCore/PAL/pal/text/EncodingTables.h +++ b/Source/WebCore/PAL/pal/text/EncodingTables.h @@ -49,7 +49,7 @@ template void stableSortByFirst(CollectionType&); template bool isSortedByFirst(const CollectionType&); template bool sortedFirstsAreUnique(const CollectionType&); template static auto findFirstInSortedPairs(const CollectionType& sortedPairsCollection, const KeyType&) -> std::optionalsecond)>; -template static auto findInSortedPairs(const CollectionType& sortedPairsCollection, const KeyType&) -> std::pair; +template static auto findInSortedPairs(const CollectionType& sortedPairsCollection, const KeyType&) -> std::span>; #if !ASSERT_ENABLED inline void checkEncodingTableInvariants() { } @@ -124,12 +124,12 @@ template static auto findFirstInSorte return iterator->second; } -template static auto findInSortedPairs(const CollectionType& collection, const KeyType& key) -> std::pair { +template static auto findInSortedPairs(const CollectionType& collection, const KeyType& key) -> std::span> { if constexpr (std::is_integral_v) { if (key != decltype(std::begin(collection)->first)(key)) - return { std::end(collection), std::end(collection) }; + return { }; } - return std::equal_range(std::begin(collection), std::end(collection), makeFirstAdapter(key), CompareFirst { }); + return std::ranges::equal_range(collection, makeFirstAdapter(key), CompareFirst { }); } } diff --git a/Source/WebCore/PAL/pal/text/TextCodecCJK.cpp b/Source/WebCore/PAL/pal/text/TextCodecCJK.cpp index 790dba7abd9cd..29cd22f329f5e 100644 --- a/Source/WebCore/PAL/pal/text/TextCodecCJK.cpp +++ b/Source/WebCore/PAL/pal/text/TextCodecCJK.cpp @@ -670,16 +670,14 @@ static Vector shiftJISEncode(StringView string, Function= range.first); - ASSERT(range.second - range.first <= 3); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - for (auto pair = range.first; pair < range.second; pair++) { - uint16_t pointer = pair->second; + ASSERT(range.size() <= 3); + for (auto& pair : range) { + uint16_t pointer = pair.second; if (pointer >= 8272 && pointer <= 8835) continue; uint8_t lead = pointer / 188; @@ -690,7 +688,6 @@ WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN result.append(trail + offset); break; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END } return result; } @@ -783,8 +780,6 @@ static const Big5EncodeIndex& big5EncodeIndex() return *table; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - // https://encoding.spec.whatwg.org/#big5-encoder static Vector big5Encode(StringView string, Function&)>&& unencodableHandler) { @@ -799,17 +794,17 @@ static Vector big5Encode(StringView string, Functionsecond; + pointer = range.back().second; else - pointer = pointerRange.first->second; + pointer = range.front().second; if (pointer < 157 * (0xA1 - 0x81)) { unencodableHandler(codePoint, result); @@ -825,8 +820,6 @@ static Vector big5Encode(StringView string, Function, 207>& gb18030Ranges() { @@ -1051,9 +1044,9 @@ static Vector gbEncodeShared(StringView string, Functionsecond; + auto range = findInSortedPairs(gb18030EncodeIndex(), codePoint); + if (!range.empty()) { + uint16_t pointer = range[0].second; uint8_t lead = pointer / 190 + 0x81; uint8_t trail = pointer % 190; uint8_t offset = trail < 0x3F ? 0x40 : 0x41; From a69cdc2b13007677cbada1e6d74ca8009a474af1 Mon Sep 17 00:00:00 2001 From: Chris Dumez Date: Sun, 26 Jan 2025 13:19:34 -0800 Subject: [PATCH 09/10] Add safe wrapper for xpc_dictionary_get_string() https://bugs.webkit.org/show_bug.cgi?id=286547 Reviewed by Per Arne Vollan. * Source/WTF/wtf/spi/darwin/XPCSPI.h: (xpc_dictionary_get_data_span): (xpc_dictionary_get_wtfstring): * Source/WebKit/NetworkProcess/Authentication/cocoa/AuthenticationManagerCocoa.mm: (WebKit::AuthenticationManager::initializeConnection): * Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementConnectionCocoa.mm: (WebKit::PCM::Connection::connectionReceivedEvent): * Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm: (WebKit::LaunchServicesDatabaseObserver::handleEvent): * Source/WebKit/Platform/cocoa/MediaCapability.mm: (WebKit::MediaCapability::environmentIdentifier const): * Source/WebKit/Platform/cocoa/XPCUtilities.mm: (WebKit::handleXPCExitMessage): * Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.h: * Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.mm: (WebKit::handleXPCEndpointMessage): * Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm: (WebKit::XPCServiceInitializerDelegate::getClientIdentifier): (WebKit::XPCServiceInitializerDelegate::getClientBundleIdentifier): (WebKit::XPCServiceInitializerDelegate::getClientSDKAlignedBehaviors): (WebKit::XPCServiceInitializerDelegate::getProcessIdentifier): (WebKit::XPCServiceInitializerDelegate::getClientProcessName): (WebKit::XPCServiceInitializerDelegate::getExtraInitializationData): * Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm: (WebKit::checkFrameworkVersion): (WebKit::XPCServiceEventHandler): * Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm: (WebKit::ProcessLauncher::finishLaunchingProcess): * Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm: (WebKit::NetworkProcessProxy::XPCEventHandler::handleXPCEvent const): * Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm: (WebKit::LaunchServicesDatabaseManager::handleEvent): * Tools/Scripts/webkitpy/style/checkers/cpp.py: (check_safer_cpp): (CppChecker): * Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py: (WebKitStyleTest.test_safer_cpp): * Tools/TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm: * Tools/WebKitTestRunner/WebKitEligibilityUtil/main.cpp: (main): Canonical link: https://commits.webkit.org/289406@main --- Source/WTF/wtf/spi/darwin/XPCSPI.h | 17 +++++++++++-- .../cocoa/AuthenticationManagerCocoa.mm | 4 +--- .../PrivateClickMeasurementConnectionCocoa.mm | 4 ++-- .../cocoa/LaunchServicesDatabaseObserver.mm | 4 ++-- .../WebKit/Platform/cocoa/MediaCapability.mm | 2 +- Source/WebKit/Platform/cocoa/XPCUtilities.mm | 4 ++-- .../Cocoa/XPCService/XPCEndpointMessages.h | 2 +- .../Cocoa/XPCService/XPCEndpointMessages.mm | 10 +++----- .../Cocoa/XPCService/XPCServiceEntryPoint.mm | 24 +++++++++---------- .../Cocoa/XPCService/XPCServiceMain.mm | 20 +++++++--------- .../Launcher/cocoa/ProcessLauncherCocoa.mm | 6 ++--- .../Network/NetworkProcessProxyCocoa.mm | 6 ++--- .../cocoa/LaunchServicesDatabaseManager.mm | 4 ++-- Tools/Scripts/webkitpy/style/checkers/cpp.py | 10 ++++++++ .../webkitpy/style/checkers/cpp_unittest.py | 10 ++++++++ .../TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm | 8 +++---- 16 files changed, 79 insertions(+), 56 deletions(-) diff --git a/Source/WTF/wtf/spi/darwin/XPCSPI.h b/Source/WTF/wtf/spi/darwin/XPCSPI.h index e42baf30177ad..75b97e3bb1d73 100644 --- a/Source/WTF/wtf/spi/darwin/XPCSPI.h +++ b/Source/WTF/wtf/spi/darwin/XPCSPI.h @@ -29,6 +29,8 @@ #include #include #include +#include +#include #if HAVE(XPC_API) || USE(APPLE_INTERNAL_SDK) #include @@ -256,9 +258,20 @@ void xpc_release(xpc_object_t); WTF_EXTERN_C_END -inline std::span xpc_dictionary_get_data_span(xpc_object_t xdict, const char* key) +inline std::span xpc_dictionary_get_data_span(xpc_object_t xdict, ASCIILiteral key) { size_t dataSize { 0 }; - auto* data = static_cast(xpc_dictionary_get_data(xdict, key, &dataSize)); + auto* data = static_cast(xpc_dictionary_get_data(xdict, key.characters(), &dataSize)); // NOLINT return unsafeMakeSpan(data, dataSize); } + +// ASCIILiteral version of XPC_ERROR_KEY_DESCRIPTION. +static constexpr auto xpcErrorDescriptionKey = "XPCErrorDescription"_s; + +inline String xpc_dictionary_get_wtfstring(xpc_object_t xdict, ASCIILiteral key) +{ + auto* cstring = xpc_dictionary_get_string(xdict, key.characters()); // NOLINT + if (!cstring) + return { }; + return String::fromUTF8(cstring); +} diff --git a/Source/WebKit/NetworkProcess/Authentication/cocoa/AuthenticationManagerCocoa.mm b/Source/WebKit/NetworkProcess/Authentication/cocoa/AuthenticationManagerCocoa.mm index 71a09542afaf7..acce5e689f3ef 100644 --- a/Source/WebKit/NetworkProcess/Authentication/cocoa/AuthenticationManagerCocoa.mm +++ b/Source/WebKit/NetworkProcess/Authentication/cocoa/AuthenticationManagerCocoa.mm @@ -62,12 +62,10 @@ if (type == XPC_TYPE_ERROR || !weakThis) return; -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (type != XPC_TYPE_DICTIONARY || strcmp(xpc_dictionary_get_string(event.get(), ClientCertificateAuthentication::XPCMessageNameKey), ClientCertificateAuthentication::XPCMessageNameValue)) { + if (type != XPC_TYPE_DICTIONARY || xpc_dictionary_get_wtfstring(event.get(), ClientCertificateAuthentication::XPCMessageNameKey) != ClientCertificateAuthentication::XPCMessageNameValue) { ASSERT_NOT_REACHED(); return; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END auto challengeID = xpc_dictionary_get_uint64(event.get(), ClientCertificateAuthentication::XPCChallengeIDKey); if (!challengeID) diff --git a/Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementConnectionCocoa.mm b/Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementConnectionCocoa.mm index df724a21d22bc..777c7e622e231 100644 --- a/Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementConnectionCocoa.mm +++ b/Source/WebKit/NetworkProcess/PrivateClickMeasurement/cocoa/PrivateClickMeasurementConnectionCocoa.mm @@ -49,14 +49,14 @@ { if (xpc_get_type(request) != XPC_TYPE_DICTIONARY) return; - const char* debugMessage = xpc_dictionary_get_string(request, protocolDebugMessageKey); + String debugMessage = xpc_dictionary_get_wtfstring(request, protocolDebugMessageKey); if (!debugMessage) return; auto messageLevel = static_cast(xpc_dictionary_get_uint64(request, protocolDebugMessageLevelKey)); auto* networkSession = m_networkSession.get(); if (!networkSession) return; - m_networkSession->networkProcess().broadcastConsoleMessage(m_networkSession->sessionID(), MessageSource::PrivateClickMeasurement, messageLevel, String::fromUTF8(debugMessage)); + m_networkSession->networkProcess().broadcastConsoleMessage(m_networkSession->sessionID(), MessageSource::PrivateClickMeasurement, messageLevel, debugMessage); } OSObjectPtr Connection::dictionaryFromMessage(MessageType messageType, EncodedMessage&& message) const diff --git a/Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm b/Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm index 49e1a1823b936..12e17244910f1 100644 --- a/Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm +++ b/Source/WebKit/NetworkProcess/cocoa/LaunchServicesDatabaseObserver.mm @@ -132,8 +132,8 @@ return; } if (xpc_get_type(event) == XPC_TYPE_DICTIONARY) { - auto* messageName = xpc_dictionary_get_string(event, xpcMessageNameKey); - if (LaunchServicesDatabaseXPCConstants::xpcRequestLaunchServicesDatabaseUpdateMessageName != messageName) + String messageName = xpc_dictionary_get_wtfstring(event, xpcMessageNameKey); + if (messageName != LaunchServicesDatabaseXPCConstants::xpcRequestLaunchServicesDatabaseUpdateMessageName) return; startObserving(connection); } diff --git a/Source/WebKit/Platform/cocoa/MediaCapability.mm b/Source/WebKit/Platform/cocoa/MediaCapability.mm index adc2e1df9cf60..749a806e74102 100644 --- a/Source/WebKit/Platform/cocoa/MediaCapability.mm +++ b/Source/WebKit/Platform/cocoa/MediaCapability.mm @@ -74,7 +74,7 @@ xpc_object_t xpcObject = [m_mediaEnvironment createXPCRepresentation]; if (!xpcObject) return emptyString(); - return String::fromUTF8(xpc_dictionary_get_string(xpcObject, "identifier")); + return xpc_dictionary_get_wtfstring(xpcObject, "identifier"_s); #endif return { }; diff --git a/Source/WebKit/Platform/cocoa/XPCUtilities.mm b/Source/WebKit/Platform/cocoa/XPCUtilities.mm index 780152ddae624..507f436442376 100644 --- a/Source/WebKit/Platform/cocoa/XPCUtilities.mm +++ b/Source/WebKit/Platform/cocoa/XPCUtilities.mm @@ -65,8 +65,8 @@ void terminateWithReason(xpc_connection_t connection, ReasonCode, const char*) void handleXPCExitMessage(xpc_object_t event) { if (xpc_get_type(event) == XPC_TYPE_DICTIONARY) { - auto* messageName = xpc_dictionary_get_string(event, messageNameKey); - if (exitProcessMessage == messageName) { + String messageName = xpc_dictionary_get_wtfstring(event, messageNameKey); + if (messageName == exitProcessMessage) { RELEASE_LOG_ERROR(IPC, "Received exit message, exiting now."); terminateProcess(EXIT_FAILURE); } diff --git a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.h b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.h index 7f9dcfc6be5f5..585296e0d991d 100644 --- a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.h +++ b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.h @@ -29,6 +29,6 @@ namespace WebKit { -void handleXPCEndpointMessage(xpc_object_t message, const char* messageName); +void handleXPCEndpointMessage(xpc_object_t message, const String& messageName); } diff --git a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.mm b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.mm index 837d84dd57282..b466f4810c878 100644 --- a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.mm +++ b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCEndpointMessages.mm @@ -65,22 +65,20 @@ static void handleVideoReceiverEndpointMessage(xpc_object_t message) } #endif -void handleXPCEndpointMessage(xpc_object_t message, const char* messageName) +void handleXPCEndpointMessage(xpc_object_t message, const String& messageName) { ASSERT_UNUSED(messageName, messageName); RELEASE_ASSERT(xpc_get_type(message) == XPC_TYPE_DICTIONARY); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - #if HAVE(LSDATABASECONTEXT) - if (!strcmp(messageName, LaunchServicesDatabaseXPCConstants::xpcLaunchServicesDatabaseXPCEndpointMessageName)) { + if (messageName == LaunchServicesDatabaseXPCConstants::xpcLaunchServicesDatabaseXPCEndpointMessageName) { handleLaunchServiceDatabaseMessage(message); return; } #endif #if ENABLE(LINEAR_MEDIA_PLAYER) - if (!strcmp(messageName, VideoReceiverEndpointMessage::messageName().characters())) { + if (messageName == VideoReceiverEndpointMessage::messageName()) { RunLoop::main().dispatch([message = OSObjectPtr(message)] { handleVideoReceiverEndpointMessage(message.get()); }); @@ -88,8 +86,6 @@ void handleXPCEndpointMessage(xpc_object_t message, const char* messageName) } #endif -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END - } } // namespace WebKit diff --git a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm index e18a8239feabf..aed1b4cffb8fd 100644 --- a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm +++ b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceEntryPoint.mm @@ -79,19 +79,19 @@ bool XPCServiceInitializerDelegate::getClientIdentifier(String& clientIdentifier) { - clientIdentifier = String::fromUTF8(xpc_dictionary_get_string(m_initializerMessage, "client-identifier")); + clientIdentifier = xpc_dictionary_get_wtfstring(m_initializerMessage, "client-identifier"_s); return !clientIdentifier.isEmpty(); } bool XPCServiceInitializerDelegate::getClientBundleIdentifier(String& clientBundleIdentifier) { - clientBundleIdentifier = String::fromLatin1(xpc_dictionary_get_string(m_initializerMessage, "client-bundle-identifier")); + clientBundleIdentifier = xpc_dictionary_get_wtfstring(m_initializerMessage, "client-bundle-identifier"_s); return !clientBundleIdentifier.isEmpty(); } bool XPCServiceInitializerDelegate::getClientSDKAlignedBehaviors(SDKAlignedBehaviors& behaviors) { - auto behaviorData = xpc_dictionary_get_data_span(m_initializerMessage, "client-sdk-aligned-behaviors"); + auto behaviorData = xpc_dictionary_get_data_span(m_initializerMessage, "client-sdk-aligned-behaviors"_s); if (behaviorData.empty()) return false; memcpySpan(behaviors.storageBytes(), behaviorData); @@ -100,7 +100,7 @@ bool XPCServiceInitializerDelegate::getProcessIdentifier(std::optional& identifier) { - auto parsedIdentifier = parseInteger(StringView::fromLatin1(xpc_dictionary_get_string(m_initializerMessage, "process-identifier"))); + auto parsedIdentifier = parseInteger(xpc_dictionary_get_wtfstring(m_initializerMessage, "process-identifier"_s)); if (!parsedIdentifier) return false; if (!ObjectIdentifier::isValidIdentifier(*parsedIdentifier)) @@ -112,7 +112,7 @@ bool XPCServiceInitializerDelegate::getClientProcessName(String& clientProcessName) { - clientProcessName = String::fromUTF8(xpc_dictionary_get_string(m_initializerMessage, "ui-process-name")); + clientProcessName = xpc_dictionary_get_wtfstring(m_initializerMessage, "ui-process-name"_s); return !clientProcessName.isEmpty(); } @@ -120,32 +120,32 @@ { xpc_object_t extraDataInitializationDataObject = xpc_dictionary_get_value(m_initializerMessage, "extra-initialization-data"); - auto inspectorProcess = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "inspector-process")); + auto inspectorProcess = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "inspector-process"_s); if (!inspectorProcess.isEmpty()) extraInitializationData.add("inspector-process"_s, inspectorProcess); - auto serviceWorkerProcess = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "service-worker-process")); + auto serviceWorkerProcess = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "service-worker-process"_s); if (!serviceWorkerProcess.isEmpty()) extraInitializationData.add("service-worker-process"_s, WTFMove(serviceWorkerProcess)); - auto registrableDomain = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "registrable-domain")); + auto registrableDomain = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "registrable-domain"_s); if (!registrableDomain.isEmpty()) extraInitializationData.add("registrable-domain"_s, WTFMove(registrableDomain)); - auto isPrewarmedProcess = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "is-prewarmed")); + auto isPrewarmedProcess = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "is-prewarmed"_s); if (!isPrewarmedProcess.isEmpty()) extraInitializationData.add("is-prewarmed"_s, isPrewarmedProcess); - auto isLockdownModeEnabled = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "enable-lockdown-mode")); + auto isLockdownModeEnabled = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "enable-lockdown-mode"_s); if (!isLockdownModeEnabled.isEmpty()) extraInitializationData.add("enable-lockdown-mode"_s, isLockdownModeEnabled); if (!isClientSandboxed()) { - auto userDirectorySuffix = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "user-directory-suffix")); + auto userDirectorySuffix = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "user-directory-suffix"_s); if (!userDirectorySuffix.isEmpty()) extraInitializationData.add("user-directory-suffix"_s, userDirectorySuffix); } - auto alwaysRunsAtBackgroundPriority = String::fromLatin1(xpc_dictionary_get_string(extraDataInitializationDataObject, "always-runs-at-background-priority")); + auto alwaysRunsAtBackgroundPriority = xpc_dictionary_get_wtfstring(extraDataInitializationDataObject, "always-runs-at-background-priority"_s); if (!alwaysRunsAtBackgroundPriority.isEmpty()) extraInitializationData.add("always-runs-at-background-priority"_s, alwaysRunsAtBackgroundPriority); diff --git a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm index d6022b6660660..c22a7735d325a 100644 --- a/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm +++ b/Source/WebKit/Shared/EntryPointUtilities/Cocoa/XPCService/XPCServiceMain.mm @@ -113,7 +113,7 @@ NEVER_INLINE NO_RETURN_DUE_TO_CRASH static void crashDueWebKitFrameworkVersionMi } static void checkFrameworkVersion(xpc_object_t message) { - auto uiProcessWebKitBundleVersion = String::fromLatin1(xpc_dictionary_get_string(message, "WebKitBundleVersion")); + auto uiProcessWebKitBundleVersion = xpc_dictionary_get_wtfstring(message, "WebKitBundleVersion"_s); auto webkitBundleVersion = ASCIILiteral::fromLiteralUnsafe(WEBKIT_BUNDLE_VERSION); if (!uiProcessWebKitBundleVersion.isNull() && uiProcessWebKitBundleVersion != webkitBundleVersion) { auto errorMessage = makeString("WebKit framework version mismatch: "_s, uiProcessWebKitBundleVersion, " != "_s, webkitBundleVersion); @@ -154,13 +154,12 @@ void XPCServiceEventHandler(xpc_connection_t peer) handleXPCExitMessage(event); #endif - auto* messageName = xpc_dictionary_get_string(event, "message-name"); + String messageName = xpc_dictionary_get_wtfstring(event, "message-name"_s); if (!messageName) { RELEASE_LOG_ERROR(IPC, "XPCServiceEventHandler: 'message-name' is not present in the XPC dictionary"); return; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - if (!strcmp(messageName, "bootstrap")) { + if (messageName == "bootstrap"_s) { WTF::initialize(); bool disableLogging = xpc_dictionary_get_bool(event, "disable-logging"); @@ -191,26 +190,25 @@ void XPCServiceEventHandler(xpc_connection_t peer) }); #endif - const char* serviceName = xpc_dictionary_get_string(event, "service-name"); + String serviceName = xpc_dictionary_get_wtfstring(event, "service-name"_s); if (!serviceName) { RELEASE_LOG_ERROR(IPC, "XPCServiceEventHandler: 'service-name' is not present in the XPC dictionary"); return; } CFStringRef entryPointFunctionName = nullptr; - if (!strncmp(serviceName, "com.apple.WebKit.WebContent", strlen("com.apple.WebKit.WebContent"))) { + if (serviceName.startsWith("com.apple.WebKit.WebContent"_s)) { s_isWebProcess = true; entryPointFunctionName = CFSTR(STRINGIZE_VALUE_OF(WEBCONTENT_SERVICE_INITIALIZER)); - } else if (!strcmp(serviceName, "com.apple.WebKit.Networking")) + } else if (serviceName == "com.apple.WebKit.Networking"_s) entryPointFunctionName = CFSTR(STRINGIZE_VALUE_OF(NETWORK_SERVICE_INITIALIZER)); - else if (!strcmp(serviceName, "com.apple.WebKit.GPU")) + else if (serviceName == "com.apple.WebKit.GPU"_s) entryPointFunctionName = CFSTR(STRINGIZE_VALUE_OF(GPU_SERVICE_INITIALIZER)); - else if (!strcmp(serviceName, "com.apple.WebKit.Model")) + else if (serviceName == "com.apple.WebKit.Model"_s) entryPointFunctionName = CFSTR(STRINGIZE_VALUE_OF(MODEL_SERVICE_INITIALIZER)); else { - RELEASE_LOG_ERROR(IPC, "XPCServiceEventHandler: Unexpected 'service-name': %{public}s", serviceName); + RELEASE_LOG_ERROR(IPC, "XPCServiceEventHandler: Unexpected 'service-name': %{public}s", serviceName.utf8().data()); return; } -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END CFBundleRef webKitBundle = CFBundleGetBundleWithIdentifier(CFSTR("com.apple.WebKit")); typedef void (*InitializerFunction)(xpc_connection_t, xpc_object_t); diff --git a/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm b/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm index db773f3f1f40e..e01717e1fa5e7 100644 --- a/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm +++ b/Source/WebKit/UIProcess/Launcher/cocoa/ProcessLauncherCocoa.mm @@ -449,7 +449,7 @@ static ASCIILiteral serviceName(const ProcessLauncher::LaunchOptions& launchOpti #endif if (event) - LOG_ERROR("Error while launching %s: %s", logName.data(), xpc_dictionary_get_string(event, XPC_ERROR_KEY_DESCRIPTION)); + LOG_ERROR("Error while launching %s: %s", logName.data(), xpc_dictionary_get_wtfstring(event, xpcErrorDescriptionKey).utf8().data()); else LOG_ERROR("Error while launching %s: No xpc_object_t event available.", logName.data()); @@ -508,9 +508,7 @@ static ASCIILiteral serviceName(const ProcessLauncher::LaunchOptions& launchOpti // launching and we already took care of cleaning things up. if (isLaunching() && xpc_get_type(reply) != XPC_TYPE_ERROR) { ASSERT(xpc_get_type(reply) == XPC_TYPE_DICTIONARY); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN - ASSERT(!strcmp(xpc_dictionary_get_string(reply, "message-name"), "process-finished-launching")); -WTF_ALLOW_UNSAFE_BUFFER_USAGE_END + ASSERT(xpc_dictionary_get_wtfstring(reply, "message-name"_s) == "process-finished-launching"_s); #if ASSERT_ENABLED mach_port_urefs_t sendRightCount = 0; diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm b/Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm index efc91f3332917..c0e927a23f558 100644 --- a/Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm +++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxyCocoa.mm @@ -62,11 +62,11 @@ if (!event || xpc_get_type(event) == XPC_TYPE_ERROR) return false; - auto* messageName = xpc_dictionary_get_string(event, XPCEndpoint::xpcMessageNameKey); - if (!messageName || !*messageName) + auto messageName = xpc_dictionary_get_wtfstring(event, XPCEndpoint::xpcMessageNameKey); + if (messageName.isEmpty()) return false; - if (LaunchServicesDatabaseXPCConstants::xpcLaunchServicesDatabaseXPCEndpointMessageName == messageName) { + if (messageName == LaunchServicesDatabaseXPCConstants::xpcLaunchServicesDatabaseXPCEndpointMessageName) { networkProcess->m_endpointMessage = event; for (auto& processPool : WebProcessPool::allProcessPools()) { for (Ref process : processPool->processes()) diff --git a/Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm b/Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm index 31937e28fd500..f44c76cef94fa 100644 --- a/Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm +++ b/Source/WebKit/WebProcess/cocoa/LaunchServicesDatabaseManager.mm @@ -48,8 +48,8 @@ void LaunchServicesDatabaseManager::handleEvent(xpc_object_t message) { - auto* messageName = xpc_dictionary_get_string(message, XPCEndpoint::xpcMessageNameKey); - if (LaunchServicesDatabaseXPCConstants::xpcUpdateLaunchServicesDatabaseMessageName == messageName) { + String messageName = xpc_dictionary_get_wtfstring(message, XPCEndpoint::xpcMessageNameKey); + if (messageName == LaunchServicesDatabaseXPCConstants::xpcUpdateLaunchServicesDatabaseMessageName) { #if HAVE(LSDATABASECONTEXT) auto database = xpc_dictionary_get_value(message, LaunchServicesDatabaseXPCConstants::xpcLaunchServicesDatabaseKey); diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp.py b/Tools/Scripts/webkitpy/style/checkers/cpp.py index f885f740b1d87..a55ed6b939ac7 100644 --- a/Tools/Scripts/webkitpy/style/checkers/cpp.py +++ b/Tools/Scripts/webkitpy/style/checkers/cpp.py @@ -3547,6 +3547,14 @@ def check_safer_cpp(clean_lines, line_number, error): if uses_strncmp: error(line_number, 'safercpp/strncmp', 4, "strncmp() is unsafe.") + uses_xpc_dictionary_get_data = search(r'xpc_dictionary_get_data\(', line) + if uses_xpc_dictionary_get_data: + error(line_number, 'safercpp/xpc_dictionary_get_data', 4, "Use xpc_dictionary_get_data_span() instead of xpc_dictionary_get_data().") + + uses_xpc_dictionary_get_string = search(r'xpc_dictionary_get_string\(', line) + if uses_xpc_dictionary_get_string: + error(line_number, 'safercpp/xpc_dictionary_get_string', 4, "Use xpc_dictionary_get_wtfstring() instead of xpc_dictionary_get_string().") + def check_style(clean_lines, line_number, file_extension, class_state, file_state, enum_state, error): """Checks rules from the 'C++ style rules' section of cppguide.html. @@ -4900,6 +4908,8 @@ class CppChecker(object): 'safercpp/strchr', 'safercpp/strstr', 'safercpp/timer_exception', + 'safercpp/xpc_dictionary_get_data', + 'safercpp/xpc_dictionary_get_string', 'security/assertion', 'security/assertion_fallthrough', 'security/javascriptcore_wtf_blockptr', diff --git a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py index 4271124658f9a..e18e170f25456 100644 --- a/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py +++ b/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py @@ -6328,6 +6328,16 @@ def test_safer_cpp(self): 'strncmp() is unsafe. [safercpp/strncmp] [4]', 'foo.cpp') + self.assert_lint( + 'auto* result = xpc_dictionary_get_data(dictionary, "foo", &size);', + 'Use xpc_dictionary_get_data_span() instead of xpc_dictionary_get_data(). [safercpp/xpc_dictionary_get_data] [4]', + 'foo.cpp') + + self.assert_lint( + 'auto* result = xpc_dictionary_get_string(dictionary, "foo");', + 'Use xpc_dictionary_get_wtfstring() instead of xpc_dictionary_get_string(). [safercpp/xpc_dictionary_get_string] [4]', + 'foo.cpp') + def test_ctype_fucntion(self): self.assert_lint( 'int i = isascii(8);', diff --git a/Tools/TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm b/Tools/TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm index daeb98724287a..712186c515151 100644 --- a/Tools/TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm +++ b/Tools/TestWebKitAPI/Tests/WebKit/XPCEndpoint.mm @@ -53,8 +53,8 @@ ASCIILiteral xpcEndpointNameKey() const final } void handleEvent(xpc_connection_t connection, xpc_object_t event) final { - auto* messageName = xpc_dictionary_get_string(event, XPCEndpoint::xpcMessageNameKey); - if (messageName && !strcmp(messageName, testMessageFromClient)) { + String messageName = xpc_dictionary_get_wtfstring(event, XPCEndpoint::xpcMessageNameKey); + if (messageName == testMessageFromClient) { endpointReceivedMessageFromClient = true; auto message = adoptOSObject(xpc_dictionary_create(nullptr, nullptr, 0)); @@ -68,8 +68,8 @@ void handleEvent(xpc_connection_t connection, xpc_object_t event) final private: void handleEvent(xpc_object_t event) final { - auto messageName = xpc_dictionary_get_string(event, XPCEndpoint::xpcMessageNameKey); - if (messageName && !strcmp(messageName, testMessageFromEndpoint)) + String messageName = xpc_dictionary_get_wtfstring(event, XPCEndpoint::xpcMessageNameKey); + if (messageName == testMessageFromEndpoint) clientReceivedMessageFromEndpoint = true; } void didConnect() final From 4ceef70587387dd74fd11e780c6d822657a7171f Mon Sep 17 00:00:00 2001 From: Tyler Wilcock Date: Sun, 26 Jan 2025 17:01:10 -0800 Subject: [PATCH 10/10] AX: Eagerly resolving style in AccessibilityObject::defaultObjectInclusion can cause crashes if done while a render tree update is already in progress https://bugs.webkit.org/show_bug.cgi?id=286538 rdar://143633880 Reviewed by Chris Fleizach. AccessibilityObject::style() eagerly resolves style if it is dirty, which is not always safe to do. This can cause a crash in this sequence: 1. An element that is part of a relation (e.g. aria-owns) gets dirty style 2. Relations get dirty, e.g. as the result of some element changing its id 3. Some element is removed via Element#removeChild, causing a render tree teardown to destroy its renderer, in turn causing AXIsolatedTree::removeNode to run 4. As part of AXIsolatedTree::removeNode, we call parentObject(), which resolves relations to determine ownerParentObject() 5. We call isIgnored() on the element with dirty style to add it to the relations map. This calls defaultObjectInclusion(), which uses style(), and eagerly resolves the the dirty style 6. This causes a crash, as we are downstream from an ongoing render tree update, and resolving style starts another render tree update, which crashes because you cannot perform re-entrant render tree updates. To fix this, this commit changes AccessibilityObject::style() to not resolve style if we are downstream of a render tree update. We can then rely on AXObjectCache::onStyleChange to perform the correct accessibility tree updates after the render tree update, at which point we actually have style that is safe to use. * LayoutTests/accessibility/dirty-style-and-relations-crash-expected.txt: Added. * LayoutTests/accessibility/dirty-style-and-relations-crash.html: Added. * LayoutTests/platform/ios/TestExpectations: Enable new test. * Source/WebCore/accessibility/AccessibilityObject.cpp: (WebCore::AccessibilityObject::speakAsProperty const): (WebCore::AccessibilityObject::insideLink const): (WebCore::AccessibilityObject::defaultObjectInclusion const): Canonical link: https://commits.webkit.org/289407@main --- ...rty-style-and-relations-crash-expected.txt | 7 ++ .../dirty-style-and-relations-crash.html | 69 +++++++++++++++++++ LayoutTests/platform/ios/TestExpectations | 1 + .../accessibility/AccessibilityObject.cpp | 22 +++--- .../accessibility/AccessibilityObject.h | 7 +- 5 files changed, 87 insertions(+), 19 deletions(-) create mode 100644 LayoutTests/accessibility/dirty-style-and-relations-crash-expected.txt create mode 100644 LayoutTests/accessibility/dirty-style-and-relations-crash.html diff --git a/LayoutTests/accessibility/dirty-style-and-relations-crash-expected.txt b/LayoutTests/accessibility/dirty-style-and-relations-crash-expected.txt new file mode 100644 index 0000000000000..642f16241661c --- /dev/null +++ b/LayoutTests/accessibility/dirty-style-and-relations-crash-expected.txt @@ -0,0 +1,7 @@ +This test passes ensures we don't crash after performing accessibility tree updates with dirty style and object relations. + +PASS: No crash. +PASS successfullyParsed is true + +TEST COMPLETE + diff --git a/LayoutTests/accessibility/dirty-style-and-relations-crash.html b/LayoutTests/accessibility/dirty-style-and-relations-crash.html new file mode 100644 index 0000000000000..f5bf748a469b4 --- /dev/null +++ b/LayoutTests/accessibility/dirty-style-and-relations-crash.html @@ -0,0 +1,69 @@ + + + + + + + + +
+
+
+ +
+ +
+
+ +
+
+
+
+ +
+
+
+
+
+ +
hello world
+ + + + diff --git a/LayoutTests/platform/ios/TestExpectations b/LayoutTests/platform/ios/TestExpectations index 10424c8a75404..db9798e5b6bca 100644 --- a/LayoutTests/platform/ios/TestExpectations +++ b/LayoutTests/platform/ios/TestExpectations @@ -2300,6 +2300,7 @@ accessibility/button-inside-label-ax-text.html [ Pass ] accessibility/changing-aria-hidden-with-display-none-parent.html [ Pass ] accessibility/checkbox-mixed-value.html [ Pass ] accessibility/dialog-slotted-content.html [ Pass ] +accessibility/dirty-style-and-relations-crash.html [ Pass ] accessibility/display-contents/dynamically-added-children.html [ Pass ] accessibility/display-contents/element-roles.html [ Pass ] accessibility/display-contents/listbox-item.html [ Pass ] diff --git a/Source/WebCore/accessibility/AccessibilityObject.cpp b/Source/WebCore/accessibility/AccessibilityObject.cpp index 1d64e46128e41..e20ff59e315bc 100644 --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityObject.cpp @@ -87,6 +87,7 @@ #include "RenderText.h" #include "RenderTextControl.h" #include "RenderTheme.h" +#include "RenderTreeBuilder.h" #include "RenderView.h" #include "RenderWidget.h" #include "RenderedPosition.h" @@ -2926,17 +2927,12 @@ const RenderStyle* AccessibilityObject::style() const if (auto* renderer = this->renderer()) return &renderer->style(); - auto* element = this->element(); - return element ? element->computedStyle() : nullptr; -} - -const RenderStyle* AccessibilityObject::existingStyle() const -{ - if (auto* renderer = this->renderer()) - return &renderer->style(); - - auto* element = this->element(); - return element ? element->existingComputedStyle() : nullptr; + RefPtr element = this->element(); + if (!element) + return nullptr; + // We cannot resolve style (as computedStyle() does) if we are downstream of an existing render tree + // update. Otherwise, a RELEASE_ASSERT preventing re-entrancy will be hit inside RenderTreeBuilder. + return RenderTreeBuilder::current() ? element->existingComputedStyle() : element->computedStyle(); } bool AccessibilityObject::isValueAutofillAvailable() const @@ -3934,7 +3930,7 @@ String AccessibilityObject::validationMessage() const AccessibilityObjectInclusion AccessibilityObject::defaultObjectInclusion() const { - if (auto* style = this->style()) { + if (const auto* style = this->style()) { if (style->effectiveInert()) return AccessibilityObjectInclusion::IgnoreObject; if (isVisibilityHidden(*style)) @@ -3950,7 +3946,7 @@ AccessibilityObjectInclusion AccessibilityObject::defaultObjectInclusion() const bool ignoreARIAHidden = isFocused(); if (Accessibility::findAncestor(*this, false, [&] (const auto& object) { - const auto* style = object.existingStyle(); + const auto* style = object.style(); if (style && WebCore::isRenderHidden(*style)) return true; diff --git a/Source/WebCore/accessibility/AccessibilityObject.h b/Source/WebCore/accessibility/AccessibilityObject.h index 0a2d4952477d6..4c759bf780647 100644 --- a/Source/WebCore/accessibility/AccessibilityObject.h +++ b/Source/WebCore/accessibility/AccessibilityObject.h @@ -243,13 +243,8 @@ class AccessibilityObject : public AXCoreObject, public CanMakeWeakPtr