Fix crash in OffscreenCanvas when using sibling-index() in font calc [chromium/src : main]
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
Hi, could you take a look at this? thanks!
Related details
- Munira Tursunova
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.This is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Related details
- Munira Tursunova
- zhenbang Jiang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.This is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Added relevant comments as discussed. Pls take another look.
Related details
- Munira Tursunova
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rune Lillesveen (Gerrit)
Rune Lillesveen added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.zhenbang JiangThis is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Added relevant comments as discussed. Pls take another look.
I forgot, but this was discussed in https://github.com/w3c/csswg-drafts/issues/10982 where the sentiment is to disallow sibling-index/count() at parse time when there is no element context:
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2724587029
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2725546630
So, I think we should make sure these values are not allowed instead of using some default index.
For this canvas case, it could be solved by making FontStyleResolver::ComputeFont() check that the values are not element-dependent before doing the resolution.
Related details
- Munira Tursunova
- zhenbang Jiang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.zhenbang JiangThis is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Rune LillesveenAdded relevant comments as discussed. Pls take another look.
I forgot, but this was discussed in https://github.com/w3c/csswg-drafts/issues/10982 where the sentiment is to disallow sibling-index/count() at parse time when there is no element context:
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2724587029
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2725546630So, I think we should make sure these values are not allowed instead of using some default index.
For this canvas case, it could be solved by making FontStyleResolver::ComputeFont() check that the values are not element-dependent before doing the resolution.
Thanks for the reminder and guidance!
I’ve moved the element-dependent calc validation to FontStyleResolver::ComputeFont() as suggested, aligning with CSSWG’s intent to disallow sibling-index/count() where there’s no element context.
I also fixed the font-style edge case: the logic now handles both regular CSSPrimitiveValue and CSSFontStyleRangeValue (oblique ranges), scanning nested values to block invalid functions.
All relevant font properties (font-size, font-stretch, font-style, font-weight) are covered, with invalid values skipped entirely.
Related details
- Munira Tursunova
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.zhenbang JiangThis is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Rune LillesveenAdded relevant comments as discussed. Pls take another look.
zhenbang JiangI forgot, but this was discussed in https://github.com/w3c/csswg-drafts/issues/10982 where the sentiment is to disallow sibling-index/count() at parse time when there is no element context:
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2724587029
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2725546630So, I think we should make sure these values are not allowed instead of using some default index.
For this canvas case, it could be solved by making FontStyleResolver::ComputeFont() check that the values are not element-dependent before doing the resolution.
Thanks for the reminder and guidance!
I’ve moved the element-dependent calc validation to FontStyleResolver::ComputeFont() as suggested, aligning with CSSWG’s intent to disallow sibling-index/count() where there’s no element context.
I also fixed the font-style edge case: the logic now handles both regular CSSPrimitiveValue and CSSFontStyleRangeValue (oblique ranges), scanning nested values to block invalid functions.
All relevant font properties (font-size, font-stretch, font-style, font-weight) are covered, with invalid values skipped entirely.
Correction: I just noticed my unit tests are failing. Need to revisit this issue. Please disregard my previous comment.
Related details
- Munira Tursunova
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
zhenbang Jiang (Gerrit)
zhenbang Jiang added 1 comment![]( "Open in Gerrit")
// In OffscreenCanvas or other contexts without DOM elements, return 0.zhenbang JiangThis is not at all specified afaict from the spec.
It's not obvious to me whether the correct behavior is to disallow these functions at parse time or be handled like this.
Given the resolution to '0' for inner tree reference, I'm fine with making this 0 for now:
"A tree counting function is a tree-scoped reference where it references an implicit tree-scoped name for the element it resolves against. This is done to not leak tree information to an outer tree. A tree counting function that is scoped to an outer tree relative to the element it resolves against, will always resolve to 0."
Could you extend the comment to say that this is not covered by the spec?
Rune LillesveenAdded relevant comments as discussed. Pls take another look.
zhenbang JiangI forgot, but this was discussed in https://github.com/w3c/csswg-drafts/issues/10982 where the sentiment is to disallow sibling-index/count() at parse time when there is no element context:
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2724587029
https://github.com/w3c/csswg-drafts/issues/10982#issuecomment-2725546630So, I think we should make sure these values are not allowed instead of using some default index.
For this canvas case, it could be solved by making FontStyleResolver::ComputeFont() check that the values are not element-dependent before doing the resolution.
zhenbang JiangThanks for the reminder and guidance!
I’ve moved the element-dependent calc validation to FontStyleResolver::ComputeFont() as suggested, aligning with CSSWG’s intent to disallow sibling-index/count() where there’s no element context.
I also fixed the font-style edge case: the logic now handles both regular CSSPrimitiveValue and CSSFontStyleRangeValue (oblique ranges), scanning nested values to block invalid functions.
All relevant font properties (font-size, font-stretch, font-style, font-weight) are covered, with invalid values skipped entirely.
Correction: I just noticed my unit tests are failing. Need to revisit this issue. Please disregard my previous comment.
Marked as resolved.
Related details
- Munira Tursunova
- Rune Lillesveen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |