This was found while fuzzing an inbound ASan build on Linux ==3072==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000070 (pc 0x7f9d23f7c67a bp 0x7ffd200f8f50 sp 0x7ffd200f8aa0 T0) #0 0x7f9d23f7c679 in get /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:261:27 #1 0x7f9d23f7c679 in operator mozilla::gfx::DrawTarget * /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:277 #2 0x7f9d23f7c679 in GetDrawTarget /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/gfxContext.h:81 #3 0x7f9d23f7c679 in GetDrawTarget /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsRenderingContext.h:35 #4 0x7f9d23f7c679 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsLineLayout.cpp:947 #5 0x7f9d23fe9475 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:4088:3 #6 0x7f9d23fe7d7d in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3890:5 #7 0x7f9d23fdf4ed in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3756:9 #8 0x7f9d23fcf5c4 in ReflowLine /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2754:5 ...

Astley, could you find someone to check this? It seems we got a invalid DrawTarget from layout. 1467208453680 addons.xpi-utils WARN Disabling foreign installed add-on ubufox@ubuntu.com in app-system-share Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.06916) [GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.06916) |[1][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.0748) [GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.06916) |[1][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.0748) |[2][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.07623) [GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.06916) |[1][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.0748) |[2][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.07623) |[3][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.07805) [GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 ASAN:DEADLYSIGNAL ================================================================= ==3072==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000070 (pc 0x7f9d23f7c67a bp 0x7ffd200f8f50 sp 0x7ffd200f8aa0 T0) #0 0x7f9d23f7c679 in get /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:261:27 #1 0x7f9d23f7c679 in operator mozilla::gfx::DrawTarget * /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:277 #2 0x7f9d23f7c679 in GetDrawTarget /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/gfxContext.h:81 #3 0x7f9d23f7c679 in GetDrawTarget /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsRenderingContext.h:35 #4 0x7f9d23f7c679 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsLineLayout.cpp:947 #5 0x7f9d23fe9475 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:4088:3 #6 0x7f9d23fe7d7d in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3890:5 #7 0x7f9d23fdf4ed in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:3756:9 #8 0x7f9d23fcf5c4 in ReflowLine /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2754:5 #9 0x7f9d23fcf5c4 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:2290 #10 0x7f9d23fc8816 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockFrame.cpp:1171:3 #11 0x7f9d23fe4e1c in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/generic/nsBlockReflowContext.cpp:306:3

The problem is presumably related to the call to CreateReferenceRenderingContext in this frame of the stack: #24 0x7f9d23f06fc9 in PresShell::DoReflow(nsIFrame*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/layout/base/nsPresShell.cpp:9602:3 These errors in the log: Crash Annotation GraphicsCriticalError: |[0][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.06916) |[1][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.0748) |[2][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.07623) |[3][GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 (t=9.07805) [GFX1-]: Invalid target in gfxContext::CreateOrNull 0x60e0000f4cc0 seem relevant. Should DoReflow perform some test on the result of CreateReferenceRenderingContext to check that it's safe to use before proceeding to use it?

Looks like the DrawTarget we got from ScreenReferenceDrawTarget() is nullptr. Wondering if the call CreateOffscreenContentDrawTarget() to create DT was failed and caused following crash. gPlatform->mScreenReferenceDrawTarget = gPlatform->CreateOffscreenContentDrawTarget(IntSize(1, 1), SurfaceFormat::B8G8R8A8);

Yes, CreateReferenceRenderingContext() can return nullptr, and the callers should deal with it if they can, or, I guess, MOZ_CRASH if they believe this is an unrecoverable situation. There is no ill effects of calling CreateReferenceRenderingContext() with parameters that cause it to return nullptr, so it's easiest to just deal with the nullptr result, rather than attempt to avoid making the call if we think the current state will cause it to fail.

By the way the '-' in the GFX1- means that, while we want these notes in the crash report, as they may give us a hint as to where to look, it is something that can happen, so we should deal with it, in general, as much as possible. The GFX1+, on the other hand, would be "this is an assert, we definitely have a bug in our code".

(In reply to Milan Sreckovic [:milan] from comment #5) > Yes, CreateReferenceRenderingContext() can return nullptr, > and the callers should deal with it if they can I disagree that's the way we should handle this in general. If CreateReferenceRenderingContext() returns null that's usually because someone did something bad with ScreenReferenceDrawTarget() that it shouldn't have done. It really should be in a valid state at all times.

The reason we get null from CreateReferenceRenderingContext in this case is this: #0 _moz_cairo_get_group_target(cr=0x7f491a34a140) gfx/cairo/cairo/src/cairo.c:4057 #1 mozilla::gfx::DrawTargetCairo::IsValid() gfx/2d/DrawTargetCairo.cpp:621 #2 gfxContext::CreateOrNull() gfx/thebes/gfxContext.cpp:90 #3 PresShell::CreateReferenceRenderingContext() layout/base/nsPresShell.cpp:2959 #4 PresShell::DoReflow() layout/base/nsPresShell.cpp:9537 #5 PresShell::ProcessReflowCommands() layout/base/nsPresShell.cpp:9782 #6 PresShell::FlushPendingNotifications() layout/base/nsPresShell.cpp:4154 #7 nsRefreshDriver::Tick() layout/base/nsRefreshDriver.cpp:1797 (rr) list 4052 * Since: 1.2 4053 **/ 4054 cairo_surface_t * 4055 cairo_get_group_target (cairo_t *cr) 4056 { 4057 if (unlikely (cr->status)) 4058 return _cairo_surface_create_in_error (cr->status); 4059 4060 return _cairo_gstate_get_target (cr->gstate); 4061 } (rr) p cr->status $29 = CAIRO_STATUS_INVALID_MATRIX (rr) watch -l cr->status (rr) reverse-continue Hardware watchpoint 1: -location cr->status Old value = CAIRO_STATUS_INVALID_MATRIX New value = CAIRO_STATUS_SUCCESS (rr) bt #0 _cairo_atomic_int_cmpxchg_impl() gfx/cairo/cairo/src/cairo-atomic-private.h:120 #1 _cairo_set_error(cr=0x7f491a34a140, status=CAIRO_STATUS_INVALID_MATRIX) gfx/cairo/cairo/src/cairo.c:208 #2 INT__moz_cairo_set_matrix() gfx/cairo/cairo/src/cairo.c:1560 #3 mozilla::gfx::DrawTargetCairo::SetTransform() gfx/2d/DrawTargetCairo.cpp:2190 #4 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText() dom/canvas/CanvasRenderingContext2D.cpp:3952 #5 mozilla::dom::CanvasRenderingContext2D::MeasureText() dom/canvas/CanvasRenderingContext2D.cpp:3410 #6 mozilla::dom::CanvasRenderingContext2DBinding::measureText() dom/bindings/CanvasRenderingContext2DBinding.cpp:4094 #7 mozilla::dom::GenericBindingMethod() dom/bindings/BindingUtils.cpp:2784 #8 js::CallJSNative() js/src/jscntxtinlines.h:232 ... i.e. we're setting a bad matrix on it here: http://searchfox.org/mozilla-central/rev/261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/dom/canvas/CanvasRenderingContext2D.cpp#3933-3934,3940 which leaves it in a permanently damaged state. So I believe the fix here should look something like this.

But we should know what the rules are for handling a DrawTarget being in an invalid state. There should be a point where we test it and either MOZ_CRASH, or bail in some other way, rather than just trying to do layout operations with it.

Inside cairo, it sets status as CAIRO_STATUS_INVALID_MATRIX[1] if the matrix is not invertible. Based on the input of test case, it is expected behavior in cairo. In my personal opinion, we should handle the null from CreateReferenceRenderingContext(). Another thing we could add here is to dump cairo status when DrawTargetCairo::IsValid()[2] return false. [1]https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo-gstate.c#738 [2]https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#620

I don't think CreateReferenceRenderingContext is returning null; I think it's returning a non-null rendering context whose DrawTargit is in an invalid state.

(In reply to Mats Palmgren (:mats) from comment #8) ... > > (rr) bt > #0 _cairo_atomic_int_cmpxchg_impl() > gfx/cairo/cairo/src/cairo-atomic-private.h:120 > #1 _cairo_set_error(cr=0x7f491a34a140, status=CAIRO_STATUS_INVALID_MATRIX) > gfx/cairo/cairo/src/cairo.c:208 > #2 INT__moz_cairo_set_matrix() gfx/cairo/cairo/src/cairo.c:1560 > #3 mozilla::gfx::DrawTargetCairo::SetTransform() > gfx/2d/DrawTargetCairo.cpp:2190 > #4 mozilla::dom::CanvasRenderingContext2D::DrawOrMeasureText() > dom/canvas/CanvasRenderingContext2D.cpp:3952 > #5 mozilla::dom::CanvasRenderingContext2D::MeasureText() > dom/canvas/CanvasRenderingContext2D.cpp:3410 > #6 mozilla::dom::CanvasRenderingContext2DBinding::measureText() > dom/bindings/CanvasRenderingContext2DBinding.cpp:4094 > #7 mozilla::dom::GenericBindingMethod() dom/bindings/BindingUtils.cpp:2784 > #8 js::CallJSNative() js/src/jscntxtinlines.h:232 > ... > > i.e. we're setting a bad matrix on it here: > http://searchfox.org/mozilla-central/rev/ > 261fe13dcd88cfd2e99e65755e7ca4b7a2e583df/dom/canvas/CanvasRenderingContext2D. > cpp#3933-3934,3940 > which leaves it in a permanently damaged state. > > So I believe the fix here should look something like this. That's interesting. Either something gets messed up in the conversion, or the matrix going in is non-finite. Mats, any chance of getting the values of the transform that causes the problem? Any NaN/Inf in there?

As in, I'm hoping this would fix it in DrawTargetCairo.cpp: - mTransformSingular = aTransform.IsSingular(); + mTransformSingular = aTransform.IsSingular() || !aTransform.IsFinite();

vincent, please help to check the matrix value based on the test case.

I tried to set the break point and the back trace would be the following. Breakpoint 1, _cairo_error (status=CAIRO_STATUS_INVALID_MATRIX) at /home/vliu-pc/proj/gecko-dev/gfx/cairo/cairo/src/cairo.c:184 184 return status; (gdb) p status $1 = CAIRO_STATUS_INVALID_MATRIX (gdb) bt #0 _cairo_error (status=CAIRO_STATUS_INVALID_MATRIX) at /home/vliu-pc/proj/gecko-dev/gfx/cairo/cairo/src/cairo.c:184 #1 0x00007fffea4c7d20 in _cairo_gstate_set_matrix (gstate=0x7fffcc3ae9c8, matrix=0x7fffffffa860) at /home/vliu-pc/proj/gecko-dev/gfx/cairo/cairo/src/cairo-gstate.c:739 #2 0x00007fffea513d40 in INT__moz_cairo_set_matrix (cr=0x7fffcc3ae800, matrix=0x7fffffffa860) at /home/vliu-pc/proj/gecko-dev/gfx/cairo/cairo/src/cairo.c:1558 #3 0x00007fffe6a53cf3 in mozilla::gfx::DrawTargetCairo::SetTransform (this=0x7fffccbc15c0, aTransform=...) at /home/vliu-pc/proj/gecko-dev/gfx/2d/DrawTargetCairo.cpp:2207 #4 0x00007fffe80aae9a in mozilla::dom::CanvasRenderingContext2D::Scale (this=0x7fffcc314800, aX=341.64060715900001, aY=753.36256717399999, aError=...) at /home/vliu-pc/proj/gecko-dev/dom/canvas/CanvasRenderingContext2D.cpp:1821 #5 0x00007fffe7938330 in mozilla::dom::CanvasRenderingContext2DBinding::scale (cx=0x7ffff6b11000, obj=..., self=0x7fffcc314800, args=...) at /home/vliu-pc/proj/gecko-dev/obj-x86_64-pc-linux-gnu/dom/bindings/CanvasRenderingContext2DBinding.cpp:1992 #6 0x00007fffe8032c40 in mozilla::dom::GenericBindingMethod (cx=0x7ffff6b11000, argc=2, vp=0x7fffd8c46090) at /home/vliu-pc/pro Apparently status got CAIRO_STATUS_INVALID_MATRIX when the following Scale () was operated. c.scale(341.640607159,753.362567174);

(In reply to Milan Sreckovic [:milan] from comment #13) > As in, I'm hoping this would fix it in DrawTargetCairo.cpp: > > - mTransformSingular = aTransform.IsSingular(); > + mTransformSingular = aTransform.IsSingular() || !aTransform.IsFinite(); 2203 mTransformSingular = aTransform.IsSingular() || !aTransform.IsFinite(); (gdb) p aTransform $1 = (const mozilla::gfx::Matrix &) @0x7fffffffa8c0: {{{_11 = 1.51653638e+19, _12 = 1.51653638e+19, _21 = 3.34416264e+19, _22 = 3.34416264e+19, _31 = 4.43898184e+16, _32 = 4.43898184e+16}, components = { 1.51653638e+19, 1.51653638e+19, 3.34416264e+19, 3.34416264e+19, 4.43898184e+16, 4.43898184e+16}}} From dumping aTransform, I through Milan's suggestion should fix this problem since the elements in matrix were quite large. But it is not. I will still look into more to figure it out.

Those certainly are large numbers, and we may be hitting some kind of underflow once we invert. It's probably a good idea to add the !IsFinite() anyway, but we'd need more. Should be able to trace through the Cairo code that determines if the matrix is invertible and find out what it is that code doesn't like.

Hi Milan, Could you please have a review for me? Really thanks.

Comment on attachment 8768623 [details] [diff] [review] Add IsFinite() and IsInvertible() check before set matrix in Cairo. Review of attachment 8768623 [details] [diff] [review]: ----------------------------------------------------------------- I'd still want to double check with Bas if he knows a reason we should allow infinite, non-invertible matrices to return false in IsSingular. ::: gfx/2d/DrawTargetCairo.cpp @@ +2182,5 @@ > DrawTargetCairo::SetTransform(const Matrix& aTransform) > { > DrawTarget::SetTransform(aTransform); > > + mTransformSingular = aTransform.IsSingular() || !aTransform.IsFinite() || !aTransform.IsInvertible(); If IsSingular() is defined to test for the "finite" condition, we shouldn't need this change. ::: gfx/2d/Matrix.h @@ +348,5 @@ > { > return Determinant() == 0; > } > > + bool IsInvertible() const I think I'd rather see this: bool IsSingular() const { Float det = Determinant(); return !mozilla::IsFinite() || det == 0; } since we seem to use "IsSingular" to mean the same as "is not invertible". Then we don't need the change to DrawTargetCairo.cpp at all, and the other places benefit from this check.

(In reply to Milan Sreckovic [:milan] from comment #19) > Comment on attachment 8768623 [details] [diff] [review] > Add IsFinite() and IsInvertible() check before set matrix in Cairo. > > Review of attachment 8768623 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'd still want to double check with Bas if he knows a reason we should allow > infinite, non-invertible matrices to return false in IsSingular. Double checked. Yes, we'd want to update IsSingular to also check for finite. In other words, we want IsSingular to return false only when 1./Determinant() is a well defined number, so checking for zero and IsFinite() should do it.

Hi, Milan, Could you please have a review for the updated patch? Thanks

Obsoletes the previous patch.

Hi Milan, Could you please have review? Thanks.

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=527fb1a470c8

Pushed by vliu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4be1c47c9cbc Add matrix checking before set matrix in Cairo. r=milan

Is there a reason the testcase wasn't landed as a crashtest?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #27) > Is there a reason the testcase wasn't landed as a crashtest? I will add crashtest ASAP.

Hi Milan, I'd missing crashtest for this bug. Could you please have a review? Thanks.

Did you test that, without the original patch but with the crashtest, the crashtest crashes when running in the crashtest harness ("./mach crashtest ...")?

(In reply to David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) from comment #30) > Did you test that, without the original patch but with the crashtest, the > crashtest crashes when running in the crashtest harness ("./mach crashtest > ...")? I should have run this kind of crashtest before sending review. Thanks for your comment.

Comment on attachment 8776440 [details] Add crashtest. Hi Milan, As suggestion from :dbaron, I would run crashtest before sending review.

Reopen it to land crashtest.

Hi Milan, Add patch crashtest for this bug and also run the try for crash on different platform in [1]. Could you please have a review? Thanks. [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a5a4c9db5d3&selectedJob=24997522

Comment on attachment 8778097 [details] [diff] [review] Add crashtest. Review of attachment 8778097 [details] [diff] [review]: ----------------------------------------------------------------- The test looks good - I can't run it right now, but I'm assuming it actually crashes without the fix.

Is this ready to land, or blocked on something?

Pushed by vliu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea917aa02159 Add crashtest. r=milan

(In reply to Milan Sreckovic [:milan] from comment #36) > Is this ready to land, or blocked on something? Landed. and try result was in [1] [1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39962e3fc549&selectedJob=25691382