856671 - Crash in TiledLayerBuffer.EmptyUpdate unit test (gtest)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Vasil Dimov [:vd]]

The unit test TiledLayerBuffer.EmptyUpdate, defined in gfx/layers/TestTiledLayerBuffer.cpp has the following two problems:

To reproduce run:
firefox -unittest
or
GTEST_FILTER=TiledLayerBuffer.EmptyUpdate firefox -unittest

Problem 1:
----------

gRectPoolTlsIndex.init() (gRectPoolTlsIndex is defined as a global variable in nsRegion.cpp) is never called and thus gRectPoolTlsIndex.inited remains false when gRectPoolTlsIndex.get() is called later which leads to an assertion failure:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff01bcb23 in mozilla::ThreadLocal<RgnRectMemoryAllocator*>::get (
    this=0x7ffff52949b0 <gRectPoolTlsIndex>)
    at ../../dist/include/mozilla/ThreadLocal.h:118
118	  MOZ_ASSERT(initialized());
(gdb) bt
#0  0x00007ffff01bcb23 in mozilla::ThreadLocal<RgnRectMemoryAllocator*>::get (
    this=0x7ffff52949b0 <gRectPoolTlsIndex>)
    at ../../dist/include/mozilla/ThreadLocal.h:118
#1  0x00007ffff01b773b in nsRegion::RgnRect::operator new ()
    at /home/vd/m/mozilla-central/gfx/src/nsRegion.cpp:215
#2  0x00007ffff01b7990 in nsRegion::SetToElements (this=0x7fffffffcff0, aCount=1)
    at /home/vd/m/mozilla-central/gfx/src/nsRegion.cpp:278
#3  0x00007ffff01b88a3 in nsRegion::Copy (this=0x7fffffffcff0, aRect=...)
    at /home/vd/m/mozilla-central/gfx/src/nsRegion.cpp:606
#4  0x00007ffff01b6aa9 in nsRegion::nsRegion (this=0x7fffffffcff0, aRect=...)
    at /home/vd/m/mozilla-central/gfx/src/nsRegion.h:73
#5  0x00007ffff01d438c in nsIntRegion::nsIntRegion (this=0x7fffffffcff0, aRect=...)
    at ../../dist/include/nsRegion.h:285
#6  0x00007ffff220346b in mozilla::layers::TiledLayerBuffer_EmptyUpdate_Test::TestBody (this=0x7ffff6c22170)
    at /home/vd/m/mozilla-central/gfx/layers/TestTiledLayerBuffer.cpp:71
...
(gdb) f 0
#0  0x00007ffff01bcb23 in mozilla::ThreadLocal<RgnRectMemoryAllocator*>::get (
    this=0x7ffff52949b0 <gRectPoolTlsIndex>)
    at ../../dist/include/mozilla/ThreadLocal.h:118
118	  MOZ_ASSERT(initialized());
(gdb) p inited
$1 = false

The solution could be to call nsRegion::InitStatic() (which calls gRectPoolTlsIndex.init()) beforehand.

Problem 2:
----------

Can only get here if Problem 1 is solved because it crashes earlier otherwise.

TestTiledLayerTile (in TestTiledLayerBuffer.cpp) is defined in such a way that any object of that type will always be equal to any other object of that type. This is because the 'value' member is initialized to 0 and never changed afterwards and the operator==() method just compares the 'value' members of the two objects being compared. So TiledLayerBuffer::IsPlaceholder() will always return true because it compares two such objects. TiledLayerBuffer::Update() contains an assert that IsPlaceholder() does not return true. That assert fails:

###!!! ABORT: index out of range: '!IsPlaceholder(newTile)', file /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h, line 420

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5426551 in mozalloc_abort (
    msg=0x7fffffffc970 "###!!! ABORT: index out of range: '!IsPlaceholder(newTile)', file /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h, line 420")
    at /home/vd/m/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30
30	    MOZ_CRASH();
(gdb) bt
#0  0x00007ffff5426551 in mozalloc_abort (
    msg=0x7fffffffc970 "###!!! ABORT: index out of range: '!IsPlaceholder(newTile)', file /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h, line 420")
    at /home/vd/m/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:30
#1  0x00007ffff209f914 in Abort (
    aMsg=0x7fffffffc970 "###!!! ABORT: index out of range: '!IsPlaceholder(newTile)', file /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h, line 420")
    at /home/vd/m/mozilla-central/xpcom/base/nsDebugImpl.cpp:430
#2  0x00007ffff209f81f in NS_DebugBreak (aSeverity=3, 
    aStr=0x7ffff3d991e7 "index out of range", 
    aExpr=0x7ffff3d991fa "!IsPlaceholder(newTile)", 
    aFile=0x7ffff3d98fe8 "/home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h", aLine=420) at /home/vd/m/mozilla-central/xpcom/base/nsDebugImpl.cpp:387
#3  0x00007ffff220413b in mozilla::layers::TiledLayerBuffer<mozilla::layers::TestTiledLayerBuffer, mozilla::layers::TestTiledLayerTile>::Update (this=0x7fffffffd030, 
    aNewValidRegion=..., aPaintRegion=...)
    at /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h:420
#4  0x00007ffff220304f in mozilla::layers::TestTiledLayerBuffer::TestUpdate (
    this=0x7fffffffd030, aNewValidRegion=..., aPaintRegion=...)
    at /home/vd/m/mozilla-central/gfx/layers/TestTiledLayerBuffer.cpp:53
#5  0x00007ffff2203490 in mozilla::layers::TiledLayerBuffer_EmptyUpdate_Test::TestBody (this=0x7ffff6c22170)
    at /home/vd/m/mozilla-central/gfx/layers/TestTiledLayerBuffer.cpp:73
...
(gdb) f 3
#3  0x00007ffff220413b in mozilla::layers::TiledLayerBuffer<mozilla::layers::TestTiledLayerBuffer, mozilla::layers::TestTiledLayerTile>::Update (this=0x7fffffffd030, 
    aNewValidRegion=..., aPaintRegion=...)
    at /home/vd/m/mozilla-central/gfx/layers/TiledLayerBuffer.h:420
420	      NS_ABORT_IF_FALSE(!IsPlaceholder(newTile), "index out of range");
(gdb) p newTile.value
$1 = 0
(gdb) p AsDerived().GetPlaceholderTile().value
$2 = 0
(gdb) 

The solution could be to change the test's GetPlaceholderTile() method to return an object with a 'value' member set to something other than 0.

The tree I used is mozilla-central as of 126538:2f63e2f90d5c from Mar 28 2013.

Comment 1

[Vasil Dimov [:vd]]

Attachment: Fix

Fix the problems as suggested in the bug description.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 731931 [details] [diff] [review]
Fix

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

Thanks for the patch.

::: gfx/layers/TestTiledLayerBuffer.cpp
@@ +29,4 @@
>  
>  public:
>    TestTiledLayerTile GetPlaceholderTile() const {
> +    return TestTiledLayerTile(1);

Lets use -1 instead

Comment 3

[Vasil Dimov [:vd]]

Attachment: Fix

Fix, use -1 as suggested by Benoit.

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Vasil Dimov [:vd] from comment #3)
> Created attachment 731979 [details] [diff] [review]
> Fix
> 
> Fix, use -1 as suggested by Benoit.

Thanks for the fix, thanks for the review! Now I don't have to keep disabling the test in the makefile :-)

Comment 5

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06c5bffd4437

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06c5bffd4437