1000634 - MOZ_ASSERT(mBuffer->IsAttached()) at ThebesLayerComposite.cpp:95

Reporter

Description

•

10 years ago

On nexus4, current trunk

STR:
Change wallpaper.

repro maybe 1/10 times.

[Parent 861] WARNING: Found a non-root APZ with no handoff parent: file ../../../gfx/layers/apz/src/APZCTreeManager.cpp, line 909
[Parent 861] WARNING: Found a non-root APZ with no handoff parent: file ../../../gfx/layers/apz/src/APZCTreeManager.cpp, line 909
[Parent 861] WARNING: Found a non-root APZ with no handoff parent: file ../../../gfx/layers/apz/src/APZCTreeManager.cpp, line 909
[Parent 861] WARNING: Found a non-root APZ with no handoff parent: file ../../../gfx/layers/apz/src/APZCTreeManager.cpp, line 909
[Parent 861] WARNING: Found a non-root APZ with no handoff parent: file ../../../gfx/layers/apz/src/APZCTreeManager.cpp, line 909
[Child 1777] WARNING: standard_derivatives marked as unsupported: file ../../../gfx/gl/GLContextFeatures.cpp, line 536
[Child 1777] WARNING: robustness marked as unsupported: file ../../../gfx/gl/GLContextFeatures.cpp, line 536
[Child 1777] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file ../../../../embedding/browser/webBrowser/nsWebBrowser.cpp, line 365
[Child 1777] WARNING: NS_ENSURE_TRUE(domWindow) failed: file ../../../../embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 86
System JS : ERROR chrome://global/content/BrowserElementChildPreload.js:41 - NS_ERROR_ILLEGAL_VALUE: Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIMessageSender.sendAsyncMessage]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 861.908]
0xb4724c7a in GetTiledLayerComposer (this=<optimized out>) at ../../../gfx/layers/composite/ThebesLayerComposite.cpp:95
95	  MOZ_ASSERT(mBuffer->IsAttached());
(gdb) bt
#0  0xb4724c7a in GetTiledLayerComposer (this=<optimized out>) at ../../../gfx/layers/composite/ThebesLayerComposite.cpp:95
#1  mozilla::layers::ThebesLayerComposite::GetTiledLayerComposer (this=<optimized out>) at ../../../gfx/layers/composite/ThebesLayerComposite.cpp:90
#2  0xb4733164 in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=0xa9ca5800) at ../../../gfx/layers/opengl/CompositorOGL.cpp:1245
#3  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
#4  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
#5  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
#6  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
#7  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
[Child 1777] WARNING: NS_ENSURE_TRUE(inBrowser) failed: file ../../../../embedding/browser/webBrowser/nsDocShellTreeOwner.cpp, line 82#8  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236

#9  0xb473314c in mozilla::layers::CompositorOGL::SetFBAcquireFence (this=0xae547c20, aLayer=<optimized out>)
    at ../../../gfx/layers/opengl/CompositorOGL.cpp:1236
#10 0xb47233e0 in mozilla::layers::LayerManagerComposite::Render (this=0xae547d00) at ../../../gfx/layers/composite/LayerManagerComposite.cpp:523
#11 0xb4723670 in EndTransaction (aFlags=<optimized out>, this=0xae547d00, aCallback=<optimized out>, aCallbackData=<optimized out>)
    at ../../../gfx/layers/composite/LayerManagerComposite.cpp:247
#12 mozilla::layers::LayerManagerComposite::EndTransaction (this=0xae547d00, aCallback=0, aCallbackData=<optimized out>, aFlags=<optimized out>)
    at ../../../gfx/layers/composite/LayerManagerComposite.cpp:203
#13 0xb4721bac in mozilla::layers::LayerManagerComposite::EndEmptyTransaction (this=0xae547d00, aFlags=<optimized out>)
    at ../../../gfx/layers/composite/LayerManagerComposite.cpp:198
#14 0xb472914a in mozilla::layers::CompositorParent::CompositeToTarget (this=0xb008d400, aTarget=0x0) at ../../../gfx/layers/ipc/CompositorParent.cpp:656
#15 0xb4272290 in DispatchToMethod<FdWatcher, void (FdWatcher::*)()> (method=
    (void (FdWatcher::*)(FdWatcher * const)) 0xb4729297 <mozilla::layers::CompositorParent::Composite()>, obj=<optimized out>, arg=<optimized out>)
    at ../../../ipc/chromium/src/base/tuple.h:383
#16 RunnableMethod<FdWatcher, void (FdWatcher::*)(), Tuple0>::Run (this=<optimized out>) at ../../../ipc/chromium/src/base/task.h:307
#17 0xb44461c4 in MessageLoop::RunTask (this=0xafaffde0, task=0xae6b4140) at ../../../ipc/chromium/src/base/message_loop.cc:344
#18 0xb4446b02 in MessageLoop::DeferOrRunPendingTask (this=<optimized out>, pending_task=<optimized out>) at ../../../ipc/chromium/src/base/message_loop.cc:352
#19 0xb4446e06 in DoDelayedWork (next_delayed_work_time=0xb203b6f0, this=0xafaffde0) at ../../../ipc/chromium/src/base/message_loop.cc:457
#20 MessageLoop::DoDelayedWork (this=0xafaffde0, next_delayed_work_time=0xb203b6f0) at ../../../ipc/chromium/src/base/message_loop.cc:440
#21 0xb4447e2e in base::MessagePumpDefault::Run (this=0xb203b6e0, delegate=0xafaffde0) at ../../../ipc/chromium/src/base/message_pump_default.cc:39
#22 0xb444630a in MessageLoop::RunInternal (this=0xafaffde0) at ../../../ipc/chromium/src/base/message_loop.cc:226
#23 0xb4446322 in RunHandler (this=0xafaffde0) at ../../../ipc/chromium/src/base/message_loop.cc:219
#24 MessageLoop::Run (this=0xafaffde0) at ../../../ipc/chromium/src/base/message_loop.cc:193
#25 0xb44499ae in base::Thread::ThreadMain (this=0xb203a730) at ../../../ipc/chromium/src/base/thread.cc:164
#26 0xb443e1fc in ThreadFunc (closure=<optimized out>) at ../../../ipc/chromium/src/base/platform_thread_posix.cc:39
#27 0xb6e9aa5c in __thread_entry (func=0xb443e1f5 <ThreadFunc(void*)>, arg=0xb203a730, tls=0xafafff00) at bionic/libc/bionic/pthread_create.cpp:92


Main thread:
(gdb) thread 1
[Switching to thread 1 (Thread 861.861)]
#0  __aeabi_ldivmod () at /Volumes/android/AOSP-toolchain/build/../gcc/gcc-4.7/libgcc/config/arm/bpabi.S:110
110	/Volumes/android/AOSP-toolchain/build/../gcc/gcc-4.7/libgcc/config/arm/bpabi.S: No such file or directory.
	in /Volumes/android/AOSP-toolchain/build/../gcc/gcc-4.7/libgcc/config/arm/bpabi.S
(gdb) bt
#0  __aeabi_ldivmod () at /Volumes/android/AOSP-toolchain/build/../gcc/gcc-4.7/libgcc/config/arm/bpabi.S:110
#1  0xb53c7df8 in bits_image_fetch_general (iter=<optimized out>, mask=0xbe83a780) at ../../../../../gfx/cairo/libpixman/src/pixman-bits-image.c:702
#2  0xb53f6314 in general_composite_rect (imp=0xabd93800, info=<optimized out>) at ../../../../../gfx/cairo/libpixman/src/pixman-general.c:211
#3  0xb53fd55e in _moz_pixman_image_composite32 (op=<optimized out>, src=<optimized out>, mask=<optimized out>, dest=<optimized out>, src_x=0, src_y=0, 
    mask_x=0, mask_y=48, dest_x=0, dest_y=48, width=768, height=1132) at ../../../../../gfx/cairo/libpixman/src/pixman.c:707
