New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bounds-check CoreGraphics paths because Facebook sucks #453

Closed
classilla opened this Issue Dec 4, 2017 · 21 comments

Comments

Projects
None yet
3 participants
@classilla
Owner

classilla commented Dec 4, 2017

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Dec 4, 2017

Owner

45.4 changepack: TenFourFox45E-changesets-20160908.zip
45.5b1 changepack: TenFourFox45E-changesets-20160923.zip

Owner

classilla commented Dec 4, 2017

45.4 changepack: TenFourFox45E-changesets-20160908.zip
45.5b1 changepack: TenFourFox45E-changesets-20160923.zip

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Dec 4, 2017

Owner

Changes between them (all other changesets match and are identical):

312309 -> 312353
312310 -> 312354
312311 -> 312355

new files in 45.5
312362
312363
312364
312365
312366

Owner

classilla commented Dec 4, 2017

Changes between them (all other changesets match and are identical):

312309 -> 312353
312310 -> 312354
312311 -> 312355

new files in 45.5
312362
312363
312364
312365
312366

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Dec 4, 2017

Owner

The three differences relate to differing landings for the MP3 fix in that version. Everything else is the same, so this is not the cause.

Owner

classilla commented Dec 4, 2017

The three differences relate to differing landings for the MP3 fix in that version. Everything else is the same, so this is not the cause.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Dec 4, 2017

Owner

312362: mostly MP3, but has a change to warmups and JS JIT inline depth
312365: IonPower-LE

Unfortunately this is most likely the latter.

definitely unrelated:
312363: pref pane
312364: VP9 IDCT
312366: VP9 convolver

Owner

classilla commented Dec 4, 2017

312362: mostly MP3, but has a change to warmups and JS JIT inline depth
312365: IonPower-LE

Unfortunately this is most likely the latter.

definitely unrelated:
312363: pref pane
312364: VP9 IDCT
312366: VP9 convolver

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Jan 3, 2018

Owner

Stack traces taken from failing processes at various times:

#0  0x9046f224 in aa_cubeto ()
#1  0x90435e5c in CGPathApply ()
#2  0x9480f428 in ripr_Path ()
#3  0x94811708 in ripc_DrawPath ()
#4  0x90453060 in CGContextDrawPath ()
#5  0x08521bfc in mozilla::gfx::DrawTargetCG::Fill (this=0x32407750, aPath=0x3243ac10, aPattern=@0xefff5d68, aDrawOptions=<value temporarily unavailable, due to optimizations>) at DrawTargetCG.cpp:1720
#6  0x092e9e94 in mozilla::dom::CanvasRenderingContext2D::Fill (this=0x3d769e00, winding=<value temporarily unavailable, due to optimizations>) at CanvasRenderingContext2D.cpp:2720
#7  0x08e4403c in fill (cx=0x3a2b0a40, obj=<value temporarily unavailable, due to optimizations>, self=0x3d769e00, args=@0xefff5ec0) at CanvasRenderingContext2DBinding.cpp:3361
#8  0x0929e32c in mozilla::dom::GenericBindingMethod (cx=0x3a2b0a40, argc=<value temporarily unavailable, due to optimizations>, vp=<value temporarily unavailable, due to optimizations>) at BindingUtils.cpp:2745
#9  0x43d2dde0 in ?? ()
#10 0x0ad50244 in EnterBaseline (cx=0x3a2b0a40, data=@0xefff68c0) at BaselineJIT.cpp:146
#11 0x0ad5c31c in js::jit::EnterBaselineMethod (cx=0x3a2b0a40, state=@0xefff69c4) at BaselineJIT.cpp:184
#0  0x9046f290 in aa_cubeto ()
#1  0x90435e5c in CGPathApply ()
#2  0x9480f428 in ripr_Path ()
#3  0x94811708 in ripc_DrawPath ()
#4  0x90453060 in CGContextDrawPath ()
#5  0x08521bfc in mozilla::gfx::DrawTargetCG::Fill (this=0x32407750, aPath=0x3243ac10, aPattern=@0xefff5d68, aDrawOptions=<value temporarily unavailable, due to optimizations>) at DrawTargetCG.cpp:1720
#6  0x092e9e94 in mozilla::dom::CanvasRenderingContext2D::Fill (this=0x3d769e00, winding=<value temporarily unavailable, due to optimizations>) at CanvasRenderingContext2D.cpp:2720

