Last Comment Bug 739604 - Crash in ThebesLayerOGL when using MOZ_DUMP_PAINT_LIST
: Crash in ThebesLayerOGL when using MOZ_DUMP_PAINT_LIST
Status: RESOLVED FIXED
[native-crash]
: crash
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- critical (vote)
: mozilla14
Assigned To: Chris Lord [:cwiiis]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-27 07:56 PDT by Chris Lord [:cwiiis]
Modified: 2012-03-29 08:49 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Restore old TiledTextureImage::NextTile behaviour (2.11 KB, patch)
2012-03-27 09:03 PDT, Chris Lord [:cwiiis]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Restore old TiledTextureImage::NextTile behaviour (2.54 KB, patch)
2012-03-28 06:06 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
Details | Diff | Splinter Review
Add NULL check in Layers.cpp:WriteSnapshotToDumpFile_internal (1.23 KB, patch)
2012-03-28 06:07 PDT, Chris Lord [:cwiiis]
matt.woodrow: review+
Details | Diff | Splinter Review
Split out paint buffer dumping from paint list dumping (3.20 KB, patch)
2012-03-28 06:54 PDT, Chris Lord [:cwiiis]
matt.woodrow: review+
Details | Diff | Splinter Review

Description Chris Lord [:cwiiis] 2012-03-27 07:56:26 PDT
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] 2012-03-27 08:21:12 PDT
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] 2012-03-27 09:03:29 PDT
Created attachment 609741 [details] [diff] [review]
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] 2012-03-27 09:15:42 PDT
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] 2012-03-28 06:06:16 PDT
Created attachment 610098 [details] [diff] [review]
Restore old TiledTextureImage::NextTile behaviour

Updated as suggested, and reset mCurrentImage when resizing - carrying over r+.
Comment 5 Chris Lord [:cwiiis] 2012-03-28 06:07:42 PDT
Created attachment 610099 [details] [diff] [review]
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] 2012-03-28 06:12:24 PDT
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] 2012-03-28 06:54:19 PDT
Created attachment 610119 [details] [diff] [review]
Split out paint buffer dumping from paint list dumping

Note You need to log in before you can comment on or make changes to this bug.