#4  0xb53a760a in _composite_mask (closure=<optimized out>, dst=<optimized out>, dst_format=<optimized out>, op=<optimized out>, src_pattern=0xbe83fe28, 
    dst_x=0, dst_y=0, extents=0xbe83fd8c, clip_region=0x0) at ../../../../../gfx/cairo/cairo/src/cairo-image-surface.c:3356
#5  0xb53a8922 in _clip_and_composite (dst=0xa9c44aa0, op=<optimized out>, src=0xbe83fe28, draw_func=0xb53a7561 <_composite_mask>, draw_closure=0xbe83ff10, 
    extents=0xbe83fd6c, clip=0x0) at ../../../../../gfx/cairo/cairo/src/cairo-image-surface.c:2328
#6  0xb53a9a36 in _cairo_image_surface_mask (abstract_surface=0xa9c44aa0, op=CAIRO_OPERATOR_OVER, source=0xbe83fe28, mask=0xbe83ff10, clip=0x0)
    at ../../../../../gfx/cairo/cairo/src/cairo-image-surface.c:3407
#7  0xb53b9932 in _cairo_surface_mask (surface=0xa9c44aa0, op=CAIRO_OPERATOR_OVER, source=0xbe83fe28, mask=0xbe83ff10, clip=0xbe83fdf8)
    at ../../../../../gfx/cairo/cairo/src/cairo-surface.c:2164
#8  0xb53a4868 in _cairo_gstate_mask (gstate=<optimized out>, mask=<optimized out>) at ../../../../../gfx/cairo/cairo/src/cairo-gstate.c:1124
#9  0xb53bf414 in _moz_cairo_paint_with_alpha (cr=0xabb6b800, alpha=0.80000001192092896) at ../../../../../gfx/cairo/cairo/src/cairo.c:2283
#10 0xb46843d0 in mozilla::gfx::DrawTargetCairo::DrawSurface (this=0xaed02a60, aSurface=<optimized out>, aDest=..., aSource=<optimized out>, aSurfOptions=..., 
    aOptions=...) at ../../../gfx/2d/DrawTargetCairo.cpp:556
#11 0xb46e3aac in gfxContext::Paint (this=0xaec0f320, alpha=<optimized out>) at ../../../gfx/thebes/gfxContext.cpp:1563
#12 0xb470de10 in mozilla::layers::PaintWithMask (aContext=0xaec0f320, aOpacity=0.800000012, aMaskLayer=0x0)
    at ../../../gfx/layers/basic/BasicLayersImpl.cpp:55
#13 0xb470e80a in mozilla::layers::BasicThebesLayer::PaintThebes (this=0xb2099930, aContext=0xaec0f320, aMaskLayer=0x0, aCallback=
    0xb4fe4935 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=0xbe841140, aReadback=0xbe840720) at ../../../gfx/layers/basic/BasicThebesLayer.cpp:107
#14 0xb470cd4e in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0xabb667e0, aPaintContext=..., aGroupTarget=0xaec0f320)
    at ../../../gfx/layers/basic/BasicLayerManager.cpp:835
#15 0xb470d064 in mozilla::layers::BasicLayerManager::PaintLayer (this=0xabb667e0, aTarget=0xaec0f320, aLayer=0xb2099930, aCallback=<optimized out>, 
    aCallbackData=0xbe841140, aReadback=0xbe840720) at ../../../gfx/layers/basic/BasicLayerManager.cpp:964
#16 0xb470cd9c in mozilla::layers::BasicLayerManager::PaintSelfOrChildren (this=0xabb667e0, aPaintContext=..., aGroupTarget=0xaec0f320)
    at ../../../gfx/layers/basic/BasicLayerManager.cpp:851
