Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Description • 11 years ago

I'm running on Linux with layers acceleration enabled using an nvidia card. Recently I've started to get tons of crashes like this one:

https://crash-stats.mozilla.com/report/index/3f287251-dec2-4270-a054-482d22131008

Matt said he thinks it might be related to bug 740200.

This bug is very easy for me to reproduce (usually I get a crash within 30 seconds). I'm happy to look for information in gdb if you guys need it.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 11 years ago

Hop on IRC #gfx and we should be able to work through this.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 11 years ago

Actually, I get this myself on startup when I enable OMTC GL in my default profile.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 11 years ago

The bug is that we have a DrawTargetDual which reports its type as BACKEND_CAIRO since that's what its two delegate DrawTargets are. However, it always returns null from GetNativeSurface, so we crash trying to wrap null. It's simple enough to take the fallback path instead.

I'm dubious that DrawTargetDual should return BACKEND_CAIRO as its backend type. Seems to me it would make more sense to have its own backend type. But I'm not sure so I'm not doing that here. Alternatively, when it wraps two cairo surfaces, we could perhaps create a cairo_tee_surface for those two surfaces and return that from GetNativeSurface.

	Matt Woodrow (:mattwoodrow)
	Comment 4 • 11 years ago

Comment on attachment 817733 [details] [diff] [review]
wallpaper-ish fix

Review of attachment 817733 [details] [diff] [review]:
-----------------------------------------------------------------

Won't this just result in the themed object not being drawn at all? We just return from gfxXlibNativeRenderer::Draw without doing anything if GetThebesSurfaceForDrawTarget returns nullptr.

I'm ok with landing this so that people don't have to crash, but we really need to fix it properly.

It appears (from a quick check) that this thebes surface is only used for a few things: creating a similar temporary surface and copying the background up into the temporary.

The former of those two things wouldn't need (or want?) a tee surface, we could just use either of the two cairo surfaces. I don't see how the latter could work with a tee surface, so i guess we're not hitting that without moz2d somehow.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 11 years ago

Maybe the thing to do here is to rewrite gfxXlibNativeRenderer to use cairo directly instead of Thebes gfxASurfaces. Everywhere that we want to use gfxXlibNativeRenderer will have cairo.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 11 years ago

I am working on that approach.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 11 years ago

There's still some usage of gfxASurfaces in gfxXlibNativeRenderer with this patch: namely, gfxImageSurfaces, since I want to get this stuff landed to fix the crash bug. But it pushes quite far in the right direction.

	Karl Tomlinson (:karlt)
	Comment 15 • 11 years ago

Comment on attachment 819619 [details] [diff] [review]
Part 3: Make gfxXlibNativeRenderer::DrawWithXlib callback take a cairo_surface_t instead of a gfxXlibSurface

