2023383 - use image::SurfaceCache::IsLegalSize over gfx::Factory::AllowedSurfaceSize in SourceSurfaceSharedDataWrapper

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2023383. Use image::SurfaceCache::IsLegalSize over gfx::Factory::AllowedSurfaceSize in SourceSurfaceSharedDataWrapper. r?#gfx-reviewers

SurfaceCache::IsLegalSize is what is used to determine if imgFrame's are valid, and that is what is on the content process side of this parent process code. So using the same function is what we want.

Pratically, on trunk SurfaceCache::IsLegalSize and Factory::AllowedSurfaceSize are basically identical (reject if one dimension is over 64k or total area can't fit in an int32_t), but that is only true after bug 1911583 which expanded that from 32k to 64k. But on esr140, that doesn't have bug 1911583, so these functions are different and we want to uplift the AllowedSurfaceSize check there.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2f89d2a7dbe4
https://hg.mozilla.org/integration/autoland/rev/b6d62fe0693f
Use image::SurfaceCache::IsLegalSize over gfx::Factory::AllowedSurfaceSize in SourceSurfaceSharedDataWrapper. r=gfx-reviewers,lsalzman

Comment 3

[amarc]

https://hg.mozilla.org/mozilla-central/rev/b6d62fe0693f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR140 uplift when you get a chance.

Comment 5

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2023383. Use image::SurfaceCache::IsLegalSize over gfx::Factory::AllowedSurfaceSize in SourceSurfaceSharedDataWrapper.

SurfaceCache::IsLegalSize is what is used to determine if imgFrame's are valid, and that is what is on the content process side of this parent process code. So using the same function is what we want.

Pratically, on trunk SurfaceCache::IsLegalSize and Factory::AllowedSurfaceSize are basically identical (reject if one dimension is over 64k or total area can't fit in an int32_t), but that is only true after bug 1911583 which expanded that from 32k to 64k. But on esr140, that doesn't have bug 1911583, so these functions are different and we want to uplift the AllowedSurfaceSize check there.

Comment 6

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: fix needed for esr140 but keeping trunk, beta, and esr140 all in sync is best practice
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: uses the same check on parent side as client side uses to validate images
• String changes made/needed?: none
• Is Android affected?: yes

Comment 7

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 2023383. Use image::SurfaceCache::IsLegalSize over gfx::Factory::AllowedSurfaceSize in SourceSurfaceSharedDataWrapper.

SurfaceCache::IsLegalSize is what is used to determine if imgFrame's are valid, and that is what is on the content process side of this parent process code. So using the same function is what we want.

Pratically, on trunk SurfaceCache::IsLegalSize and Factory::AllowedSurfaceSize are basically identical (reject if one dimension is over 64k or total area can't fit in an int32_t), but that is only true after bug 1911583 which expanded that from 32k to 64k. But on esr140, that doesn't have bug 1911583, so these functions are different and we want to uplift the AllowedSurfaceSize check there.

Comment 8

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: fix needed to make sec bug bug 2018113 work properly on esr140
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: uses the same check on parent side as client side uses to validate images
• String changes made/needed?: none
• Is Android affected?: yes

Comment 9

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/c127ad4a263b
https://hg.mozilla.org/releases/mozilla-beta/rev/e04a62bfcbfa

Comment 10

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/ebc913dcb2ab