#17 0xb470d064 in mozilla::layers::BasicLayerManager::PaintLayer (this=0xabb667e0, aTarget=0xaec0f320, aLayer=0xac3f7400, aCallback=<optimized out>, 
    aCallbackData=0xbe841140, aReadback=0x0) at ../../../gfx/layers/basic/BasicLayerManager.cpp:964
#18 0xb470d86a in mozilla::layers::BasicLayerManager::EndTransactionInternal (this=0xabb667e0, 
    aCallback=0xb4fe4935 <mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)>, aCallbackData=<optimized out>, aFlags=<optimized out>) at ../../../gfx/layers/basic/BasicLayerManager.cpp:627
#19 0xb4fe45a2 in PaintInactiveLayer (aCtx=<optimized out>, aContext=0xaec0f320, aItem=0xa9ccceb0, aManager=0xabb667e0, aBuilder=<optimized out>)
    at ../../../layout/base/FrameLayerBuilder.cpp:2333
#20 mozilla::FrameLayerBuilder::PaintItems (this=0xaf8dde20, aItems=..., aRect=<optimized out>, aContext=0xaec0f320, aRC=0xae52e5e0, aBuilder=0xbe841140, 
    aPresContext=0xae217c00, aOffset=..., aXScale=1, aYScale=1, aCommonClipCount=0) at ../../../layout/base/FrameLayerBuilder.cpp:3645
#21 0xb4fe4c98 in mozilla::FrameLayerBuilder::DrawThebesLayer (aLayer=0xaec726b0, aContext=0xaec0f320, aRegionToDraw=..., aClip=<optimized out>, 
    aRegionToInvalidate=..., aCallbackData=0xbe841140) at ../../../layout/base/FrameLayerBuilder.cpp:3822

Gregor Wagner [:gwagner]

Reporter

Updated

•

10 years ago

Flags: needinfo?(milan)

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

Assignee

Comment 1

•

10 years ago

I'm assuming regression, so the range would be useful, but I have a feeling that Kats or Botond would be able to figure out which change it was even without that.

blocking-b2g: --- → 2.0+

Component: Graphics → Panning and Zooming

Flags: needinfo?(milan)

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Comment 2

•

10 years ago

This doesn't look like an APZ bug; the warnings in comment 0 are APZ-related but look unrelated to the crash.

Component: Panning and Zooming → Graphics: Layers

Keywords: regressionwindow-wanted

Jason Smith [:jsmith]

Comment 3

•

10 years ago

We can't do windows on debug assertions.

Keywords: regressionwindow-wanted

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Comment 4

•

10 years ago

In that case somebody who knows the code should be looking at this. Not sure who that is - maybe nical (because mBuffer is a ContentHost) or Sotaro (because of the Fence stuff in the stack)?

Flags: needinfo?(sotaro.ikeda.g)

Flags: needinfo?(nical.bugzilla)

Nicolas Silva [:nical]

Comment 5

•

10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> In that case somebody who knows the code should be looking at this. Not sure
> who that is - maybe nical (because mBuffer is a ContentHost) or Sotaro
> (because of the Fence stuff in the stack)?

Looking at the code it looks like we are checking that if the layers has a reference to a compositable, then the compositable should be attached (implied: to the layer). This assertion breaks there. It's not obvious to me why we have a compositable that is not attached.
It would mean something like we successfully called LayerCOmposite::SetCompositable but didn't call Attach on the corresponding Compositable, or we detached the compositable without nulling the layer's reference to it.

I briefly looked but didn't find a place where either of these two can occur (for Thebes layers).

Flags: needinfo?(nical.bugzilla)

Botond Ballo [:botond]

Comment 6

•

10 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> the warnings in comment 0 are APZ-related but look unrelated to the crash.

I filed bug 1001550 for these warnings.

Jason Smith [:jsmith]

Comment 7

•

10 years ago

Why is this a blocker - this is just a debug assertion? Renom for discussion.

blocking-b2g: 2.0+ → 2.0?

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

Assignee

Comment 8

•

10 years ago