No change by disabling antialiasing or other options. The thought is that somehow Facebook violated an internal constraint on a native-endian float array and then used it as int to construct a bad path which causes CoreGraphics to bug out.

Switching canvas to Cairo doesn't crash, though the images still don't display.

Owner

classilla commented Jan 3, 2018

Stack traces taken from failing processes at various times:

#0  0x9046f224 in aa_cubeto ()
#1  0x90435e5c in CGPathApply ()
#2  0x9480f428 in ripr_Path ()
#3  0x94811708 in ripc_DrawPath ()
#4  0x90453060 in CGContextDrawPath ()
#5  0x08521bfc in mozilla::gfx::DrawTargetCG::Fill (this=0x32407750, aPath=0x3243ac10, aPattern=@0xefff5d68, aDrawOptions=<value temporarily unavailable, due to optimizations>) at DrawTargetCG.cpp:1720
#6  0x092e9e94 in mozilla::dom::CanvasRenderingContext2D::Fill (this=0x3d769e00, winding=<value temporarily unavailable, due to optimizations>) at CanvasRenderingContext2D.cpp:2720
#7  0x08e4403c in fill (cx=0x3a2b0a40, obj=<value temporarily unavailable, due to optimizations>, self=0x3d769e00, args=@0xefff5ec0) at CanvasRenderingContext2DBinding.cpp:3361
#8  0x0929e32c in mozilla::dom::GenericBindingMethod (cx=0x3a2b0a40, argc=<value temporarily unavailable, due to optimizations>, vp=<value temporarily unavailable, due to optimizations>) at BindingUtils.cpp:2745
#9  0x43d2dde0 in ?? ()
#10 0x0ad50244 in EnterBaseline (cx=0x3a2b0a40, data=@0xefff68c0) at BaselineJIT.cpp:146
#11 0x0ad5c31c in js::jit::EnterBaselineMethod (cx=0x3a2b0a40, state=@0xefff69c4) at BaselineJIT.cpp:184
#0  0x9046f290 in aa_cubeto ()
#1  0x90435e5c in CGPathApply ()
#2  0x9480f428 in ripr_Path ()
#3  0x94811708 in ripc_DrawPath ()
#4  0x90453060 in CGContextDrawPath ()
#5  0x08521bfc in mozilla::gfx::DrawTargetCG::Fill (this=0x32407750, aPath=0x3243ac10, aPattern=@0xefff5d68, aDrawOptions=<value temporarily unavailable, due to optimizations>) at DrawTargetCG.cpp:1720
#6  0x092e9e94 in mozilla::dom::CanvasRenderingContext2D::Fill (this=0x3d769e00, winding=<value temporarily unavailable, due to optimizations>) at CanvasRenderingContext2D.cpp:2720

No change by disabling antialiasing or other options. The thought is that somehow Facebook violated an internal constraint on a native-endian float array and then used it as int to construct a bad path which causes CoreGraphics to bug out.

Switching canvas to Cairo doesn't crash, though the images still don't display.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Jan 3, 2018

Owner

Backing out the compiler changes in 312362 didn't change anything, and the 2D code in DrawTargetCG didn't change, so IonPower-LE is now very likely the underlying problem.

Owner

classilla commented Jan 3, 2018

Backing out the compiler changes in 312362 didn't change anything, and the 2D code in DrawTargetCG didn't change, so IonPower-LE is now very likely the underlying problem.

@internetzel

This comment has been minimized.

Show comment
Hide comment
@internetzel

internetzel Jan 5, 2018

This does affect Leopard WebKit as well - so I'm quite sure it's not related to IonPower-LE.
In fact I already found a way to prevent WebKit from hanging - and that will most probably be the way how to solve this for TFF as well:
you shouldn't add paths to a CGContext (using CGContextAddPath) if their height and/or width (determined by CGPathGetBoundingBox) exceed INT_MAX.
When I first added that check the icons were drawing and working again, but now they don't draw at all - but at least the browser doesn't hang.

internetzel commented Jan 5, 2018

This does affect Leopard WebKit as well - so I'm quite sure it's not related to IonPower-LE.
In fact I already found a way to prevent WebKit from hanging - and that will most probably be the way how to solve this for TFF as well:
you shouldn't add paths to a CGContext (using CGContextAddPath) if their height and/or width (determined by CGPathGetBoundingBox) exceed INT_MAX.
When I first added that check the icons were drawing and working again, but now they don't draw at all - but at least the browser doesn't hang.

