Skip to content

Fix std::accumulate truncating float values in parseKeyTimes assertion#61569

Merged
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
Ahmad-S792:eng/Fix-std-accumulate-truncating-float-values-in-parseKeyTimes-assertion
Mar 29, 2026
Merged

Fix std::accumulate truncating float values in parseKeyTimes assertion#61569
webkit-commit-queue merged 1 commit into
WebKit:mainfrom
Ahmad-S792:eng/Fix-std-accumulate-truncating-float-values-in-parseKeyTimes-assertion

Conversation

@Ahmad-S792
Copy link
Copy Markdown
Contributor

@Ahmad-S792 Ahmad-S792 commented Mar 28, 2026

@Ahmad-S792 Ahmad-S792 self-assigned this Mar 28, 2026
@Ahmad-S792 Ahmad-S792 added the SVG For bugs in the SVG implementation. label Mar 28, 2026
@Ahmad-S792 Ahmad-S792 added the merge-queue Applied to send a pull request to merge-queue label Mar 29, 2026
https://bugs.webkit.org/show_bug.cgi?id=310972
rdar://173577708

Reviewed by Antoine Quint.

The ASSERT used 0 (int) as the initial value, causing all float
values to be truncated to int during accumulation, making the
assertion always trivially pass. Use 0.0f instead.

* Source/WebCore/svg/SVGAnimationElement.cpp:
(WebCore::parseKeyTimes):

Canonical link: https://commits.webkit.org/310175@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Fix-std-accumulate-truncating-float-values-in-parseKeyTimes-assertion branch from 4d2dccd to 4e73e06 Compare March 29, 2026 07:26
@webkit-commit-queue
Copy link
Copy Markdown
Collaborator

Committed 310175@main (4e73e06): https://commits.webkit.org/310175@main

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

@webkit-commit-queue webkit-commit-queue merged commit 4e73e06 into WebKit:main Mar 29, 2026
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 29, 2026

if (verifyOrder && !result.isEmpty() && !result.last()) {
ASSERT(!std::accumulate(result.begin(), result.end(), 0));
ASSERT(!std::accumulate(result.begin(), result.end(), 0.0f));
Copy link
Copy Markdown
Contributor

@shallawa shallawa Mar 30, 2026

Choose a reason for hiding this comment

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

I am not sure what the purpose of this change is. This ASSERT is in fact checks if all the elements of result are zeros. If verifyOrder is true and the last element of result is zero, then all the elements before it must be zeros. Elements of result cannot be negative since we return if time is negative:

        if (!ok || time < 0 || time > 1)
            return { };

And a time can't be added unless it is in order:

        if (verifyOrder && (result.isEmpty() ? time : time < result.last()))
            return { };

Maybe using std::all_of() would make the purpose of this ASSERT clearer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@shallawa - Something like this?

ASSERT(std::all_of(result.begin(), result.end(), [](float time) { return !time; }));

@Ahmad-S792 Ahmad-S792 deleted the eng/Fix-std-accumulate-truncating-float-values-in-parseKeyTimes-assertion branch April 10, 2026 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

SVG For bugs in the SVG implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants