Bug 295239 – ASAN_TRAP | Style::CSSValueConversion::operator; Style::BuilderFunctions::applyValueScrollPaddingRight; Style::BuilderGenerated::applyProperty

• Component: CSS
• Status: RESOLVED FIXED

[John Wilander]

Created attachment 475712 [details]
Repro case

<rdar://154646251>

See attached repro case.

Stack Trace
=========

frame #0: WebCore`WTFCrashWithInfo(int, char const*, char const*, int)+0x24
frame #1: WebCore`WebCore::Style::CSSValueConversion<WebCore::Style::ScrollPaddingEdge>::operator()(WebCore::Style::BuilderState&, WebCore::CSSValue const&)+0x380
frame #2: WebCore`WebCore::Style::BuilderFunctions::applyValueScrollPaddingRight(WebCore::Style::BuilderState&, WebCore::CSSValue&)+0xa8
frame #3: WebCore`WebCore::Style::BuilderGenerated::applyProperty(WebCore::CSSPropertyID, WebCore::Style::BuilderState&, WebCore::CSSValue&, WebCore::Style::ApplyValueType)+0xc97c
frame #4: WebCore`WebCore::Style::Builder::applyProperty(WebCore::CSSPropertyID, WebCore::CSSValue&, WebCore::SelectorChecker::LinkMatchMask, WebCore::Style::CascadeLevel)+0x2b0
frame #5: WebCore`WebCore::Style::Builder::applyNonHighPriorityProperties()+0x218
frame #6: WebCore`WebCore::Style::Resolver::applyMatchedProperties(WebCore::Style::Resolver::State&, WebCore::Style::MatchResult const&, WebCore::Style::PropertyCascade::IncludedProperties&&)+0x43c
frame #7: WebCore`WebCore::Style::Resolver::unadjustedStyleForElement(WebCore::Element&, WebCore::Style::ResolutionContext const&, WebCore::RuleMatchingBehavior)+0x568
frame #8: WebCore`WebCore::Style::TreeResolver::styleForStyleable(WebCore::Styleable const&, WebCore::Style::TreeResolver::ResolutionType, WebCore::Style::ResolutionContext const&, WebCore::RenderStyle const*)+0x600
frame #9: WebCore`WebCore::Style::TreeResolver::resolveElement(WebCore::Element&, WebCore::RenderStyle const*, WebCore::Style::TreeResolver::ResolutionType)+0x374
frame #10: WebCore`WebCore::Style::TreeResolver::resolveComposedTree()+0x1000
frame #11: WebCore`WebCore::Style::TreeResolver::resolve()+0x4d0
frame #12: WebCore`WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)+0x444
frame #13: WebCore`WebCore::Document::updateStyleIfNeeded()+0x1f4
frame #14: WebCore`WebCore::Document::updateLayout(WTF::OptionSet<WebCore::LayoutOptions>, WebCore::Element const*)+0x228
frame #15: WebCore`WebCore::LocalDOMWindow::scrollTo(WebCore::ScrollToOptions const&, WebCore::ScrollClamping, WebCore::ScrollSnapPointSelectionMethod, std::__1::optional<WebCore::FloatSize>) const+0x500
frame #16: WebCore`WebCore::DOMWindow::scrollTo(WebCore::ScrollToOptions const&, WebCore::ScrollClamping, WebCore::ScrollSnapPointSelectionMethod, std::__1::optional<WebCore::FloatSize>) const+0x4c
frame #17: WebCore`WebCore::jsDOMWindowInstanceFunction_scroll1Body(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)+0x244
frame #18: WebCore`WebCore::jsDOMWindowInstanceFunction_scrollOverloadDispatcher(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDOMWindow*)+0x1bc
frame #19: WebCore`WebCore::jsDOMWindowInstanceFunction_scroll(JSC::JSGlobalObject*, JSC::CallFrame*)+0x220
frame #20: `0x00014acc8040+
frame #21: `0x00014acb8084+
frame #22: `0x00014acb8544+
frame #23: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0xc54
frame #24: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x224
frame #25: WebCore`WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x1e8
frame #26: WebCore`WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0x864
frame #27: WebCore`WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener>>, 1ul, WTF::CrashOnOverflow, 2ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)+0x4c0
frame #28: WebCore`WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const+0xa58
frame #29: WebCore`WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)+0x208
frame #30: WebCore`WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)+0x18b8
frame #31: WebCore`WebCore::FrameSelection::selectAll()+0x568
frame #32: WebCore`WebCore::executeSelectAll(WebCore::LocalFrame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+0x40
frame #33: WebCore`WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0x1ac
frame #34: WebCore`WebCore::Document::execCommand(WTF::String const&, bool, mpark::variant<WTF::String, WTF::RefPtr<WebCore::TrustedHTML, WTF::RawPtrTraits<WebCore::TrustedHTML>, WTF::DefaultRefDerefTraits<WebCore::TrustedHTML>>> const&)+0x32c
frame #35: WebCore`WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)+0x4f0
frame #36: WebCore`WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)+0x158
frame #37: `0x00014acc8040+
frame #38: `0x00014acb8084+
frame #39: `0x00014acb8064+
frame #40: `0x00014acb8544+
frame #41: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0xc54
frame #42: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x224
frame #43: WebCore`WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x1e8
frame #44: WebCore`WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0x864
frame #45: WebCore`WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener>>, 1ul, WTF::CrashOnOverflow, 2ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)+0x4c0
frame #46: WebCore`WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const+0xa58
frame #47: WebCore`WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)+0x208
frame #48: WebCore`WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)+0x18b8
frame #49: WebCore`WebCore::simulateClick(WebCore::Element&, WebCore::Event*, WebCore::SimulatedClickMouseEventOptions, WebCore::SimulatedClickVisualOptions, WebCore::SimulatedClickSource)+0x474
frame #50: WebCore`WebCore::jsHTMLElementPrototypeFunction_click(JSC::JSGlobalObject*, JSC::CallFrame*)+0x230
frame #51: `0x00014acc8040+
frame #52: `0x00014acb8084+
frame #53: `0x00014acb8544+
frame #54: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0xc54
frame #55: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x224
frame #56: WebCore`WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x1e8
frame #57: WebCore`WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0x864
frame #58: WebCore`WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener>>, 1ul, WTF::CrashOnOverflow, 2ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)+0x4c0
frame #59: WebCore`WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const+0xa58
frame #60: WebCore`WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)+0x208
frame #61: WebCore`WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)+0x18b8
frame #62: WebCore`WebCore::FrameSelection::selectAll()+0x568
frame #63: WebCore`WebCore::executeSelectAll(WebCore::LocalFrame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)+0x40
frame #64: WebCore`WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const+0x1ac
frame #65: WebCore`WebCore::Document::execCommand(WTF::String const&, bool, mpark::variant<WTF::String, WTF::RefPtr<WebCore::TrustedHTML, WTF::RawPtrTraits<WebCore::TrustedHTML>, WTF::DefaultRefDerefTraits<WebCore::TrustedHTML>>> const&)+0x32c
frame #66: WebCore`WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)+0x4f0
frame #67: WebCore`WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*)+0x158
frame #68: `0x00014acc8040+
frame #69: `0x00014acb8084+
frame #70: `0x00014acb8544+
frame #71: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0xc54
frame #72: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x224
frame #73: WebCore`WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x1e8
frame #74: WebCore`WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0x864
frame #75: WebCore`WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener>>, 1ul, WTF::CrashOnOverflow, 2ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)+0x4c0
frame #76: WebCore`WebCore::EventContext::handleLocalEvents(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) const+0xa58
frame #77: WebCore`WebCore::dispatchEventInDOM(WebCore::Event&, WebCore::EventPath const&)+0x208
frame #78: WebCore`WebCore::EventDispatcher::dispatchEvent(WebCore::Node&, WebCore::Event&)+0x18b8
frame #79: WebCore`WebCore::ScopedEventQueue::dispatchEvent(WebCore::ScopedEventQueue::ScopedEvent const&) const+0x160
frame #80: WebCore`WebCore::EventDispatcher::dispatchScopedEvent(WebCore::Node&, WebCore::Event&)+0x28c
frame #81: WebCore`WebCore::ContainerNode::replaceAll(WebCore::Node*)+0x940
frame #82: WebCore`WebCore::ContainerNode::stringReplaceAll(WTF::String&&)+0x130
frame #83: WebCore`WebCore::Node::setTextContent(WTF::String&&)+0x54
frame #84: WebCore`WebCore::setJSNode_textContentSetter(JSC::JSGlobalObject&, WebCore::JSNode&, JSC::JSValue)+0x224
frame #85: WebCore`WebCore::setJSNode_textContent(JSC::JSGlobalObject*, long long, long long, JSC::PropertyName)+0x154
frame #86: JavaScriptCore`JSC::JSObject::putInlineSlow(JSC::JSGlobalObject*, JSC::PropertyName, JSC::JSValue, JSC::PutPropertySlot&)+0xcc8
frame #87: JavaScriptCore`llint_slow_path_put_by_id+0x13ac
frame #88: JavaScriptCore`jsc_llint_llintOpWithMetadata__llintOpWithReturn__llintOp__commonOp__fn__fn__makeReturn__fn__fn__2116_fn__opPutByIdSlow_LowLevelInterpreter_asm_510+0xc
frame #89: `0x00014acb8064+
frame #90: `0x00014acb8544+
frame #91: JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)+0xc54
frame #92: JavaScriptCore`JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x224
frame #93: WebCore`WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)+0x1e8
frame #94: WebCore`WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)+0x864
frame #95: WebCore`WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::RawPtrTraits<WebCore::RegisteredEventListener>, WTF::DefaultRefDerefTraits<WebCore::RegisteredEventListener>>, 1ul, WTF::CrashOnOverflow, 2ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase)+0x4c0
frame #96: WebCore`WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase)+0x5a0
frame #97: WebCore`WebCore::LocalDOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*)+0x328
frame #98: WebCore`WebCore::LocalDOMWindow::dispatchLoadEvent()+0x404
frame #99: WebCore`WebCore::Document::dispatchWindowLoadEvent()+0x12c
frame #100: WebCore`WebCore::Document::implicitClose()+0x588
frame #101: WebCore`WebCore::FrameLoader::checkCallImplicitClose()+0x1cc
frame #102: WebCore`WebCore::FrameLoader::checkCompleted()+0x438
frame #103: WebCore`WebCore::CachedResourceLoader::loadDone(WebCore::LoadCompletionType, bool)+0x220
frame #104: WebCore`WebCore::SubresourceLoader::notifyDone(WebCore::LoadCompletionType)+0x1a8
frame #105: WebCore`WebCore::SubresourceLoader::didFinishLoading(WebCore::NetworkLoadMetrics const&)+0xde0
frame #106: WebCore`_ZZZN7WebCore14ResourceLoader11loadDataURLEvEN3$_0clINSt3__18optionalINS_14DataURLDecoder6ResultEEEEEDaT_ENKUlvE_clEv+0x1cc
frame #107: WebCore`WTF::Detail::CallableWrapper<WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse&&, WTF::CompletionHandler<void ()>&&)::$_2, void>::~CallableWrapper()+0xc4
frame #108: WebCore`WebCore::ResourceLoader::didReceiveResponse(WebCore::ResourceResponse&&, WTF::CompletionHandler<void ()>&&)+0x67c
frame #109: WebCore`WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse&&, WTF::CompletionHandler<void ()>&&)+0x240c
frame #110: WebCore`_ZZN7WebCore14ResourceLoader11loadDataURLEvEN3$_0clINSt3__18optionalINS_14DataURLDecoder6ResultEEEEEDaT_+0x22c
frame #111: WebCore`WTF::Detail::CallableWrapper<WebCore::ResourceLoader::loadDataURL()::$_0, void, std::__1::optional<WebCore::DataURLDecoder::Result>>::call(std::__1::optional<WebCore::DataURLDecoder::Result>)+0xc0
frame #112: WebCore`WTF::Function<void (std::__1::optional<WebCore::DataURLDecoder::Result>)>::operator()(std::__1::optional<WebCore::DataURLDecoder::Result>) const+0x11c
frame #113: WebCore`WebCore::DataURLDecoder::decode(WTF::URL const&, WebCore::DataURLDecoder::ScheduleContext const&, WebCore::DataURLDecoder::ShouldValidatePadding, WTF::Function<void (std::__1::optional<WebCore::DataURLDecoder::Result>)>&&)::$_0::operator()()::'lambda'()::operator()()+0xc8
frame #114: JavaScriptCore`WTF::RunLoop::dispatch(WTF::HashSet<WTF::RefPtr<WTF::SchedulePair, WTF::RawPtrTraits<WTF::SchedulePair>, WTF::DefaultRefDerefTraits<WTF::SchedulePair>>, WTF::SchedulePairHash, WTF::HashTraits<WTF::RefPtr<WTF::SchedulePair, WTF::RawPtrTraits<WTF::SchedulePair>, WTF::DefaultRefDerefTraits<WTF::SchedulePair>>>, WTF::HashTableTraits, (WTF::ShouldValidateKey)0> const&, WTF::Function<void ()>&&)::$_0::__invoke(__CFRunLoopTimer*, void*)+0xe0
frame #115: CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__+0x1c
frame #116: CoreFoundation`__CFRunLoopDoTimer+0x3d0
frame #117: CoreFoundation`__CFRunLoopDoTimers+0x114
frame #118: CoreFoundation`__CFRunLoopRun+0x72c
frame #119: CoreFoundation`_CFRunLoopRunSpecificWithOptions+0x230
frame #120: Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:]+0xd0
frame #121: Foundation`-[NSRunLoop(NSRunLoop) run]+0x3c
frame #122: libxpc.dylib`_xpc_objc_main+0x2b8
frame #123: libxpc.dylib`_xpc_main+0x24
frame #124: libxpc.dylib`xpc_main+0x3c
frame #125: WebKit`WebKit::XPCServiceMain(int, char const**)+0xa4
frame #126: dyld`start+0x1b7c

[Darin Adler]

Should we be adding Sam Weinig to the CC list on this?

[Frédéric Wang (:fredw)]

Created attachment 475742 [details]
Minimized and cleaned up repro (for via applyValueHeight)

[Frédéric Wang (:fredw)]

Created attachment 475743 [details]
Minimized and cleaned up repro (for via applyValueScrollPaddingRight)

[Frédéric Wang (:fredw)]

The backtrace in this bug report (applyValueScrollPaddingRight) does not match the one of the repro case (applyValueHeight).

Anyway, this is very similar to bug 295241, I'm providing two minimized tests obtained by just tweaking the CSS property of the inner element tweaked.

[Claudio Saavedra]

The three crashes in this bug, bug 295240, and bug 295241 are a regression after https://commits.webkit.org/296172@main .

[Darin Adler]

Should we revert that change and try to do it again without introducing these regressions?

[Darin Adler]

And again, should we be letting Sam Weinig see these bug reports?

[Claudio Saavedra]

I think Sam's input would be useful, but I don't think I'm in a position to decide whether access can be granted to him. Same about the question about reverting. John? Ryosuke?

[Frédéric Wang (:fredw)]

Just to explain a bit more the issue, the all these bug reports seem to hit this code:

https://searchfox.org/wubkat/rev/71ff127bf571a46d302baa3b98f1bf9129d9f84a/Source/WebCore/style/values/primitives/StyleLengthWrapper%2BCSSValueConversion.h#49