@internetzel

This comment has been minimized.

Show comment
Hide comment
@internetzel

internetzel Jan 5, 2018

I wonder if the cause for those high values passed to CoreGraphics has to do with little endian typed arrays...

internetzel commented Jan 5, 2018

I wonder if the cause for those high values passed to CoreGraphics has to do with little endian typed arrays...

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Jan 6, 2018

Owner

I guess that would be a way to prevent a malicious (or in this case batty) script from hanging up the browser, though it's annoying to have to check that every time.

My theory is since TenFourFox doesn't byteswap floats and doubles for performance reasons, Facebook may have stored something there and then accessed the same tract of memory as an int, which is byteswapped (yielding an endian failure). But I thought LWK does byteswap floats and doubles as well?

Owner

classilla commented Jan 6, 2018

I guess that would be a way to prevent a malicious (or in this case batty) script from hanging up the browser, though it's annoying to have to check that every time.

My theory is since TenFourFox doesn't byteswap floats and doubles for performance reasons, Facebook may have stored something there and then accessed the same tract of memory as an int, which is byteswapped (yielding an endian failure). But I thought LWK does byteswap floats and doubles as well?

@internetzel

This comment has been minimized.

Show comment
Hide comment
@internetzel

internetzel Jan 6, 2018

Leopard WebKit doesn't byteswap floating point values anymore since I didn't see any sense in doing so.
Afer all, storing floating point values in memory and later interpreting that same memory as integer values should never result in anything reasonable, except for a value of 0. Or does it?

internetzel commented Jan 6, 2018

Leopard WebKit doesn't byteswap floating point values anymore since I didn't see any sense in doing so.
Afer all, storing floating point values in memory and later interpreting that same memory as integer values should never result in anything reasonable, except for a value of 0. Or does it?

@internetzel

This comment has been minimized.

Show comment
Hide comment
@internetzel

internetzel Jan 6, 2018

By the way, CGPathGetBoundingBox is quite fast in execution, since it simply walks the path elements considering any points, including virtual control points, for the total size.
There also is the hidden CGPathGetGeometricBoundingBox, which only includes the points that actually are on the path. In later versions of OS X that was made public as CGPathGetPathBoundingBox. Actually I don't know whether the geometric or virtual bounds are the problem but I opted to the virtual bounds because of speed.
In fact, since CoreGraphics doesn't protect itself, there's no alternative to doing it on our own. You might choose to validate the values earlier, upon path element construction, but it might become difficult to evaluate the total path dimensions at that time.
I came to my approach by assuming that at some point CoreGraphics would need to convert to integer dimensions in number of pixels - so it seemed reasonable to check the float values for convertibility to integer.

internetzel commented Jan 6, 2018

By the way, CGPathGetBoundingBox is quite fast in execution, since it simply walks the path elements considering any points, including virtual control points, for the total size.
There also is the hidden CGPathGetGeometricBoundingBox, which only includes the points that actually are on the path. In later versions of OS X that was made public as CGPathGetPathBoundingBox. Actually I don't know whether the geometric or virtual bounds are the problem but I opted to the virtual bounds because of speed.
In fact, since CoreGraphics doesn't protect itself, there's no alternative to doing it on our own. You might choose to validate the values earlier, upon path element construction, but it might become difficult to evaluate the total path dimensions at that time.
I came to my approach by assuming that at some point CoreGraphics would need to convert to integer dimensions in number of pixels - so it seemed reasonable to check the float values for convertibility to integer.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Jan 6, 2018

Owner

It depends, it might give NaN or some absurd value. I imagine it would only take one redonkulous coordinate to cause this hang.

I think you're right, since even an innocent error could hang up the browser, and the hang does not occur when Cairo is the canvas backend, so this is just something that will have to be done. I'll put it in the next beta just in case this regresses something unexpected. The workaround will be for affected users to turn on Cairo canvas.

Owner

classilla commented Jan 6, 2018

It depends, it might give NaN or some absurd value. I imagine it would only take one redonkulous coordinate to cause this hang.

I think you're right, since even an innocent error could hang up the browser, and the hang does not occur when Cairo is the canvas backend, so this is just something that will have to be done. I'll put it in the next beta just in case this regresses something unexpected. The workaround will be for affected users to turn on Cairo canvas.

