New intermittent crash on Talos[@ nsRefPtr<TypeInState>::assign_assuming_AddRef(TypeInState*) | nsRefPtr<mozilla::net::HttpBaseChannel>::assign_with_AddRef(mozilla::net::HttpBaseChannel*) | nsRefPtr<IDirect3DTexture9>::operator=(IDirect3DTexture9*) | mozi

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: bas, Assigned: bas)

Tracking

({crash, reproducible})

unspecified
x86
Windows 7
crash, reproducible
Points:
---

Firefox Tracking Flags

(blocking2.0 beta9+)

Details

Attachments

(7 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
First seen in: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1292927135.1292927747.3975.gz



This will probably start showing up in crash-stats as well. Although I don't have an idea on how it can happen just yet:

RETURN:s: talos-r3-xp-017
RETURN:id:20101221015339
RETURN:<a href = "http://hg.mozilla.org/mozilla-central/rev/fb629ae54510">rev:fb629ae54510</a>
talos-r3-xp-017: 
		Started Tue, 21 Dec 2010 02:33:51
Running test tp4: 
		Started Tue, 21 Dec 2010 02:33:51
	Screen width/height:1280/1024
	colorDepth:24
	Browser inner width/height: 1016/653

NOISE: Cycle 1: loaded http://localhost/page_load_test/tp4/www.youtube.com/www.youtube.com/index.html (next: http://localhost/page_load_test/tp4/www.msn.com/www.msn.com/index.html)
NOISE: 
NOISE: __FAILbrowser non-zero return code (253)__FAIL
NOISE: Cycle 1: loaded http://localhost/page_load_test/tp4/www.youtube.com/www.youtube.com/index.html (next: http://localhost/page_load_test/tp4/www.msn.com/www.msn.com/index.html)
NOISE: 
NOISE: __FAILbrowser non-zero return code (253)__FAIL
NOISE: Found crashdump: c:\docume~1\cltbld\locals~1\temp\tmplygzt5\profile\minidumps\ec742f71-8bf9-472f-9f4a-9fd3af236fca.dmp
Operating system: Windows NT
                  5.1.2600 Service Pack 2
CPU: x86
     GenuineIntel family 6 model 23 stepping 10
     2 CPUs

Crash reason:  EXCEPTION_ACCESS_VIOLATION
Crash address: 0x2

Thread 0 (crashed)
 0  xul.dll!nsRefPtr<nsHTMLInputElementState>::assign_assuming_AddRef(nsHTMLInputElementState *) [nsAutoPtr.h:fb629ae54510 : 957 + 0x0]
    eip = 0x104abe16   esp = 0x0012cd70   ebp = 0x0012cdc8   ebx = 0x046e48f8
    esi = 0x00000000   edi = 0x046e48f8   eax = 0x05b152e4   ecx = 0x00000002
    edx = 0x00000000   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  xul.dll!nsRefPtr<nsHTMLStyleSheet::HTMLColorRule>::assign_with_AddRef(nsHTMLStyleSheet::HTMLColorRule *) [nsAutoPtr.h:fb629ae54510 : 941 + 0x9]
    eip = 0x10495016   esp = 0x0012cd74   ebp = 0x0012cdc8
    Found by: call frame info
 2  xul.dll!mozilla::layers::CairoImageD3D9::SetDevice(IDirect3DDevice9 *) [ImageLayerD3D9.cpp:fb629ae54510 : 592 + 0xa]
    eip = 0x10521e77   esp = 0x0012cd84   ebp = 0x0012cdc8
    Found by: call frame info with scanning
 3  xul.dll!mozilla::layers::ImageLayerD3D9::RenderLayer() [ImageLayerD3D9.cpp:fb629ae54510 : 336 + 0xe]
    eip = 0x10717397   esp = 0x0012cd94   ebp = 0x0012cdc8
    Found by: call frame info with scanning
 4  xul.dll!mozilla::layers::ContainerLayerD3D9::RenderLayer() [ContainerLayerD3D9.cpp:fb629ae54510 : 249 + 0x6]
    eip = 0x105a30d1   esp = 0x0012cdd0   ebp = 0x0012cec8
    Found by: previous frame's frame pointer
 5  xul.dll!mozilla::layers::ContainerLayerD3D9::RenderLayer() [ContainerLayerD3D9.cpp:fb629ae54510 : 249 + 0x6]
    eip = 0x105a30d1   esp = 0x0012ced0   ebp = 0x0012cfc8
    Found by: previous frame's frame pointer
 6  xul.dll!mozilla::layers::LayerManagerD3D9::Render() [LayerManagerD3D9.cpp:fb629ae54510 : 299 + 0xa]
    eip = 0x10664fdf   esp = 0x0012cfd0   ebp = 0x0012d000
    Found by: previous frame's frame pointer
 7  xul.dll!mozilla::layers::LayerManagerD3D9::EndTransaction(void (*)(mozilla::layers::ThebesLayer *,gfxContext *,nsIntRegion const &,nsIntRegion const &,void *),void *) [LayerManagerD3D9.cpp:fb629ae54510 : 156 + 0x6]
    eip = 0x1066519d   esp = 0x0012d008   ebp = 0x0012d058
    Found by: previous frame's frame pointer
 8  xul.dll!nsDisplayList::PaintForFrame(nsDisplayListBuilder *,nsIRenderingContext *,nsIFrame *,unsigned int) [nsDisplayList.cpp:fb629ae54510 : 477 + 0x10]
    eip = 0x10119972   esp = 0x0012d060   ebp = 0x0012d0e8
    Found by: previous frame's frame pointer
 9  xul.dll!nsLayoutUtils::PaintFrame(nsIRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int) [nsLayoutUtils.cpp:fb629ae54510 : 1433 + 0x1d]
    eip = 0x1006ab2a   esp = 0x0012d0f0   ebp = 0x0012d4cc
    Found by: previous frame's frame pointer
10  xul.dll!PresShell::Paint(nsIView *,nsIView *,nsIWidget *,nsRegion const &,nsIntRegion const &,int,int) [nsPresShell.cpp:fb629ae54510 : 6095 + 0xe]
    eip = 0x100321d9   esp = 0x0012d4d4   ebp = 0x0012d540
    Found by: previous frame's frame pointer
11  xul.dll!nsViewManager::RenderViews(nsView *,nsIWidget *,nsRegion const &,nsIntRegion const &,int,int) [nsViewManager.cpp:fb629ae54510 : 447 + 0x20]
    eip = 0x1003231f   esp = 0x0012d510   ebp = 0x0012d5a0
    Found by: call frame info with scanning
12  xul.dll!nsViewManager::Refresh(nsView *,nsIWidget *,nsIntRegion const &,unsigned int) [nsViewManager.cpp:fb629ae54510 : 413 + 0x1c]
    eip = 0x1003243a   esp = 0x0012d5a8   ebp = 0x0000003c
    Found by: previous frame's frame pointer
13  xul.dll!nsViewManager::DispatchEvent(nsGUIEvent *,nsIView *,nsEventStatus *) [nsViewManager.cpp:fb629ae54510 : 912 + 0xe]
    eip = 0x1005e233   esp = 0x0012d618   ebp = 0x0012d688
    Found by: call frame info
14  xul.dll!AttachedHandleEvent [nsView.cpp:fb629ae54510 : 193 + 0x17]
    eip = 0x1003a8a3   esp = 0x0012d690   ebp = 0x0154e4e0
    Found by: previous frame's frame pointer
15  xul.dll!nsWindow::DispatchEvent(nsGUIEvent *,nsEventStatus &) [nsWindow.cpp:fb629ae54510 : 3620 + 0x2]
    eip = 0x10014076   esp = 0x0012d6b0   ebp = 0x0154e4e0
    Found by: call frame info with scanning
16  xul.dll!nsWindow::DispatchWindowEvent(nsGUIEvent *,nsEventStatus &) [nsWindow.cpp:fb629ae54510 : 3648 + 0x12]
    eip = 0x100139e6   esp = 0x0012d6c4   ebp = 0x0154e4e0
    Found by: call frame info with scanning
17  xul.dll!nsWindow::OnPaint(HDC__ *,unsigned int) + 0x1e9
    eip = 0x104678a9   esp = 0x0012d6d8   ebp = 0x0154e4e0
    Found by: call frame info with scanning
18  ntdll.dll + 0xee17
    eip = 0x7c90ee18   esp = 0x0012d704   ebp = 0x0154e4e0
    Found by: stack scanning
19  ntdll.dll + 0x10737
    eip = 0x7c910738   esp = 0x0012d708   ebp = 0x0154e4e0
    Found by: stack scanning
20  ntdll.dll + 0x10731
    eip = 0x7c910732   esp = 0x0012d724   ebp = 0x0154e4e0
    Found by: stack scanning
21  0x6d98ffff
    eip = 0x6d990000   esp = 0x0012d748   ebp = 0x0154e4e0
    Found by: stack scanning

What makes this strange is that it seems we somehow hit a device change. This situation is rare and shouldn't really happen during a Talos run, however, even if it does happen our mitigation strategy seems to be failing.

What's making it really odd is that this is the line 'mTexture = NULL'. And the line that this crashes in nsAutoPtr is oldPtr->Release(). The rest of the crash info suggests oldPtr here is NULL, which is not unexpected if our code didn't create a texture yet. However that call is preceded by if ( oldPtr ) so this line -should- never get hit if oldPtr is NULL. I don't understand yet how this can occur.
(Assignee)

Updated

8 years ago
Summary: New intermittent crash on Talos [@mozilla::layers::CairoImageD3D9::SetDevice(IDirect3DDevice9 *)] → New intermittent crash on Talos [@mozilla::layers::CairoImageD3D9::SetDevice(IDirect3DDevice9 *) | nsRefPtr<IDirect3DTexture9>::operator=(IDirect3DTexture9*) ]
(Assignee)

Updated

8 years ago
Summary: New intermittent crash on Talos [@mozilla::layers::CairoImageD3D9::SetDevice(IDirect3DDevice9 *) | nsRefPtr<IDirect3DTexture9>::operator=(IDirect3DTexture9*) ] → mozi New intermittent crash on Talos[@ nsRefPtr<TypeInState>::assign_assuming_AddRef(TypeInState*) | nsRefPtr<mozilla::net::HttpBaseChannel>::assign_with_AddRef(mozilla::net::HttpBaseChannel*) | nsRefPtr<IDirect3DTexture9>::operator=(IDirect3DTexture9*) |
(Assignee)

Comment 2

8 years ago
I've thought about this some more. One thing that could be happening here is SetCurrentImage is being called on a D3D9 container with an image that was not created with a D3D9 container. I wonder if that's somehow possible.
(Assignee)

Comment 3

8 years ago
Alright, I've discovered STR for this crash by thinking about my theory:

The easiest way to do this is using session restore to get the timings right, we switch Basic to D3D9 layers 5 seconds after first widget initialization.

1) Open one tab with a windowless flash thing that -does not- animate or in any other way invalidate (for example: http://www.pagetraffic.com/guide/flash-sample-popup.html works for me)
2) Open another tab and go to google.com or something
3) Switch to the first tab
4) Save session and quit
5) Start, as soon as you see tab 1 content, switch to tab 2 (within 5 seconds)
6) Switch back to tab 1
7) Kaboom
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1292960373.1292960773.30783.gz
Rev3 WINNT 5.1 mozilla-central talos tp4 on 2010/12/21 11:39:33 
s: talos-r3-xp-016

