858039 - Assertion failure: !prevStream || mType != prevStream->mType (We should not need to create a SurfaceStream from another of the same type.), at src/gfx/gl/SurfaceStream.h:64

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Douglas Crosher [:dougc]]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:19.0) Gecko/20100101 Firefox/19.0
Build ID: 20130311191316

Steps to reproduce:

Run the demo at http://kripken.github.com/ammo.js/examples/new/ammo.html on a debug build of Firefox 23a1 for Android on the Android emulator.


Actual results:

A fatal assertion failure is triggered.

F/MOZ_Assert(  854): Assertion failure: !prevStream || mType != prevStream->mType (We should not need to create a SurfaceStream from another of the same type.), at /<src>/gfx/gl/SurfaceStream.h:64
F/libc    (  854): Fatal signal 11 (SIGSEGV) at 0x00000000 (code=1), thread 902 (Gecko)

A backtrace shows:

#0  mozilla::gfx::SurfaceStream::SurfaceStream (this=0x5add3e00, type=<optimized out>, prevStream=0x50cd6300) at <path>/gfx/gl/SurfaceStream.h:64
#1  0x51f9b7fe in mozilla::gfx::SurfaceStream_TripleBuffer::SurfaceStream_TripleBuffer (this=0x5add3e00, prevStream=0x50cd6300) at <path>/gfx/gl/SurfaceStream.cpp:359
#2  0x51f9b856 in mozilla::gfx::SurfaceStream_TripleBuffer_Async::SurfaceStream_TripleBuffer_Async (this=0x5add3e00, prevStream=<optimized out>) at <path>/gfx/gl/SurfaceStream.cpp:434
#3  0x51f9b966 in mozilla::gfx::SurfaceStream::CreateForType (type=<optimized out>, prevStream=0x50cd6300) at <path>/gfx/gl/SurfaceStream.cpp:42
#4  0x51f974b6 in mozilla::gl::GLScreenBuffer::Morph (this=0x57077800, newFactory=0x5adb06a0, streamType=<optimized out>) at <path>/gfx/gl/GLScreenBuffer.cpp:321
#5  0x51f5c768 in mozilla::layers::BasicShadowableCanvasLayer::Initialize (this=0x5a5cee00, aData=<optimized out>) at <path>/gfx/layers/basic/BasicCanvasLayer.cpp:400

The function Morph checks if the stream type is the same as the
previous stream type and returns without creating a new stream if
so. However the stream type TripleBuffer_Async is actually implemented
by creating a TripleBuffer stream so if the previous stream was a
TripleBuffer stream then the assertion can still fail when creating a
TripleBuffer_Async stream.

This appears to be related to the work on bug 829747.




Expected results:

The demo runs fine on some other platforms without hitting this assertion failure.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Indeed, in that patch we added a `TripleBuffer_Async` type, but just directly inherited from the TripleBuffer parent class, which uses `TripleBuffer` instead.

Comment 2

[Kelsey Gilbert [:jgilbert]]

To be clear, we should fix this.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Make SurfaceStream_TripleBuffer_Async use the correct SurfaceStreamType

This should fix it

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

This patch would be a lot cleaner if we could use delegated constructors, but we apparently cannot.

Comment 5

[Oleg Romashin (:romaxa)]

Sounds like same issue I see here:
https://github.com/tmeshkova/mozilla-central/issues/15

Comment 6

[Oleg Romashin (:romaxa)]

confirm, this patch fixes the problem

Comment 7

[Justin Wood (:Callek)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/04864ed004a7

Comment 8

[Douglas Crosher [:dougc]]

Thank you for the quick patch.  The patch does not appear to completely solve the problem because the mType is still incorrect when the assertion is checked.  I think I understand the intention of the patch and will look into an alternative soon.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/04864ed004a7

Comment 10

[Douglas Crosher [:dougc]]

The patch corrects the mType but the assertion check still fails due to the the mType being set correctly after the check.

Comment 11

[Douglas Crosher [:dougc]]

Attachment: Revert and add a new constructor for SurfaceStream_TripleBuffer to address the issue.

This patch reverts changeset 127889:04864ed004a7 which did not prevent the assertion failure, and alternatively adds a new constructor for SurfaceStream_TripleBuffer that accepts the SurfaceStreamType as an argument for initialization. Tested on Android and x64 Linux.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 734342 [details] [diff] [review]
Revert and add a new constructor for SurfaceStream_TripleBuffer to address the issue.

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

Thanks. I had this same patch locally but didn't have time to test. Looks good, though I think the new constructor should probably be protected instead of public.

Comment 13

[Douglas Crosher [:dougc]]

Attachment: Add a new constructor for SurfaceStream_TripleBuffer and use it to correctly initialize the SurfaceStreamType.

This minor revision makes the new constructor protected as the reviewer requested. Thank you for the very quick review and suggestion.  Re-tested on Android and x64 Linux.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5111f19fd4a6

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5111f19fd4a6