@classilla classilla changed the title from Issue with Facebook likes between 45.4 and 45.5 to Bounds-check CoreGraphics paths because Facebook sucks Jan 6, 2018

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Jan 25, 2018

Owner

The hang no longer occurs.

Owner

classilla commented Jan 25, 2018

The hang no longer occurs.

@classilla classilla closed this Jan 25, 2018

@chris-chtrusch

This comment has been minimized.

Show comment
Hide comment
@chris-chtrusch

chris-chtrusch Feb 9, 2018

Collaborator

I don't know if this should go in a new bug or if it's covered by the "Like" button fix in FPR6.

The problem is on the Activity Log page, see screenshot at https://tinyurl.com/y78vt3nz

Clicking the blue "Review x item(s)" button or simply hovering over it will lock up the browser reliably even if canvas.azure backends is set to Cairo. There is no animation coming up with this button except it gets a little darker when you hover over it or click it. Hovering or clicking any of the items in the Filters menu to the left also locks up the browser in the same way.

Tested fresh profile, disabled ion, baseline, set content.azure backends to Cairo as well, etc., no relation.

The regression window, again, is 45.4.0 --- 45.5.0b1. So this looks like the same problem, but it's not fixed by switching to Canvas.

Collaborator

chris-chtrusch commented Feb 9, 2018

I don't know if this should go in a new bug or if it's covered by the "Like" button fix in FPR6.

The problem is on the Activity Log page, see screenshot at https://tinyurl.com/y78vt3nz

Clicking the blue "Review x item(s)" button or simply hovering over it will lock up the browser reliably even if canvas.azure backends is set to Cairo. There is no animation coming up with this button except it gets a little darker when you hover over it or click it. Hovering or clicking any of the items in the Filters menu to the left also locks up the browser in the same way.

Tested fresh profile, disabled ion, baseline, set content.azure backends to Cairo as well, etc., no relation.

The regression window, again, is 45.4.0 --- 45.5.0b1. So this looks like the same problem, but it's not fixed by switching to Canvas.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Feb 9, 2018

Owner

I'm not sure how to test it exactly as you did since I don't get a "Review # Item(s)" button, but I can click the items in the Filters menu in FPR6 and they don't hang either. So I'm going to assume the same fix covers it. I'll reopen if it does not, though I'll need some additional STRs in that case.

Owner

classilla commented Feb 9, 2018

I'm not sure how to test it exactly as you did since I don't get a "Review # Item(s)" button, but I can click the items in the Filters menu in FPR6 and they don't hang either. So I'm going to assume the same fix covers it. I'll reopen if it does not, though I'll need some additional STRs in that case.

@chris-chtrusch

This comment has been minimized.

Show comment
Hide comment
@chris-chtrusch

chris-chtrusch Feb 10, 2018

Collaborator

Tenny Fox now has something in their account that needs to be reviewed.

Problem is that this account doesn't have the bug on the Activity page (yet). I've seen this in my private account only so far. (They don't roll out changes to all accounts at once.) I'll test it as soon as FPR6 is available.

Collaborator

chris-chtrusch commented Feb 10, 2018

Tenny Fox now has something in their account that needs to be reviewed.

Problem is that this account doesn't have the bug on the Activity page (yet). I've seen this in my private account only so far. (They don't roll out changes to all accounts at once.) I'll test it as soon as FPR6 is available.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Feb 10, 2018

Owner
Owner

classilla commented Feb 10, 2018

@chris-chtrusch

This comment has been minimized.

Show comment
Hide comment
@chris-chtrusch

chris-chtrusch Feb 22, 2018

Collaborator

Both "like button" and "activity log" are fixed for my FB profile in FPR6b1.

Collaborator

chris-chtrusch commented Feb 22, 2018

Both "like button" and "activity log" are fixed for my FB profile in FPR6b1.

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Feb 22, 2018

Owner
Owner

classilla commented Feb 22, 2018

@chris-chtrusch

This comment has been minimized.

Show comment
Hide comment
@chris-chtrusch

chris-chtrusch Feb 22, 2018

Collaborator

Of course I turned Cairo off :)

Collaborator

chris-chtrusch commented Feb 22, 2018

Of course I turned Cairo off :)

@classilla

This comment has been minimized.

Show comment
Hide comment
@classilla

classilla Feb 22, 2018

Owner
Owner

classilla commented Feb 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment