Closed Bug 914426 Opened 6 years ago Closed 6 years ago

Dumping layer trees in B2G crashes

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attached patch fix (obsolete) — Splinter Review
Attachment #801923 - Flags: review?(ncameron)
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.
I crash on mac too, by the way
This seems like a dup of 911730.
(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.
(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.
Attached patch fix v2Splinter Review
Attachment #801923 - Attachment is obsolete: true
Attachment #801923 - Flags: review?(ncameron)
Attachment #807689 - Flags: review?(ncameron)
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.
Attachment #807689 - Flags: review?(ncameron) → review+
https://hg.mozilla.org/mozilla-central/rev/722a17aaa05b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.