914426 - Dumping layer trees in B2G crashes

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 2

[Nick Cameron [:nrc]]

Comment on attachment 801923 [details] [diff] [review]
fix

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

::: gfx/layers/composite/ContentHost.cpp
@@ +234,5 @@
>    }
>    if (aDumpHtml) {
>      fprintf(aFile, "<ul>");
>    }
> +  if (mDeprecatedTextureHost && aDumpHtml) {

I don't see why you are adding these checks for aDumpHtml. They are covered by the ternary operator if expressions in the fprintfs and presumably we still want the ability to dump non-html dumps.

Comment 3

[Andreas Gal :gal]

I crash on mac too, by the way

Comment 4

[Matt Woodrow (:mattwoodrow)]

This seems like a dup of 911730.

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> This seems like a dup of 911730.

With MOZ_DUMP_PAINTING symbol, we need these buffer checking conditions in attachment 801923 [details] [diff] [review] to prevent crash.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> This seems like a dup of 911730.

That bug helped, but my patch is also needed.

(In reply to Nick Cameron [:nrc] from comment #2)
> I don't see why you are adding these checks for aDumpHtml. They are covered
> by the ternary operator if expressions in the fprintfs and presumably we
> still want the ability to dump non-html dumps.

Getting the gralloc buffer contents crashes for me, somewhere in darkest Android. I don't think we need to try dumping mDeprecatedTextureHost contents for plaintext output, that seems unnecessary.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 807689 [details] [diff] [review]
fix v2

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

::: gfx/layers/composite/ContentHost.cpp
@@ +762,2 @@
>      fprintf(aFile, "%s", aPrefix);
>      fprintf(aFile, aDumpHtml ? "<li> <a href=" : "Back buffer: ");

You don't need these checks for aDumpHTML in the fprintfs. Probably nicer to do an early return if !aDumpHtml like you do in the base version.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/722a17aaa05b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/722a17aaa05b