739604 - Crash in ThebesLayerOGL when using MOZ_DUMP_PAINT_LIST

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Lord [:cwiiis]]

If you enable MOZ_DUMP_PAINT_LIST when using tiles and GL layers, you'll almost certainly get a crash due to uninitialised memory access. This is caused by using TextureImage calls before calling BeginIteration (meaning it may be in an inconsistent state).

Comment 1

[Chris Lord [:cwiiis]]

erk, so fixing that issue unfortunately just shows it crashing later down the line :( Trying to debug, but it's not giving me a stack...

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Restore old TiledTextureImage::NextTile behaviour

This restores the behaviour of TiledTextureImage::NextTile to before the patches in bug 732917. This fixes/avoids (at least) one of the crashes, I'm not sure what the cause of the other is yet.

Comment 3

[Ali Juma [:ajuma]]

Comment on attachment 609741 [details] [diff] [review]
Restore old TiledTextureImage::NextTile behaviour

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

::: gfx/gl/GLContext.cpp
@@ +1084,5 @@
> +        mCurrentImage++;
> +    else
> +        return false;
> +
> +    return continueIteration;

It's cleaner to go back to the way it was before:

if (mCurrentImage + 1 < mImages.Length()) {
    mCurrentImage++;
    return continueIteration;
}
return false;

Comment 4

[Chris Lord [:cwiiis]]

Attachment: Restore old TiledTextureImage::NextTile behaviour

Updated as suggested, and reset mCurrentImage when resizing - carrying over r+.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Add NULL check in Layers.cpp:WriteSnapshotToDumpFile_internal

This fixes the next crash I was seeing, where gfxUtils::sDumpPaintFile was NULL, but fprintf was being called using it as a parameter.

Comment 6

[Chris Lord [:cwiiis]]

MOZ_DUMP_PAINT_LIST is still not useful on mobile anymore because it enables read-back of (almost) every texture (which causes every composite to take in the region of 800-1500ms, instead of the usual 10-40ms) - This read-back also doesn't work correctly with tiled texture-images anyway, so I'll write a patch to separate it into a different option.

Comment 7

[Chris Lord [:cwiiis]]

Attachment: Split out paint buffer dumping from paint list dumping

Comment 8

[Chris Lord [:cwiiis]]

Pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/35546f02ddb1
http://hg.mozilla.org/integration/mozilla-inbound/rev/f4ab0eddbe90
http://hg.mozilla.org/integration/mozilla-inbound/rev/2fbd3599386c

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/35546f02ddb1
https://hg.mozilla.org/mozilla-central/rev/f4ab0eddbe90
https://hg.mozilla.org/mozilla-central/rev/2fbd3599386c