Thread 0 (crashed)
 0  xul.dll!nsRefPtr<nsComputedDOMStyle>::assign_assuming_AddRef(nsComputedDOMStyle *) [nsAutoPtr.h:a599d358d01a : 957 + 0x0]
    eip = 0x10498eac   esp = 0x0012cc6c   ebp = 0x0012ccd0   ebx = 0x00000001
    esi = 0x00000000   edi = 0x027d8300   eax = 0x027d8324   ecx = 0x00000002
    edx = 0x00000000   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  xul.dll!nsRefPtr<nsDOMWorkerTimeout>::assign_with_AddRef(nsDOMWorkerTimeout *) [nsAutoPtr.h:a599d358d01a : 941 + 0x9]
    eip = 0x104928ad   esp = 0x0012cc70   ebp = 0x0012ccd0
    Found by: call frame info
 2  xul.dll!nsRefPtr<IDirect3DTexture9>::operator=(IDirect3DTexture9 *) [nsAutoPtr.h:a599d358d01a : 1025 + 0xc]
    eip = 0x10358966   esp = 0x0012cc80   ebp = 0x0012ccd0
    Found by: call frame info with scanning
(Assignee)

Updated

8 years ago
Duplicate of this bug: 620742
Keywords: crash, reproducible
(Assignee)