(In reply to Jason Smith [:jsmith] from comment #7)
> Why is this a blocker - this is just a debug assertion? Renom for discussion.

Probably more appropriate as feature-b2g set to 2.0 or 2.1

bhavana bajaj [:bajaj]

Comment 9

•

10 years ago

(In reply to Jason Smith [:jsmith] from comment #7)
> Why is this a blocker - this is just a debug assertion? Renom for discussion.

I agree, I am clearing the nomination, :milan can help set the target feature-b2g flag

blocking-b2g: 2.0? → ---

Kartikaya Gupta (email:kats@mozilla.staktrace.com)

Updated

•

10 years ago

Flags: needinfo?(sotaro.ikeda.g)

Gregor Wagner [:gwagner]

Reporter

Comment 10

•

10 years ago

I am still seeing this on 2.0 on the flame

Gregor Wagner [:gwagner]

Reporter

Comment 11

•

10 years ago

My stability testing gets stuck here over and over again. Milan, can we finally fix this?

Flags: needinfo?(milan)

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

Assignee

Comment 12

•

10 years ago

It's really difficult to prioritize Nexus 4 debug build issue, this is why nothing has been done.  I'll see if I can reproduce it on my build.

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

Assignee

Comment 13

•

10 years ago

Yes, I can reproduce locally.

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

Assignee

Comment 14

•

10 years ago

Gregor, I'm assuming you also have JellyBean based Nexus 4?

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

Assignee

Comment 15

•

10 years ago

(In reply to Nicolas Silva [:nical] from comment #5)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> > In that case somebody who knows the code should be looking at this. Not sure
> > who that is - maybe nical (because mBuffer is a ContentHost) or Sotaro
> > (because of the Fence stuff in the stack)?
> 
> Looking at the code it looks like we are checking that if the layers has a
> reference to a compositable, then the compositable should be attached
> (implied: to the layer). This assertion breaks there. It's not obvious to me
> why we have a compositable that is not attached.
> It would mean something like we successfully called
> LayerCOmposite::SetCompositable but didn't call Attach on the corresponding
> Compositable, or we detached the compositable without nulling the layer's
> reference to it.
> 
> I briefly looked but didn't find a place where either of these two can occur
> (for Thebes layers).

What if there is a color layer involved? I've mostly been able to reproduce this when changing the wallpaper between image and solid color (in both directions, but not all the time.)

Flags: needinfo?(nical.bugzilla)

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

Assignee

Comment 16

•

10 years ago

(In reply to Gregor Wagner [:gwagner] from comment #11)
> My stability testing gets stuck here over and over again. Milan, can we
> finally fix this?

Not a solution, but you can try running locally with this patch and hopefully unblock:

--- a/gfx/layers/composite/ThebesLayerComposite.cpp
+++ b/gfx/layers/composite/ThebesLayerComposite.cpp
@@ -88,10 +88,9 @@ ThebesLayerComposite::GetLayer()
 TiledLayerComposer*
 ThebesLayerComposite::GetTiledLayerComposer()
 {
-  if (!mBuffer) {
+  if (!mBuffer || !mBuffer->IsAttached()) {
     return nullptr;
   }
-  MOZ_ASSERT(mBuffer->IsAttached());
   return mBuffer->AsTiledLayerComposer();
 }

Nicolas Silva [:nical]

Comment 17

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #15)
> What if there is a color layer involved? I've mostly been able to reproduce
> this when changing the wallpaper between image and solid color (in both
> directions, but not all the time.)

It's not obvious to me how a color layer could be involved, but I agree that we're better off taking your patch and not blocking statibilty testing. Adding a warning would be good since something's still not right.

Flags: needinfo?(nical.bugzilla)

Gregor Wagner [:gwagner]

Reporter

Comment 18

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #14)
> Gregor, I'm assuming you also have JellyBean based Nexus 4?

Thats happening on the flame now.

Gregor Wagner [:gwagner]

Reporter

Comment 19

•

10 years ago

(In reply to Gregor Wagner [:gwagner] from comment #18)
> (In reply to Milan Sreckovic [:milan] from comment #14)
> > Gregor, I'm assuming you also have JellyBean based Nexus 4?
> 
> Thats happening on the flame now.

Oh and I am running kk

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

Assignee

Comment 20

•

10 years ago

(In reply to Nicolas Silva [:nical] from comment #5)
> ...It's not obvious to me
> why we have a compositable that is not attached.
> It would mean something like we successfully called
> LayerCOmposite::SetCompositable but didn't call Attach on the corresponding
> Compositable, or we detached the compositable without nulling the layer's
> reference to it.
> 
> I briefly looked but didn't find a place where either of these two can occur
> (for Thebes layers).

Just adding some details.

The unattached buffer in question, that causes this assertion, is attached at some point, then detached, then we call GetTiledLayerComposer.

Incidentally, we call Detach() very often on buffers that are not attached, and Attach() very often on the buffers that are already attached.  Not in this case though.

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

Assignee

Comment 21

•

10 years ago

Recording some notes so that I don't forget.

LayerManagerComposite::Render calls (on mCompositor) CompositorOGL::SetFBAcquireFence, which calls SetFBAcquireFence on all the children of the container layer, and then wants a TiledLayerComposer (the mBuffer of ThebesLayerComposite), and calls SetReleaseFence() on it. When this mBuffer is detached, we assert. 

This buffer is detached because CompositableParent::ActorDestroy calls TiledContentHost::Detach, which eventually calls CompositableHost::Detach.  (By the way, why is it that CompositableHost::Attach sets the compositor pointer, but CompositableHost::Detach does not "unset" the compositor pointer?)

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

Assignee

Comment 22

•

10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=198ab4771fdc

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

Assignee

Comment 23

•

10 years ago

Attached patch Don't ask for composer if there is no compositable host. — Details — Splinter Review

This should take care of it, but I don't know if we should have to deal with this scenario, or if we get to this point in the code erroneously when we've lost the host connection...

Attachment #8485874 - Flags: review?(sotaro.ikeda.g)

Attachment #8485874 - Flags: review?(nical.bugzilla)

Flags: needinfo?(milan)

Nicolas Silva [:nical]

Updated

•

10 years ago

Attachment #8485874 - Flags: review?(nical.bugzilla) → review+

Nicolas Silva [:nical]

Comment 24

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #20)
> Incidentally, we call Detach() very often on buffers that are not attached,
> and Attach() very often on the buffers that are already attached.  Not in
> this case though.

This rings an alarm bell. Especially the thing about calling Attach on already attached compositables.
I guess it's okay to call detach once on a compositable if it was created but never got attached (for timing reasons, maybe), but if we do it too often it means we are doing useless work.

Nicolas Silva [:nical]

Comment 25

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #21)
> (By the way, why is it that CompositableHost::Attach sets the compositor
> pointer, but CompositableHost::Detach does not "unset" the compositor
> pointer?)

We could unset the compositor. We are usually a bit "lazy" about doing this because in some places, if you set the compositor to a different one, you have to throw away resources (like already uploaded textures, etc.) so if you set a compositor, then unset it and for some reason set it back to the same one, you'll have discarded resources that you could have kept. Now, this is more true for textures than compositables, so maybe it doesn't really matter that much for compositables and we could just unset the compositor there. Generally speaking, we have had to set compositors in places where we didn't expect to have to, because of corner cases with the ordering of messages and with async-video.

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

Assignee

Comment 26

•

10 years ago

(In reply to Nicolas Silva [:nical] from comment #24)
> (In reply to Milan Sreckovic [:milan] from comment #20)
> > Incidentally, we call Detach() very often on buffers that are not attached,
> > and Attach() very often on the buffers that are already attached.  Not in
> > this case though.
> 
> This rings an alarm bell. Especially the thing about calling Attach on
> already attached compositables.

I'll follow up on this and try to get the details.  Eventually :)

> I guess it's okay to call detach once on a compositable if it was created
> but never got attached (for timing reasons, maybe), but if we do it too
> often it means we are doing useless work.

Sotaro Ikeda [:sotaro]

Updated

•

10 years ago

Attachment #8485874 - Flags: review?(sotaro.ikeda.g) → review+

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

Assignee

Updated

•

10 years ago

Assignee: nobody → milan

Keywords: checkin-needed

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

Assignee

Comment 27

•

10 years ago

In addition to try from comment 22, there is also https://tbpl.mozilla.org/?tree=Try&rev=1496a7164e1a

Carsten Book [:Tomcat]

Comment 28

•

10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0fcbcc8b789

Keywords: checkin-needed

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 29

•

10 years ago

https://hg.mozilla.org/mozilla-central/rev/c0fcbcc8b789

Status: NEW → RESOLVED

Closed: 10 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla35

Gregor Wagner [:gwagner]

Reporter

Comment 30

•

10 years ago

[Blocking Requested - why for this release]:
I hit this assertion maybe 4/5 times when I take a picture and set it as a background on 2.1

blocking-b2g: --- → 2.1?

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

Assignee

Comment 31

•

10 years ago

Gregor, you're still seeing this, or it's gone on the trunk, and you're just asking for an uplift?

OS: Mac OS X → Gonk (Firefox OS)

Gregor Wagner [:gwagner]

Reporter

Comment 32

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #31)
> Gregor, you're still seeing this, or it's gone on the trunk, and you're just
> asking for an uplift?

I was only testing on 2.1 so asking for an uplift.

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

Assignee

Comment 33

•

10 years ago

For triage - this message shows up in debug only, but it's a precursor to crash, so the fix does have an effect in the release build as well.

bhavana bajaj [:bajaj]

Comment 34

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #33)
> For triage - this message shows up in debug only, but it's a precursor to
> crash, so the fix does have an effect in the release build as well.

:milan, can you re-confirm that the crash happens on release builds without which I want to avoid landing this in 2.1 at this point ? i was not sure about the comment, hence the clarification request.

bhavana bajaj [:bajaj]

Updated

•

10 years ago

Flags: needinfo?(milan)

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

Assignee

Comment 35

•

10 years ago

I had it crash on the release build, but on Nexus 4.  Gregor also saw it on the Flame.

Flags: needinfo?(milan)

Dietrich Ayala (:dietrich)

Comment 36

•

10 years ago

Milan, can you provide an estimate of risk and code footprint? Triage would like to know before approving uplift. Thanks!

Flags: needinfo?(milan)

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

Assignee

Comment 37

•

10 years ago

OK, I've been looking at too many bugs and everything is just getting mixed up.  Now I'm questioning whether I saw the crash in the release build at all.  Also, this is a bit of an unexplained situation, so the fix could be just wallpapering over another problem, or it could be what we should be doing.  The code change is small, we avoid making a call that ends up being useless anyway, not a lot of risk.  On the other hand, the trunk has changed in this area since this fix, so if there are problems they will be specific to 2.1 and thus more expensive to fix.  Maybe it is better to just let this ride the trains...

Flags: needinfo?(milan)

bhavana bajaj [:bajaj]

Comment 38

•

10 years ago

(In reply to Milan Sreckovic [:milan] from comment #37)
> OK, I've been looking at too many bugs and everything is just getting mixed
> up.  Now I'm questioning whether I saw the crash in the release build at
> all.  Also, this is a bit of an unexplained situation, so the fix could be
> just wallpapering over another problem, or it could be what we should be
> doing.  The code change is small, we avoid making a call that ends up being
> useless anyway, not a lot of risk.  On the other hand, the trunk has changed
> in this area since this fix, so if there are problems they will be specific
> to 2.1 and thus more expensive to fix.  Maybe it is better to just let this
> ride the trains...
Agree on this riding the trains. Gregor, if you are able to repro the crash on release builds, feel free to re-nom it for 2.1.

blocking-b2g: 2.1? → -