Closed Bug 1283113 Opened 4 years ago Closed 4 years ago

SEGV around null [@nsLineLayout::ReflowFrame] because !DrawTargetCairo::IsValid() because canvas text measurement causes CAIRO_STATUS_INVALID_MATRIX state

Categories

(Core :: Canvas: 2D, defect, critical)

50 Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: tsmith, Assigned: vliu)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Attachments

(5 files, 4 obsolete files)

Attached file test_case.html
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
...
Attached file crash_log.txt
Flags: needinfo?(howareyou322)
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
Component: Canvas: 2D → Layout
Flags: needinfo?(howareyou322) → needinfo?(aschen)
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?
Flags: needinfo?(milan)
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);
Flags: needinfo?(aschen)
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.
Flags: needinfo?(milan)
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.
Attached patch fix?Splinter Review
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.
Component: Layout → Canvas: 2D
Summary: SEGV around null [@nsLineLayout::ReflowFrame] → SEGV around null [@nsLineLayout::ReflowFrame] because !DrawTargetCairo::IsValid() because canvas text measurement causes CAIRO_STATUS_INVALID_MATRIX state
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.
Flags: needinfo?(vliu)
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);
Flags: needinfo?(vliu)
(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.
Assignee: nobody → vliu
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.
Attachment #8768623 - Flags: review?(milan)
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.
Attachment #8768623 - Flags: review?(milan)
(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
Attachment #8770032 - Flags: review?(milan)
Obsoletes the previous patch.
Attachment #8768623 - Attachment is obsolete: true
Hi Milan,

Could you please have review? Thanks.
Attachment #8770053 - Flags: review?(milan)
Attachment #8770032 - Attachment is obsolete: true
Attachment #8770036 - Attachment is obsolete: true
Attachment #8770032 - Flags: review?(milan)
Attachment #8770053 - Flags: review?(milan) → review+
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be1c47c9cbc
Add matrix checking before set matrix in Cairo. r=milan
https://hg.mozilla.org/mozilla-central/rev/4be1c47c9cbc
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: grizzly
Is there a reason the testcase wasn't landed as a crashtest?
Flags: needinfo?(vliu)
Flags: in-testsuite?
(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.
Flags: needinfo?(vliu)
Attached file Add crashtest. (obsolete) —
Hi Milan,

I'd missing crashtest for this bug. Could you please have a review? Thanks.
Attachment #8776440 - Flags: review?(milan)
Did you test that, without the original patch but with the crashtest, the crashtest crashes when running in the crashtest harness ("./mach crashtest ...")?
Flags: needinfo?(vliu)
(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.
Flags: needinfo?(vliu)
Comment on attachment 8776440 [details]
Add crashtest.

Hi Milan,

As suggestion from :dbaron, I would run crashtest before sending review.
Attachment #8776440 - Attachment is obsolete: true
Attachment #8776440 - Attachment is patch: false
Attachment #8776440 - Flags: review?(milan)
Reopen it to land crashtest.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Add crashtest.Splinter Review
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
Attachment #8778097 - Flags: review?(milan)
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.
Attachment #8778097 - Flags: review?(milan) → review+
Is this ready to land, or blocked on something?
Flags: needinfo?(vliu)
(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
Flags: needinfo?(vliu)
https://hg.mozilla.org/mozilla-central/rev/ea917aa02159
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: mozilla50 → mozilla51
You need to log in before you can comment on or make changes to this bug.