Comment 6

8 years ago
I cannot reproduce this in local optimized or debug builds as far as I can see. This raises the question whether this is perhaps a bug in the VS2005 optimizer(it had quite a few) or a result of some from profile guided optimizations. I've fired a patch that disables optimization on some functions to try, and I'll see how that behaves.
(Assignee)

Comment 7

8 years ago
I can confirm a try server build disabling optimizations in RenderLayer and nsPluginInstanceOwner::SetCurrentImage still crashes. I need to get VS symbols though to get a better look at what's happening there. Local builds still aren't crashing.
(Assignee)

Comment 8

8 years ago
I've finally managed to find out what the problem here is. The problem is that the newly created D3D9 LayerManager gets put at the same spot as the old BasicLayerManager on our Tinderbox builds. What this means is GetContainer()->Manager() == Manager(), but never-the-less the container and the image both aren't D3D9 containers.

I can see two possible solutions:
1. On LayerManager destruction, null the manager on all its containers. This seems like the best solution for the problem.
2. Add a GetType() to ImageContainer and verify that.

I'll write a patch for approach 1.
(Assignee)

Comment 9

8 years ago
Top crasher, should block.
blocking2.0: --- → ?
(Assignee)

Comment 10

8 years ago
Having thought about this some more, I think I'll add option 2. Considering we want Containers to be independent of managers as per bug 615316 this seems like the better solution. Longer run containers should probably be LayerManager ignorant.
(Assignee)