and the call to CSSPrimitiveValue::resolveAsLength reaches that line:

https://searchfox.org/wubkat/rev/71ff127bf571a46d302baa3b98f1bf9129d9f84a/Source/WebCore/style/values/primitives/StyleLengthResolution.cpp#403

In all the attached reduced test cases, a length value of 0px is used (value = 0 and lengthUnit = Px) within nested div increasing zoom level (conversionData.zoom() = infinity) and the result of the multiplication is is -nan.

Finally, we hit this RELEASE_ASSERT because the length value (-nan) is not within the required range (which depends on the CSS property but can be for instance between 0 and +inf):

https://searchfox.org/wubkat/rev/71ff127bf571a46d302baa3b98f1bf9129d9f84a/Source/WebCore/style/values/primitives/StyleLengthWrapper.h#189

Likely we can get similar assertion failure with slightly different test configuration (property, unit, value, ...) but this seems to be the general pattern.

[Frédéric Wang (:fredw)]

(In reply to Frédéric Wang (:fredw) from comment #9)
> In all the attached reduced test cases, a length value of 0px is used (value
> = 0 and lengthUnit = Px) within nested div increasing zoom level
> (conversionData.zoom() = infinity) and the result of the multiplication is
> is -nan.
> 

Not sure yet whether this is what we want, but the following small patch fixes all the crashes for the testcases attached here and on bug 295240 and bug 295241. It sounds sensible to use 0 if the value is zero, even if the zoom is "infinite".

diff --git a/Source/WebCore/style/values/primitives/StyleLengthResolution.cpp b/Source/WebCore/style/values/primitives/StyleLengthResolution.cpp
index 5a10fa7e63b4..05ecfb059f6f 100644
--- a/Source/WebCore/style/values/primitives/StyleLengthResolution.cpp
+++ b/Source/WebCore/style/values/primitives/StyleLengthResolution.cpp
@@ -400,6 +400,9 @@ double computeNonCalcLengthDouble(double value, CSS::LengthUnit lengthUnit, cons
     if (conversionData.computingFontSize() || isFontOrRootFontRelativeLength(lengthUnit))
         return value;
 
+    if (value == 0)
+        return 0;
+
     return value * conversionData.zoom();
 }

[Frédéric Wang (:fredw)]

Created attachment 475779 [details]
Patch with tests

Claudio mentioned the infinite zoom factor is visible on the embargoed branch, but that's only causing the release assert after sam's refactoring some weeks ago on main. So we cannot submit a patch to the embargoed branch until it is rebased to more recent commits.

I'm attaching a patch with the fix proposed in comment 10 as well as all testcases attached to bug 295239, bug 295240, and bug 295241 (from the original repros, or the reported backtraces).

[Claudio Saavedra]

I think it's very likely that we could land this directly in main, but some feedback would be appreciated.

[Frédéric Wang (:fredw)]

(In reply to Claudio Saavedra from comment #12)
> I think it's very likely that we could land this directly in main, but some
> feedback would be appreciated.

Right, given the analysis from comment 9 it's likely not a security bug. So we can just make this bug public (and a fortiori, replying to Darin, share cc Sam). But yeah, let's see what John or Ryosuke think about it.

[Darin Adler]

*** Bug 295240 has been marked as a duplicate of this bug. ***

[Darin Adler]

*** Bug 295241 has been marked as a duplicate of this bug. ***

[Claudio Saavedra]

Sam, wdyt?

[Sam Weinig]

I am not sure this is the right approach, as I don't think this really gets to the heart of the problem, which is the infinite used zoom. 

Instead, I think should probably do two things:

1. Cap the value the used zoom to some finite max value in RenderStyle::setZoom(). (We already enforce a minimum, so this would be giving it a maximum). 

```
inline bool RenderStyle::setZoom(float zoomLevel)
{
    setUsedZoom(clampTo<float>(usedZoom() * zoomLevel, std::numeric_limits<float>::epsilon(), std::numeric_limits<float>::max()));
    if (compareEqual(m_nonInheritedData->rareData->zoom, zoomLevel))
        return false;
    m_nonInheritedData.access().rareData.access().zoom = zoomLevel;
    return true;
}
```

2. Update clamping in CSSPrimitiveNumericRange.h clamping functions to use WTF::clampTo rather than std::clamp, to ensure that NaN is not propagated. We should also enforce finite minimums/maximums even when one of the Range values is +/- infinity.

[Sam Weinig]

(also, I think I have access, or should have access, to all the security bugs, as I one of the individual members of the WebKit Security group team, https://webkit.org/security-team/, so just ping me for any of these).

[Darin Adler]

Seems to me that we could do the "wrong" fix first, and then move to the "right" one soon after. But no matter how we fix this, we’d want maximum/infinity regression test cases, maybe even in WPT.

[Sam Weinig]

I'm unclear on the state of things here. Is this still being treated like a security bug? Or can I make a PR with a fix and test cases?

[Darin Adler]

Need a security expert to confirm, but I suspect a RELEASE_ASSERT meant this need not be treated like a security bug.

[John Wilander]

We've concluded that this bug doesn't have security implications.

[Frédéric Wang (:fredw)]

Comment on attachment 475779 [details]
Patch with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=475779&action=review

> LayoutTests/fast/css/resolve-zero-length-with-infinite-zoom-factor-crash.html:14
> +                <div style="-webkit-logical-height: 0px;"></div>-->

This line has an unnecessary closing comment that should be removed.

[Frédéric Wang (:fredw)]

(In reply to Sam Weinig from comment #18)
> (also, I think I have access, or should have access, to all the security
> bugs, as I one of the individual members of the WebKit Security group team,
> https://webkit.org/security-team/, so just ping me for any of these).

👍

(In reply to Sam Weinig from comment #20)
> I'm unclear on the state of things here. Is this still being treated like a
> security bug? Or can I make a PR with a fix and test cases?

It has been move out of the security component and it seems you already have a plan, so I'm reassigning to you.

(In reply to Sam Weinig from comment #17)
> I am not sure this is the right approach, as I don't think this really gets
> to the heart of the problem, which is the infinite used zoom. 

Yes, that was implicit in comment 10 and comment 11, we were also considering clamping the zoom level to finite values. But the infinite zoom level seems to be something that has already been there for a long time, so changing that behavior may have a bigger impact than just working it around in computeNonCalcLengthDouble. Let's try it and if it causes unexpected issue we can always reconsider the "wrong" but "safer" fix.

[Claudio Saavedra]

Comment on attachment 475779 [details]
Patch with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=475779&action=review

>> LayoutTests/fast/css/resolve-zero-length-with-infinite-zoom-factor-crash.html:14
>> +                <div style="-webkit-logical-height: 0px;"></div>-->
> 
> This line has an unnecessary closing comment that should be removed.

This should be replaced with standard properties, like block-size.

> LayoutTests/fast/css/resolve-zero-length-with-infinite-zoom-factor-crash.html:25
> +                <div style="-webkit-padding-after: 0px;"></div>

These too.

[Frédéric Wang (:fredw)]

Comment on attachment 475779 [details]
Patch with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=475779&action=review

>>> LayoutTests/fast/css/resolve-zero-length-with-infinite-zoom-factor-crash.html:14
>>> +                <div style="-webkit-logical-height: 0px;"></div>-->
>> 
>> This line has an unnecessary closing comment that should be removed.
> 
> This should be replaced with standard properties, like block-size.

For this and below, I got them from the original test cases and I'm not sure if unprefixed equivalents give the same backtrace as in the original reports (I believe I tried, but probably not very hard).

[Sam Weinig]

Pull request: https://github.com/WebKit/WebKit-security/pull/118

[Sam Weinig]

git-webkit required the PR be made to the WebKit-security fork, but there is the PR. Going to see what the bots think.

[Sam Weinig]

What’s going on with this? I see there is now another PR up? https://github.com/WebKit/WebKit/pull/47728 (https://bugs.webkit.org/show_bug.cgi?id=295543)

Apple folks, which bug are you using to track this issue?

[Sam Weinig]

*** Bug 295543 has been marked as a duplicate of this bug. ***

[Sam Weinig]

*** Bug 295593 has been marked as a duplicate of this bug. ***

[Sam Weinig]

*** Bug 295609 has been marked as a duplicate of this bug. ***

[Sam Weinig]

Pull request: https://github.com/WebKit/WebKit/pull/47742

[Sam Weinig]

Re-opened the PR in the security fork: https://github.com/WebKit/WebKit/pull/47742

[EWS]

Committed 297165@main (512db2970b38): <https://commits.webkit.org/297165@main>

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