>+     * @param surfaceSize the surface has this size.
>+     * @param offset  draw at this offset into the given drawable
>      * @param clipRects an array of rectangles; clip to the union.
>      *                  Any rectangles provided will be contained by the
>      *                  rectangle with size provided to Draw and by the
>      *                  surface extents.
>      * @param numClipRects the number of rects in the array, or zero if
>      *                     no clipping is required.
>      */
>-    virtual nsresult DrawWithXlib(gfxXlibSurface* surface,
>+    virtual nsresult DrawWithXlib(cairo_surface_t* surface,
>                                   nsIntPoint offset,
>                                   nsIntRect* clipRects, uint32_t numClipRects) = 0;

No need to document |surfaceSize|, which doesn't exist.

	Karl Tomlinson (:karlt)
	Comment 16 • 11 years ago

Comment on attachment 819621 [details] [diff] [review]
Part 5: Add gfxXlibSurface::CreateCairo

Need to do something to free the pixmap when the surface is destroyed.

	Karl Tomlinson (:karlt)
	Comment 17 • 11 years ago

When I first read "CreateCairo", I thought it might create a cairo_t with an xlib surface as the target, so I suggest "CreateCairoSurface".

	Karl Tomlinson (:karlt)
	Comment 18 • 11 years ago

Comment on attachment 819624 [details] [diff] [review]
Part 6: Remove gfxXlibNativeRender::DrawOutput and convert tmpXlibSurface from gfxXlibSurface to cairo_surface_t

Can gfxColor.h nsAutoPtr.h gfxXlibSurface.h includes be removed from
gfxXlibNativeRenderer.h now?

>-        cairo_xlib_surface_get_screen (target) : screen;

>+        cairo_xlib_surface_get_screen (cairoTarget ) : screen;

Unnecessary extra space added.

>     // When the background has an alpha channel, we need to draw with an alpha
>     // channel anyway, so there is no need to copy the background.  If
>     // doCopyBackground is set here, we'll also need to check below that the
>     // background can copied without any loss in format conversions.
>     bool doCopyBackground = !drawIsOpaque && canDrawOverBackground &&
>-        target_content == CAIRO_CONTENT_COLOR;
>+        cairoTarget && cairo_surface_get_content (cairoTarget) == CAIRO_CONTENT_COLOR;

It would be better to set doCopyBackground if using the drawTarget.
We could even do that when the cairoTarget has alpha because the only
difference that might result would be using a visual more closing matching the
target (if there is more than one visual with alpha and
supportsAlternateVisual is set).

>-    if (ctx->IsCairo()) {

>-    } else {
>-        DrawTarget* drawTarget = ctx->GetDrawTarget();
>-        if (!drawTarget) {
>-            return;

>+    if (ctx->IsCairo()) {
>+      cairoTarget = cairo_get_group_target(ctx->GetCairo());
>+    } else {
>+      drawTarget = ctx->GetDrawTarget();
>+      cairoTarget = static_cast<cairo_surface_t*>
>+        (drawTarget->GetNativeSurface(NATIVE_SURFACE_CAIRO_SURFACE));
>+    }
>+
>+    cairo_surface_t* tempXlibSurface =
>+        CreateTempXlibSurface(cairoTarget, drawTarget, size,

Indentation confusion.

Do you know that GetDrawTarget() will return non-null here?

>+            NS_ASSERTION(cairoTarget, "eCopyBackground only used when there's a cairoTarget");
>+            cairo_set_operator(tmpCtx, CAIRO_OPERATOR_SOURCE);
>+            gfxPoint pt = -(offset + ctx->CurrentMatrix().GetTranslation());
>+            cairo_set_source_surface(tmpCtx, cairoTarget, pt.x, pt.y);

Oh.

I wonder whether it is worth including a comment here to indicate that the
extra slow path is taken unnecessarily when the DrawTarget is a
DrawTargetSkia, for example.

	Karl Tomlinson (:karlt)
	Comment 19 • 11 years ago

>+    SurfaceFormat moz2DFormat =
>+        cairo_surface_get_content(tempXlibSurface) == CAIRO_CONTENT_COLOR ?
>+            FORMAT_B8G8R8A8 : FORMAT_B8G8R8X8;

>+            RefPtr<SourceSurface> sourceSurface =
>+                Factory::CreateSourceSurfaceForCairoSurface(tempXlibSurface,
>+                                                            moz2DFormat);

What happens when moz2DFormat doesn't match the format of the cairo_surface_t?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 11 years ago

(In reply to Karl Tomlinson (:karlt) from comment #18)
> Comment on attachment 819624 [details] [diff] [review]
> >     // When the background has an alpha channel, we need to draw with an alpha
> >     // channel anyway, so there is no need to copy the background.  If
> >     // doCopyBackground is set here, we'll also need to check below that the
> >     // background can copied without any loss in format conversions.
> >     bool doCopyBackground = !drawIsOpaque && canDrawOverBackground &&
> >-        target_content == CAIRO_CONTENT_COLOR;
> >+        cairoTarget && cairo_surface_get_content (cairoTarget) == CAIRO_CONTENT_COLOR;
> 
> It would be better to set doCopyBackground if using the drawTarget.
> We could even do that when the cairoTarget has alpha because the only
> difference that might result would be using a visual more closing matching
> the
> target (if there is more than one visual with alpha and
> supportsAlternateVisual is set).

We could, but the case where the drawTarget wraps a single cairo surface (which should be the common case) is already handled because cairoTarget will be non-null in that case.

> Do you know that GetDrawTarget() will return non-null here?

Yes.

> >+            NS_ASSERTION(cairoTarget, "eCopyBackground only used when there's a cairoTarget");
> >+            cairo_set_operator(tmpCtx, CAIRO_OPERATOR_SOURCE);
> >+            gfxPoint pt = -(offset + ctx->CurrentMatrix().GetTranslation());
> >+            cairo_set_source_surface(tmpCtx, cairoTarget, pt.x, pt.y);
> 
> Oh.
> 
> I wonder whether it is worth including a comment here to indicate that the
> extra slow path is taken unnecessarily when the DrawTarget is a
> DrawTargetSkia, for example.

I'm a little confused. Why is this unnecessary for DrawTargetSkia?

(In reply to Karl Tomlinson (:karlt) from comment #19)
> >+    SurfaceFormat moz2DFormat =
> >+        cairo_surface_get_content(tempXlibSurface) == CAIRO_CONTENT_COLOR ?
> >+            FORMAT_B8G8R8A8 : FORMAT_B8G8R8X8;
> 
> >+            RefPtr<SourceSurface> sourceSurface =
> >+                Factory::CreateSourceSurfaceForCairoSurface(tempXlibSurface,
> >+                                                            moz2DFormat);
> 
> What happens when moz2DFormat doesn't match the format of the
> cairo_surface_t?

I don't think it matters all that much since we're just wrapping the cairo implementation here.

	Karl Tomlinson (:karlt)
	Comment 22 • 11 years ago

Comment on attachment 820246 [details] [diff] [review]
Part 5 v2

This looks correct, but I'm not sure the extra code is worthwhile.

CreateTempXlibSurface() could use the existing gfxXlibSurface Create() method and return the result of CairoSurface() on that.

	Karl Tomlinson (:karlt)
	Comment 23 • 11 years ago

Similarly, I'm not clear what moving from the C++ gfxXlibSurface objects to the C cairo_surface_t objects, with manual ref counting, really wins here.

Is there a reason why the cairo_surface_t from GetNativeSurface(NATIVE_SURFACE_CAIRO_SURFACE) can't be wrapped in a gfxXlibSurface()?

	Karl Tomlinson (:karlt)
	Comment 24 • 11 years ago

> > >+            NS_ASSERTION(cairoTarget, "eCopyBackground only used when there's a cairoTarget");
> > >+            cairo_set_operator(tmpCtx, CAIRO_OPERATOR_SOURCE);
> > >+            gfxPoint pt = -(offset + ctx->CurrentMatrix().GetTranslation());
> > >+            cairo_set_source_surface(tmpCtx, cairoTarget, pt.x, pt.y);
> > 
> > Oh.
> > 
> > I wonder whether it is worth including a comment here to indicate that the
> > extra slow path is taken unnecessarily when the DrawTarget is a
> > DrawTargetSkia, for example.
> 
> I'm a little confused. Why is this unnecessary for DrawTargetSkia?

Currently the patch only supports this path for DrawTargetCairo IIUC, and yes, that is all that is important now.

I assume Snapshot() could be used efficiently in copying the background if the native surface is a CPU memory surface in a DrawTargetSkia.  If we are planning to use Skia with CPU surfaces and compare perf one day, then a comment might help someone trying to improve perf with Skia.  If we are not, then don't worry.

Similarly, we may be able to do better with tee surfaces, but we'd need additional code to do some sort of optimization and I guess DrawTargetDual is the same kind of thing.  It's not important unless it is a common path.

> (In reply to Karl Tomlinson (:karlt) from comment #19)
> > >+    SurfaceFormat moz2DFormat =
> > >+        cairo_surface_get_content(tempXlibSurface) == CAIRO_CONTENT_COLOR ?
> > >+            FORMAT_B8G8R8A8 : FORMAT_B8G8R8X8;
> > 
> > >+            RefPtr<SourceSurface> sourceSurface =
> > >+                Factory::CreateSourceSurfaceForCairoSurface(tempXlibSurface,
> > >+                                                            moz2DFormat);
> > 
> > What happens when moz2DFormat doesn't match the format of the
> > cairo_surface_t?
> 
> I don't think it matters all that much since we're just wrapping the cairo
> implementation here.

Yes, I suspected it didn't matter, but maybe it's worth a comment explaining that it doesn't so the formats don't need to match exactly.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 25 • 11 years ago

(In reply to Karl Tomlinson (:karlt) from comment #23)
> Similarly, I'm not clear what moving from the C++ gfxXlibSurface objects to
> the C cairo_surface_t objects, with manual ref counting, really wins here.

The goal is to move away from gfxASurface stuff to use Moz2D only. It seemed simpler here to use cairo_surface_t than to convert the code to use Moz2D DrawTarget and SourceSurface.

(In reply to Karl Tomlinson (:karlt) from comment #24)
> I assume Snapshot() could be used efficiently in copying the background if
> the native surface is a CPU memory surface in a DrawTargetSkia.  If we are
> planning to use Skia with CPU surfaces and compare perf one day, then a
> comment might help someone trying to improve perf with Skia.  If we are not,
> then don't worry.

Yeah I don't think we'll be trying to mix Skia drawing with X drawing and expecting it to perform well.

> Yes, I suspected it didn't matter, but maybe it's worth a comment explaining
> that it doesn't so the formats don't need to match exactly.

OK. Thanks!

	Karl Tomlinson (:karlt)
	Comment 26 • 11 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #25)
> (In reply to Karl Tomlinson (:karlt) from comment #23)
> > Similarly, I'm not clear what moving from the C++ gfxXlibSurface objects to
> > the C cairo_surface_t objects, with manual ref counting, really wins here.
> 
> The goal is to move away from gfxASurface stuff to use Moz2D only. It seemed
> simpler here to use cairo_surface_t than to convert the code to use Moz2D
> DrawTarget and SourceSurface.

OK, thanks, and yes, I could imagine cairo_surface_t being simpler here than using Moz2D.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f168034813bb
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b489625999
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e1e608cfe3
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c3c13fc333c
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6057ba50267
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3b1b03a8155
https://hg.mozilla.org/integration/mozilla-inbound/rev/f31b00b83f66

	Carsten Book [:Tomcat]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/f168034813bb
https://hg.mozilla.org/mozilla-central/rev/87b489625999
https://hg.mozilla.org/mozilla-central/rev/28e1e608cfe3
https://hg.mozilla.org/mozilla-central/rev/4c3c13fc333c
https://hg.mozilla.org/mozilla-central/rev/e6057ba50267
https://hg.mozilla.org/mozilla-central/rev/f3b1b03a8155
https://hg.mozilla.org/mozilla-central/rev/f31b00b83f66

	Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Comment 29 • 11 years ago

Thanks Rob!

	u279076
	Comment 30 • 11 years ago

(In reply to Bill McCloskey (:billm) from comment #29)
> Thanks Rob!

Bill, I assume this is verified fixed based on your comment, can you please confirm?

	u279076
	Comment 31 • 11 years ago

Sorry, wrong status flag.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Reporter
	Comment 32 • 11 years ago

I think I did check that it worked. I stopped using hardware acceleration a while ago since it doesn't seem like we intend to support it in the future. So I'm not sure.

	u279076
	Comment 33 • 11 years ago

(In reply to Bill McCloskey (:billm) from comment #32)
> I think I did check that it worked. I stopped using hardware acceleration a
> while ago since it doesn't seem like we intend to support it in the future.
> So I'm not sure.

Okay, thanks Bill. I'm going to flag this to see if we can reproduce this and verify it's fixed on our own hardware. If not we'll have to assume this is fixed and mark it [qa-].

	Andrei Vaida [:avaida]
	Comment 34 • 11 years ago

I tried to replicate this issue based on the available data from socorro (Comment 0) and other comments or dependencies (Bug 740200) associated to it, with no luck. I've used Ubuntu 13.10 64-bit [1] with the latest Beta (BuildID: 20140303165517) and a GeForce 210 graphics card.

Marking this issue as verified. If anyone has any additional information in terms of reproducibility, please let me know and I will follow up on this. 


[1] Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0