Comment 11

8 years ago
Created attachment 499507 [details] [diff] [review]
Part 1: Add GetBackendType to ImageContainers
Attachment #499507 - Flags: review?(roc)
(Assignee)

Comment 12

8 years ago
Created attachment 499508 [details] [diff] [review]
Part 2: Check backend type matching when rendering ImageLayerD3D*
Attachment #499508 - Flags: review?(roc)
(Assignee)

Comment 13

8 years ago
Created attachment 499509 [details] [diff] [review]
Part 3: Add a minor safety check to ImageContainerD3D9
Attachment #499509 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #499509 - Attachment is patch: true
Attachment #499509 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 14

8 years ago
Created attachment 499510 [details] [diff] [review]
Part 4: Also recreate imagecontainer when backend types don't match

Since we now know that there can be aliasing of different recreated managers of different types, we should also verify the types when deciding if we should recreate.
Attachment #499510 - Flags: review?(roc)
(Assignee)

Comment 15

8 years ago
Patches passed all tryserver tests and the build no longer seems to crash using STR.
If ImageContainer::mManager could be a dangling pointer then we shouldn't have it at all, since we can never reliably use it. So your option 1 would seem to be best if we actually do need mManager.
(Assignee)

Comment 17

8 years ago
(In reply to comment #16)
> If ImageContainer::mManager could be a dangling pointer then we shouldn't have
> it at all, since we can never reliably use it. So your option 1 would seem to
> be best if we actually do need mManager.

Actually, I would propose removing it longer run. I don't think we need an mManager, we don't from D3D perspective (For D3D9 I can move it to passing the device), I just wasn't ready to remove it altogether because of other backends. I propose these patches now to fix it though, without risking other backends or code which might depend on the manager. I'll add a patch to remove usage of mManager altogether for D3D9 and D3D10.
I still think dangling pointers are inherently evil. LayerManagerOGL has code to track the ImageContainers, why not just hoist it to into LayerManager and use it for all layer types?

Although I think mImageContainers should be a hash-set instead of an array.
... and ImageContainerOGL::SetLayerManager needs locking!
For the longer term, I've put my thoughts about how ImageContainers should be managed here:
https://wiki.mozilla.org/Gecko:ImageContainerManagement
(Assignee)

Comment 21

8 years ago
(In reply to comment #18)
> I still think dangling pointers are inherently evil. LayerManagerOGL has code
> to track the ImageContainers, why not just hoist it to into LayerManager and
> use it for all layer types?
> 
> Although I think mImageContainers should be a hash-set instead of an array.

So I have code that does this, but it's just doing useless work for D3D9/D3D10 which I'd really like to avoid, as I said I'd rather remove their usage of mManager altogether. More below.

(In reply to comment #20)
> For the longer term, I've put my thoughts about how ImageContainers should be
> managed here:
> https://wiki.mozilla.org/Gecko:ImageContainerManagement

So, I think I agree with what's generally said there. Couple of remarks:

'When a LayerManager dies, any Images it owns are rescued if necessary/possible so they keep working (unless image data was lost, in which case it's treated as black).' <-- I think as a general policy LayerManagers shouldn't own images. Images might be optimized for a certain context, which might be related to a layermanager. But I think they should exist independently as far as this is possible (it's possible for D3D, not sure about OGL)

'When a LayerManager dies, any ImageFactories it owns should keep working, either working normally or just delegating to the BasicLayers ImageFactory.' <-- I think they should keep on working as well as they good, period, I'm not sure I think anything should rely on BasicLayers implicitly working. As if it were some magical 'catch-all' layer.

'Whenever an ImageLayer is created for a retained layer manager, update the ImageFactory of its ImageContainer to the ImageFactory of the retained layer manager. ' <-- This sounds like a bit magical behavior, and it seems to suggest a lot of inter-dependencies. I'm not sure that makes me happy. I think in a good design this behavior shouldn't be needed.

So for now:

I agree dangling pointers are evil, I believe for now we should just remove the mManager pointer and have no dangling pointers in any situation, and just match backend types. The GetBackendType for ImageContainers I've proposed should do all we need (I can try an OGL patch for this as well).

Bug 615316 really requires them to be independent of layermanagers anyway (hence they need no pointer to the layer manager). In any case for D3D10/D3D9 I'd see no point in clearing those pointers out, simply because we don't really need those pointers anyway, and it seems like poor practice to keep them around and manage them for no reason. Part 3 was probably a poor one because it suggests we do, even though we should do a D3D10 like solution of just passing around the device.

For D3D9/D3D10 the image containers might care about the device, but that changes much less frequently and code to handle that should exist purely on the D3D9/D3D10 implementation side.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1293237555.1293237923.20900.gz
Rev3 WINNT 5.1 mozilla-central talos tp4 on 2010/12/24 16:39:15 
s: talos-r3-xp-016

 0  xul.dll!nsRefPtr<mozilla::WebGLUniformLocation>::assign_assuming_AddRef(mozilla::WebGLUniformLocation *) [nsAutoPtr.h:84745bfb592e : 957 + 0x0]
    eip = 0x104a2bf1   esp = 0x0012cc58   ebp = 0x0012ccb0   ebx = 0x0150c7c0
    esi = 0x00000000   edi = 0x05317500   eax = 0x05317524   ecx = 0x00000001
    edx = 0x00000000   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  xul.dll!nsRefPtr<IDirect3DDevice9>::assign_with_AddRef(IDirect3DDevice9 *) [nsAutoPtr.h:84745bfb592e : 941 + 0x9]
    eip = 0x104a7fa9   esp = 0x0012cc5c   ebp = 0x0012ccb0
    Found by: call frame info
(In reply to comment #21)
> 'When a LayerManager dies, any Images it owns are rescued if necessary/possible
> so they keep working (unless image data was lost, in which case it's treated as
> black).' <-- I think as a general policy LayerManagers shouldn't own images.
> Images might be optimized for a certain context, which might be related to a
> layermanager. But I think they should exist independently as far as this is
> possible (it's possible for D3D, not sure about OGL)

Well, it's not really possible for D3D either since Images are associated with a D3D device; from outside of the layer system, a D3D device dying looks like layer managers dying.

But I agree that we don't want to track image ownership if we don't have to. Actually, what I've proposed wouldn't require D3D to explicitly track image ownership, as long as images keep working as well as possible. I added a sentence to the wiki to clarify that.

> 'When a LayerManager dies, any ImageFactories it owns should keep working,
> either working normally or just delegating to the BasicLayers ImageFactory.'
> <-- I think they should keep on working as well as they good, period, I'm not
> sure I think anything should rely on BasicLayers implicitly working. As if it
> were some magical 'catch-all' layer.

Well, it is and I think it's useful to be able to rely on that instead of having D3D or GL fall back to their own image surfaces and YCbCr conversion code if the GPU is no longer working for some reason.

> 'Whenever an ImageLayer is created for a retained layer manager, update the
> ImageFactory of its ImageContainer to the ImageFactory of the retained layer
> manager. ' <-- This sounds like a bit magical behavior, and it seems to suggest
> a lot of inter-dependencies. I'm not sure that makes me happy. I think in a
> good design this behavior shouldn't be needed.

The alternative is to duplicate the code to reset the current ImageFactory for the ImageContainer in every place we create an ImageLayer --- that's three places so far. I think it's better to do it in just one place.

> I agree dangling pointers are evil, I believe for now we should just remove the
> mManager pointer and have no dangling pointers in any situation, and just match
> backend types. The GetBackendType for ImageContainers I've proposed should do
> all we need (I can try an OGL patch for this as well).

Completely removing the mManager pointers is fine too. Have you got a patch for that?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1293406686.1293407305.28460.gz
Rev3 WINNT 5.1 mozilla-central talos tp4 on 2010/12/26 15:38:06 
s: talos-r3-xp-020

Thread 0 (crashed)
 0  xul.dll!nsRefPtr<nsDOMWorkerXHRProxy>::assign_assuming_AddRef(nsDOMWorkerXHRProxy *) [nsAutoPtr.h:572175c2a64f : 957 + 0x3]
    eip = 0x1048f403   esp = 0x0012cc54   ebp = 0x0012ccb0   ebx = 0x0150c7c0
    esi = 0x00000000   edi = 0x05269f00   eax = 0x12741045   ecx = 0x00610074
    edx = 0x00000000   efl = 0x00010206
    Found by: given as instruction pointer in context
(Assignee)

Comment 25

8 years ago
(In reply to comment #23)
> (In reply to comment #21)
> > 'Whenever an ImageLayer is created for a retained layer manager, update the
> > ImageFactory of its ImageContainer to the ImageFactory of the retained layer
> > manager. ' <-- This sounds like a bit magical behavior, and it seems to suggest
> > a lot of inter-dependencies. I'm not sure that makes me happy. I think in a
> > good design this behavior shouldn't be needed.
> 
> The alternative is to duplicate the code to reset the current ImageFactory for
> the ImageContainer in every place we create an ImageLayer --- that's three
> places so far. I think it's better to do it in just one place.

So, I need to think this through some more, but I wonder if we could make Images layer manager independent, and store 'userdata' on them for different layer managers. I.e. on first use (association with an ImageLayer of a certain kind, or on first rendering of that image layer, whatever the backend prefers) there is user data associated with the image for that layer manager. This wouldn't explicitly make the images 'prefer' any one backend. It would mean that we'd need to keep the source data for the image around, which isn't great. But on the other hand for 'good' behavior we need that anyway, since VRAM might disappear at any time. I'm not sure yet.

> 
> > I agree dangling pointers are evil, I believe for now we should just remove the
> > mManager pointer and have no dangling pointers in any situation, and just match
> > backend types. The GetBackendType for ImageContainers I've proposed should do
> > all we need (I can try an OGL patch for this as well).
> 
> Completely removing the mManager pointers is fine too. Have you got a patch for
> that?

I don't yet, but I will put one up here soon.
(In reply to comment #25)
> (In reply to comment #23)
> > (In reply to comment #21)
> > > 'Whenever an ImageLayer is created for a retained layer manager, update the
> > > ImageFactory of its ImageContainer to the ImageFactory of the retained layer
> > > manager. ' <-- This sounds like a bit magical behavior, and it seems to suggest
> > > a lot of inter-dependencies. I'm not sure that makes me happy. I think in a
> > > good design this behavior shouldn't be needed.
> > 
> > The alternative is to duplicate the code to reset the current ImageFactory for
> > the ImageContainer in every place we create an ImageLayer --- that's three
> > places so far. I think it's better to do it in just one place.

I lied, it's only two places so far.

> So, I need to think this through some more, but I wonder if we could make
> Images layer manager independent, and store 'userdata' on them for different
> layer managers. I.e. on first use (association with an ImageLayer of a certain
> kind, or on first rendering of that image layer, whatever the backend prefers)
> there is user data associated with the image for that layer manager. This
> wouldn't explicitly make the images 'prefer' any one backend. It would mean
> that we'd need to keep the source data for the image around, which isn't great.
> But on the other hand for 'good' behavior we need that anyway, since VRAM might
> disappear at any time. I'm not sure yet.

When an Image is initialized via SetData, we need to do something optimized for a particular backend-type, e.g. upload the image to VRAM in a particular way. Whatever SetData does defines where the data will live and how it can be used; it effectively determines what kind of Image we have, whether the Image implementation class is specific to the backend-type or not. Delegating that backend-specific behavior to somewhere else seems like unnecessary complexity.

What you describe above for Images is more or less what I propose doing for ImageContainers. The current ImageFactory indicates what target we're currently optimizing image creation for. Changing the current ImageFactory means we start optimizing for a different target.

I think it's important to not have to keep the source data for the image around because that is fundamentally a significant performance hit. Video and plugin ImageLayer users can regenerate Image objects if it really matters. That's why I want to leave in the option of a dead image turning black.

Updated

8 years ago
Duplicate of this bug: 621476
(Assignee)

Comment 28

8 years ago
(In reply to comment #26)
> I think it's important to not have to keep the source data for the image around
> because that is fundamentally a significant performance hit. Video and plugin
> ImageLayer users can regenerate Image objects if it really matters. That's why
> I want to leave in the option of a dead image turning black.

Just a memory hit, right? Not a perf hit? Although if we can live with dead images turning black (it's up to the plugin people I guess, that's the only usecase we have right now for an image being black long-term), I'm fine with doing something like user data on containers. Although it will mean it's slightly harder to efficiently share images across backends, but that might never be needed.
(Assignee)

Comment 29

8 years ago
Created attachment 500040 [details] [diff] [review]
Part 5: Do not use mManager on D3D10 layers.
Attachment #500040 - Flags: review?(roc)
(Assignee)

Comment 30

8 years ago
Created attachment 500041 [details] [diff] [review]
Part 5: Do not use mManager on D3D9 layers.
Attachment #500041 - Flags: review?(roc)
(Assignee)

Comment 31

8 years ago
Created attachment 500043 [details] [diff] [review]
Part 7: Do not use mManager on basic layers.

These patches remove mManager usage from all but OGL layers (where the dangling pointers are dealt with already). I didn't feel comfortable enough with the OGL code to see if I could remove it there. But there should be no more dangling pointers with these patches. There's a patch upcoming that prevents us from recreating containers all the time with this patch.
Attachment #500043 - Flags: review?(roc)
(Assignee)

Comment 32

8 years ago
Created attachment 500057 [details] [diff] [review]
Also recreate imagecontainer when backend types don't match v2

Updated to ignore manager pointer when it's NULL.
Attachment #500057 - Flags: review?(roc)
(In reply to comment #28)
> Just a memory hit, right? Not a perf hit?

Making a backup copy of the image is a perf hit.

> Although if we can live with dead
> images turning black (it's up to the plugin people I guess, that's the only
> usecase we have right now for an image being black long-term),

We should be able to easily detect that and ask the plugin to rerender itself.

> I'm fine with
> doing something like user data on containers. Although it will mean it's
> slightly harder to efficiently share images across backends, but that might
> never be needed.

I want to avoid needing that.
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 35

8 years ago
First crash data from the new Nightly seems to suggest this has indeed been fixed.

Comment 36

8 years ago
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+

Comment 37

8 years ago
Moving back to beta 9 due to irc discussion:

"It was our #1 crasher on 
Soccoro about a factor of 10 more prevalent than the #2 crasher. 
So it should probably block Beta9 :) Also, it's already landed so 
that shouldn't be an issue."
blocking2.0: betaN+ → beta9+

Comment 39

4 months ago
Check our sites blog section you might get the answer. 
https://www.udisystem.com
You need to log in before you can comment on or make changes to this bug.