Closed Bug 1072313 Opened 10 years ago Closed 8 years ago

Intermittent leakcheck | default process: 40 bytes leaked (MacIOSurface)

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: cbook, Assigned: nical)

References

(
URL
)

Details

(Keywords: intermittent-failure, memory-leak)

Attachments

(4 files, 2 obsolete files)

MozReview Request: Bug 1072313 - Check for more leaks in the layers code. r=nical
40 bytes, text/x-review-board-request
mstange
: review+
Details
KeepUntilDeallocation on the ipdl thread to avoid the race
10.84 KB, patch
mattwoodrow
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
P1. Make MacIOSurface refcount thread-safe.
1.72 KB, patch
mattwoodrow
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
P2 Prevent instanciating MacIOSurfaceLib directly.
2.72 KB, patch
mattwoodrow
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
Rev4 MacOSX Snow Leopard 10.6 fx-team debug test mochitest-1 on 2014-09-24 02:49:57 PDT for push dcd7ac495365

slave: t-snow-r4-0111

https://tbpl.mozilla.org/php/getParsedLog.php?id=48761625&tree=Fx-Team

TEST-UNEXPECTED-FAIL | leakcheck | default process: 40 bytes leaked (MacIOSurface)
only happened once
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
See Also: → 1176366
See Also: → 1167744
See Also: → 1187167
This seems to have spiked recently.
This is getting really bad lately. Any chance you can take a look, BenWa?
Flags: needinfo?(bgirard)
I'll have a look.
Bug 1072313 - Check for more leaks in the layers code. r=nical
Attachment #8640542 - Flags: review?(nical.bugzilla)
Alright this patch is a good starting point to see if we're leaking objects that may be holding this surface.
Comment on attachment 8640542 [details]
MozReview Request: Bug 1072313 - Check for more leaks in the layers code. r=nical

https://reviewboard.mozilla.org/r/14325/#review13201

Ship It!
Attachment #8640542 - Flags: review+
url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/13dd879617e4481e27877d010bcda87f6d906d84
changeset:  13dd879617e4481e27877d010bcda87f6d906d84
user:       Benoit Girard <b56girard@gmail.com>
date:       Wed Jul 29 11:57:03 2015 -0400
description:
Bug 1072313 - Check for more leaks in the layers code. r=mstange
Flags: needinfo?(bgirard)
Keywords: leave-open
This patch doesn't help. Since this is debug only I'm going to capture the stack for every IOSurface allocation and print the one that's left.
Attachment #8640542 - Flags: review?(nical.bugzilla)
I'm not sure how Canvas CaptureStream is involved, but some of my recent (WIP) patches in that area makes this go bananas: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc3e79e780b
Flags: needinfo?(bgirard)
Depends on: 1196430
Yay, we have a stack:

08:37:12     INFO -  Serial Numbers of Leaked Objects:
08:37:12     INFO -  4363 @0x10bc151f0 (1 references; 0 from COMPtrs)
08:37:12     INFO -  allocation stack:
08:37:45     INFO -  #00: mozilla::AppleVDADecoder::OutputFrame(mozilla::CFRefPtr<__CVBuffer*>, mozilla::AppleVDADecoder::AppleFrameRef) [mfbt/nsRefPtr.h:285]
08:37:45     INFO -  #01: nsRunnableMethodImpl<nsresult (mozilla::AppleVDADecoder::*)(mozilla::CFRefPtr<__CVBuffer *>, mozilla::AppleVDADecoder::AppleFrameRef), true, mozilla::CFRefPtr<__CVBuffer *>, mozilla::AppleVDADecoder::AppleFrameRef>::Run [dom/media/platforms/apple/AppleUtils.h:82]
08:37:45     INFO -  #02: mozilla::TaskQueue::Runner::Run() [xpcom/threads/TaskQueue.h:135]
08:37:45     INFO -  #03: nsThreadPool::Run() [xpcom/glue/nsCOMPtr.h:403]
08:37:45     INFO -  #04: _ZThn8_N12nsThreadPool3RunEv [/var/folders/vs/t_n93bks6pd2b2mjk9q8pmdr00000w/T/tmpdywaKJ.cpp:242]
08:37:45     INFO -  #05: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:864]
08:37:45     INFO -  #06: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:277]
08:37:45     INFO -  #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:355]
08:37:45     INFO -  #08: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:235]
08:37:45     INFO -  #09: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:520]
08:37:45     INFO -  #10: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:361]
08:37:46     INFO -  #11: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:215]
08:37:46     INFO -  #12: libSystem.B.dylib + 0x39fd6
08:37:46     INFO -  TEST-INFO | leakcheck | default process: leaked 1 MacIOSurface (40 bytes)

This is what I expected so I'm glad we can confirm it. This leak is caused by video change.
Flags: needinfo?(bgirard)
I'm told :jya is the owner of this code. Can you take a look?
Flags: needinfo?(jyavenard)
It looks like MacIOSurface uses non-threadsafe refcounting, but RefCounted<T> doesn't appear to do any threading assertions (like NS_INLINE_DECL_REFCOUNTING does).

Switching MacIOSurface to use NS_INLINE_DECL_REFCOUNTING might show more useful output, if that is indeed the problem.
Im fairly certain those started to spike after the timestamped image / compositor work
Anthony, this is the #2 overall orange we're currently dealing with (and #1 non-infra). Is there someone who can look into this soon so I don't need to resort to mass-disabling and/or hiding?
Flags: needinfo?(ajones)
Component: General → Audio/Video: Playback
:kaustabh93 has been working to get runbydir going, and this leak shows up in mochitest-2 chunk for dom/media/test in the majority of cases.  It is random but frequent enough that it is blocking us.

If it helps knowing the directory is dom/media/test, then maybe we can fix it faster.

Our only other option is to disable this entire directory on osx until developers have time to investigate the leaks as I have seen needinfo's waiting for a full week here.
hmm, no response from the needinfo's in over a week, I will work on disabling the primary directory where we see these for osx debug.  We will still get coverage on OSX and debug on other platforms.
not sure whom to ask for review, needinfo's produce no response for a full week, so lets stop wasting time of developers and sheriffs and let the folks responsible handle it on their own time as they see fit!
Attachment #8658602 - Flags: review?(philringnalda)
Comment on attachment 8658602 [details] [diff] [review]
disable dom/media/test/* on osx debug only (1.0)

Thank you.
Attachment #8658602 - Flags: review?(philringnalda) → review+
It is sad we have to disable the whole media tests on OSX 10.6.

I also see PCompositorChild and PImageBridgeChild on the leak list. Is it possible that MacIOSurface is held by one of them and therefore being leaked as well?

We should have gfx guys to take a look before deciding to disable the tests.

Hi Jerry,
Can you find a gfx guy to investigate the leak? Thanks.
Flags: needinfo?(hshih)
totally agree.
There's invaluable advantages using debug build.. most of our serious error handling is in the form of MOZ_ASSERT.
Flags: needinfo?(jyavenard)
they will only be disabled until somebody has the time to fix it.  In the meantime these errors are wasting a lot of other developer time.  Ideally this will just be a few days or at most a week.  There needs to be more tests disabled as well, there are more than just the media tests- with try closed all day yesterday I couldn't determine the full impact.
Usually "somebody has the time to fix it" means long long time after. We had experiences that regressions creep in when the tests are disabled. And the later you fix it, the more effort it takes. Is it possible to just increase the leak threshold to keep it green without disabling the whole tests?
if we want to increase the leak threshold that is fine, please get a patch in and lets land it ASAP.  The argument that nobody has time to fix when the tests are disabled is exactly why we are ready to disable all tests causing this problem- nobody has time to fix it with the tests enabled.  It is funny how there is a pending needinfo for over a week, and then when the tests are about to be disabled people respond.  That is more proof that this isn't a high priority issue for anyone to look into.
(In reply to Joel Maher (:jmaher) from comment #905)
> about to be disabled people respond.  That is more proof that this isn't a
> high priority issue for anyone to look into.

that's not quite right.

A leak of 40 or 80 bytes isn't high on anyone's priority list. Which is what this bug is about (and most likely, it's only a shutdown leak ; so totally harmless in practice).

However, the tests are extremely useful.
Ok, here is the plan:
1. I will send a patch to increase the threshold to get rid of the oranges for now.
2. I've ni Jerry who can help investigate if this leak is gfx related.
Btw, anyone knows where to change the threshold value?
mccr8 would know, but I also suspect he'll strongly resist doing so.
It is not acceptable to increase the parent process leak threshold. The problem with increasing the leak threshold is that it increases it for all test suites. It might be possible to hack up something that just ignores a leak of a single MacIOSurface, which would be acceptable to me.
for osx mochitests we could set the default leakthreshold at 40bytes and it would blanket any type of leak- although most leaks are >40bytes.
(In reply to JW Wang [:jwwang] from comment #895)
> I also see PCompositorChild and PImageBridgeChild on the leak list. Is it
> possible that MacIOSurface is held by one of them and therefore being leaked
> as well?
Those two things just always leak in the child process (in every e10s test), because the compositor and image bridge aren't being shut down properly. This leak is in the parent process. So it is probably unrelated.
(In reply to Joel Maher (:jmaher) from comment #905)
> It is funny how there is a pending needinfo for over a week, and then when 
> the tests are about to be disabled people respond.

I don't see the humour. Jean-Yves was away last week and I asked him to look into it when he returned. I didn't remove the needinfo because it hasn't been assigned yet so I wanted to keep it on my radar. This bug will probably take some time to resolve because it seems to fall somewhere between e10s, graphics and media.
If increasing the parent process' leak threshold is unacceptable (comment 919), then we need someone to fix the leak (or disable the test). Is that Jean-Yves?
Well, obviously making the refcounting thread-safe didn't help.
Which isn't surprising considering that a MacIOSurface could be created by the media decoder and once passed to the compositor it doesn't hold a reference to it any longer. As such, refcounting is only ever done on a single thread at once.


(In reply to Andrew McCreight [:mccr8] from comment #919)
> (In reply to JW Wang [:jwwang] from comment #895)
> > I also see PCompositorChild and PImageBridgeChild on the leak list. Is it
> > possible that MacIOSurface is held by one of them and therefore being leaked
> > as well?
> Those two things just always leak in the child process (in every e10s test),
> because the compositor and image bridge aren't being shut down properly.
> This leak is in the parent process. So it is probably unrelated.

I don't believe for a second that a compositor leaking doesn't have a relation with a MacIOSurface also leaking.

The leaks shown here aren't on e10s run ; but plain mochitest-2.

In any case ; it's almost certain that this isn't a media playback bug but a gfx/compositor one.

Is there a way to show where the object was first created?
(In reply to Jean-Yves Avenard [:jya] from comment #993)
> Is there a way to show where the object was first created?

Bug 1196430 comment 35 has a leaking stack and a patch to use for try pushes if you're interested.  For reference here, I'll reproduce the stack:


08:37:12     INFO -  Serial Numbers of Leaked Objects:
08:37:12     INFO -  4363 @0x10bc151f0 (1 references; 0 from COMPtrs)
08:37:12     INFO -  allocation stack:
08:37:45     INFO -  #00: mozilla::AppleVDADecoder::OutputFrame(mozilla::CFRefPtr<__CVBuffer*>, mozilla::AppleVDADecoder::AppleFrameRef) [mfbt/nsRefPtr.h:285]
08:37:45     INFO -  #01: nsRunnableMethodImpl<nsresult (mozilla::AppleVDADecoder::*)(mozilla::CFRefPtr<__CVBuffer *>, mozilla::AppleVDADecoder::AppleFrameRef), true, mozilla::CFRefPtr<__CVBuffer *>, mozilla::AppleVDADecoder::AppleFrameRef>::Run [dom/media/platforms/apple/AppleUtils.h:82]
08:37:45     INFO -  #02: mozilla::TaskQueue::Runner::Run() [xpcom/threads/TaskQueue.h:135]
08:37:45     INFO -  #03: nsThreadPool::Run() [xpcom/glue/nsCOMPtr.h:403]
08:37:45     INFO -  #04: _ZThn8_N12nsThreadPool3RunEv [/var/folders/vs/t_n93bks6pd2b2mjk9q8pmdr00000w/T/tmpdywaKJ.cpp:242]
08:37:45     INFO -  #05: nsThread::ProcessNextEvent(bool, bool*) [xpcom/threads/nsThread.cpp:864]
08:37:45     INFO -  #06: NS_ProcessNextEvent(nsIThread*, bool) [xpcom/glue/nsThreadUtils.cpp:277]
08:37:45     INFO -  #07: mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) [ipc/glue/MessagePump.cpp:355]
08:37:45     INFO -  #08: MessageLoop::RunInternal() [ipc/chromium/src/base/message_loop.cc:235]
08:37:45     INFO -  #09: MessageLoop::Run() [ipc/chromium/src/base/message_loop.cc:520]
08:37:45     INFO -  #10: nsThread::ThreadFunc(void*) [xpcom/threads/nsThread.cpp:361]
08:37:46     INFO -  #11: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:215]
08:37:46     INFO -  #12: libSystem.B.dylib + 0x39fd6
08:37:46     INFO -  TEST-INFO | leakcheck | default process: leaked 1 MacIOSurface (40 bytes)
(In reply to Nathan Froyd [:froydnj] from comment #994)
> (In reply to Jean-Yves Avenard [:jya] from comment #993)
> > Is there a way to show where the object was first created?
> 
> Bug 1196430 comment 35 has a leaking stack and a patch to use for try pushes
> if you're interested.  For reference here, I'll reproduce the stack:
> 

Thanks.

Now what we need is knowing who is the last object holding a reference to it :)

Because as soon as the MacIOSurface is allocated by the Apple VideoToolbox decoder ; it's passed back to the compositor...

here is the last try run with threadsafe refcounting and showing where MacIOSurface were allocated:
https://treeherder.mozilla.org/logviewer.html#?job_id=11360579&repo=try
:jya, I saw the try link. Is the leak also reproducible at mac 10.10?
If so, I will try to reproduce at my laptop first.
(In reply to Jerry Shih[:jerry] (UTC+8) from comment #1004)
> :jya, I saw the try link. Is the leak also reproducible at mac 10.10?
> If so, I will try to reproduce at my laptop first.

It's reproducible on 10.10 try ; I can't reproduce it locally.
Upon close investigation, there's definitely concurrent call happening from time to time between AddRef/Release().

If the mozilla::RefCounted<> base class isn't thread-safe ; it could definitely cause problem.
Makes you wonder about other SourceSurface ; aren't MacIOSurface used in a similar fashion to SourceSurface?

In which case we definitely have a much broader thread-safety issue.
(In reply to Jean-Yves Avenard [:jya] from comment #1094)
> Upon close investigation, there's definitely concurrent call happening from
> time to time between AddRef/Release().
> 
> If the mozilla::RefCounted<> base class isn't thread-safe ; it could
> definitely cause problem.
> Makes you wonder about other SourceSurface ; aren't MacIOSurface used in a
> similar fashion to SourceSurface?
> 
> In which case we definitely have a much broader thread-safety issue.

What happens if you take the tack from comment 1000, but use inline non-threadsafe refcounting from xpcom, rather than threadsafe?  That will tell you who's touching things on the wrong thread, at least.
Flags: needinfo?(jyavenard)
(In reply to Nathan Froyd [:froydnj] from comment #1097)
> (In reply to Jean-Yves Avenard [:jya] from comment #1094)
> > Upon close investigation, there's definitely concurrent call happening from
> > time to time between AddRef/Release().
> > 
> > If the mozilla::RefCounted<> base class isn't thread-safe ; it could
> > definitely cause problem.
> > Makes you wonder about other SourceSurface ; aren't MacIOSurface used in a
> > similar fashion to SourceSurface?
> > 
> > In which case we definitely have a much broader thread-safety issue.
> 
> What happens if you take the tack from comment 1000, but use inline
> non-threadsafe refcounting from xpcom, rather than threadsafe?  That will
> tell you who's touching things on the wrong thread, at least.

We would hit the assert extremely quickly as we create the object on a media task queue which can use different threads.

The non-threadsafe refcount is useless with task queues as if the object gets to run on the same task queue but on a different thread, it will assert.

I opened a bug about this a while back (bug 1159567). So rather than comparing if the exact thread that created the object is the one releasing it ; it would check that we're on the same task queue.

Gerald has wrapped some code that keeps track on where an object was allocated, where it was AddRefed or Released and if when shutting down any have a ref count > 1 will give you the stack trace. Testing it right now.
Flags: needinfo?(jyavenard)
Here is one output:
0. AddRef  0 -> 1
#00: nsRefPtr<MacIOSurface>::AddRefTraits<MacIOSurface>::AddRef(MacIOSurface*) (nsRefPtr.h:364, in XUL)
#00: nsRefPtr<MacIOSurface>::nsRefPtr(MacIOSurface*) (nsRefPtr.h:92, in XUL)
#00: nsRefPtr<MacIOSurface>::nsRefPtr(MacIOSurface*) (nsRefPtr.h:92, in XUL)
#00: mozilla::AppleVDADecoder::OutputFrame(__CVBuffer*, mozilla::AppleVDADecoder::AppleFrameRef) (AppleVDADecoder.cpp:391, in XUL)
#00: mozilla::PlatformCallback(void*, void*, int, unsigned int, __CVBuffer*, CMTime, CMTime) (AppleVTDecoder.cpp:177, in XUL)
#00: 0x0000d35a (in VideoToolbox) + 338
#00: 0x0000d204 (in VideoToolbox) + 158
#00: 0x00097286 (in VideoToolbox) + 440
#00: 0x0008e3cb (in VideoToolbox) + 90
#00: 0x0008e439 (in VideoToolbox) + 86
#00: 0x0009766a (in VideoToolbox) + 139
#00: figThreadMain (in CoreMedia) + 417
#00: _pthread_body (in libsystem_pthread.dylib) + 131
#00: _pthread_body (in libsystem_pthread.dylib) + 0
1. AddRef  1 -> 2
#01: mozilla::RefPtr<MacIOSurface>::ref(MacIOSurface*) (RefPtr.h:314, in XUL)
#01: mozilla::RefPtr<MacIOSurface>::operator=(MacIOSurface*) (RefPtr.h:263, in XUL)
#01: mozilla::layers::MacIOSurfaceImage::SetSurface(MacIOSurface*) (MacIOSurfaceImage.h:20, in XUL)
#01: mozilla::AppleVDADecoder::OutputFrame(__CVBuffer*, mozilla::AppleVDADecoder::AppleFrameRef) (AppleVDADecoder.cpp:398, in XUL)
#01: mozilla::PlatformCallback(void*, void*, int, unsigned int, __CVBuffer*, CMTime, CMTime) (AppleVTDecoder.cpp:177, in XUL)
#01: 0x0000d35a (in VideoToolbox) + 338
#01: 0x0000d204 (in VideoToolbox) + 158
#01: 0x00097286 (in VideoToolbox) + 440
#01: 0x0008e3cb (in VideoToolbox) + 90
#01: 0x0008e439 (in VideoToolbox) + 86
#01: 0x0009766a (in VideoToolbox) + 139
#01: figThreadMain (in CoreMedia) + 417
#01: _pthread_body (in libsystem_pthread.dylib) + 131
#01: _pthread_body (in libsystem_pthread.dylib) + 0
2. Release 2 -> 1
#02: nsRefPtr<MacIOSurface>::AddRefTraits<MacIOSurface>::Release(MacIOSurface*) (nsRefPtr.h:367, in XUL)
#02: nsRefPtr<MacIOSurface>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#02: nsRefPtr<MacIOSurface>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#02: mozilla::AppleVDADecoder::OutputFrame(__CVBuffer*, mozilla::AppleVDADecoder::AppleFrameRef) (AppleVDADecoder.cpp:406, in XUL)
#02: mozilla::PlatformCallback(void*, void*, int, unsigned int, __CVBuffer*, CMTime, CMTime) (AppleVTDecoder.cpp:177, in XUL)
#02: 0x0000d35a (in VideoToolbox) + 338
#02: 0x0000d204 (in VideoToolbox) + 158
#02: 0x00097286 (in VideoToolbox) + 440
#02: 0x0008e3cb (in VideoToolbox) + 90
#02: 0x0008e439 (in VideoToolbox) + 86
#02: 0x0009766a (in VideoToolbox) + 139
#02: figThreadMain (in CoreMedia) + 417
#02: _pthread_body (in libsystem_pthread.dylib) + 131
#02: _pthread_body (in libsystem_pthread.dylib) + 0
3. AddRef  1 -> 2
#03: mozilla::RefPtr<MacIOSurface>::ref(MacIOSurface*) (RefPtr.h:314, in XUL)
#03: mozilla::RefPtr<MacIOSurface>::operator=(MacIOSurface*) (RefPtr.h:263, in XUL)
#03: mozilla::layers::MacIOSurfaceTextureClientOGL::Create(mozilla::layers::ISurfaceAllocator*, mozilla::layers::TextureFlags, MacIOSurface*) (MacIOSurfaceTextureClientOGL.cpp:35, in XUL)
#03: mozilla::layers::MacIOSurfaceImage::GetTextureClient(mozilla::layers::CompositableClient*) (MacIOSurfaceImage.cpp:20, in XUL)
#03: mozilla::layers::ImageClientSingle::UpdateImage(mozilla::layers::ImageContainer*, unsigned int) (ImageClient.cpp:159, in XUL)
#03: mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*) (ImageBridgeChild.cpp:411, in XUL)
#03: void DispatchToFunction<void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> >(void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), Tuple2<mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> > const&) (tuple.h:459, in XUL)
#03: RunnableFunction<void (*)(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*), Tuple2<mozilla::layers::ImageClient*, nsRefPtr<mozilla::layers::ImageContainer> > >::Run() (task.h:434, in XUL)
#03: MessageLoop::RunTask(Task*) (message_loop.cc:365, in XUL)
#03: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:375, in XUL)
#03: MessageLoop::DoWork() (message_loop.cc:459, in XUL)
#03: base::MessagePumpDefault::Run(base::MessagePump::Delegate*) (message_pump_default.cc:34, in XUL)
#03: MessageLoop::RunInternal() (message_loop.cc:235, in XUL)
#03: MessageLoop::RunHandler() (message_loop.cc:228, in XUL)
#03: MessageLoop::Run() (message_loop.cc:201, in XUL)
#03: base::Thread::ThreadMain() (thread.cc:173, in XUL)
#03: ThreadFunc(void*) (platform_thread_posix.cc:39, in XUL)
#03: _pthread_body (in libsystem_pthread.dylib) + 131
#03: _pthread_body (in libsystem_pthread.dylib) + 0
4. AddRef  2 -> 3
#04: mozilla::RefPtr<MacIOSurface>::ref(MacIOSurface*) (RefPtr.h:314, in XUL)
#04: mozilla::RefPtr<MacIOSurface>::RefPtr(MacIOSurface*) (RefPtr.h:239, in XUL)
#04: mozilla::RefPtr<MacIOSurface>::RefPtr(MacIOSurface*) (RefPtr.h:239, in XUL)
#04: mozilla::layers::TKeepAlive<MacIOSurface>::TKeepAlive(MacIOSurface*) (TextureClient.h:737, in XUL)
#04: mozilla::layers::TKeepAlive<MacIOSurface>::TKeepAlive(MacIOSurface*) (TextureClient.h:737, in XUL)
#04: mozilla::detail::UniqueSelector<mozilla::layers::TKeepAlive<MacIOSurface> >::SingleObject mozilla::MakeUnique<mozilla::layers::TKeepAlive<MacIOSurface>, mozilla::RefPtr<MacIOSurface>&>(mozilla::RefPtr<MacIOSurface>&&&) (UniquePtr.h:643, in XUL)
#04: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:21, in XUL)
#04: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:23, in XUL)
#04: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:19, in XUL)
#04: mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureClient>::Release() (AtomicRefCountedWithFinalize.h:152, in XUL)
#04: mozilla::RefPtr<mozilla::layers::TextureClient>::unref(mozilla::layers::TextureClient*) (RefPtr.h:322, in XUL)
#04: mozilla::RefPtr<mozilla::layers::TextureClient>::~RefPtr() (RefPtr.h:244, in XUL)
#04: mozilla::RefPtr<mozilla::layers::TextureClient>::~RefPtr() (RefPtr.h:244, in XUL)
#04: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#04: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#04: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#04: mozilla::layers::Image::Release() (ImageContainer.h:132, in XUL)
#04: nsRefPtr<mozilla::layers::Image>::AddRefTraits<mozilla::layers::Image>::Release(mozilla::layers::Image*) (nsRefPtr.h:367, in XUL)
#04: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#04: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#04: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#04: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#04: mozilla::VideoData::~VideoData() (MediaData.cpp:129, in XUL)
#04: mozilla::MediaData::Release() (MediaData.h:32, in XUL)
5. Release 3 -> 2
#05: mozilla::RefPtr<MacIOSurface>::unref(MacIOSurface*) (RefPtr.h:322, in XUL)
#05: mozilla::RefPtr<MacIOSurface>::~RefPtr() (RefPtr.h:244, in XUL)
#05: mozilla::RefPtr<MacIOSurface>::~RefPtr() (RefPtr.h:244, in XUL)
#05: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:23, in XUL)
#05: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:23, in XUL)
#05: mozilla::layers::MacIOSurfaceTextureClientOGL::~MacIOSurfaceTextureClientOGL() (MacIOSurfaceTextureClientOGL.cpp:19, in XUL)
#05: mozilla::AtomicRefCountedWithFinalize<mozilla::layers::TextureClient>::Release() (AtomicRefCountedWithFinalize.h:152, in XUL)
#05: mozilla::RefPtr<mozilla::layers::TextureClient>::unref(mozilla::layers::TextureClient*) (RefPtr.h:322, in XUL)
#05: mozilla::RefPtr<mozilla::layers::TextureClient>::~RefPtr() (RefPtr.h:244, in XUL)
#05: mozilla::RefPtr<mozilla::layers::TextureClient>::~RefPtr() (RefPtr.h:244, in XUL)
#05: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#05: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#05: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#05: mozilla::layers::Image::Release() (ImageContainer.h:132, in XUL)
#05: nsRefPtr<mozilla::layers::Image>::AddRefTraits<mozilla::layers::Image>::Release(mozilla::layers::Image*) (nsRefPtr.h:367, in XUL)
#05: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#05: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#05: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#05: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#05: mozilla::VideoData::~VideoData() (MediaData.cpp:129, in XUL)
#05: mozilla::MediaData::Release() (MediaData.h:32, in XUL)
#05: nsRefPtr<mozilla::MediaData>::AddRefTraits<mozilla::MediaData>::Release(mozilla::MediaData*) (nsRefPtr.h:367, in XUL)
#05: nsRefPtr<mozilla::MediaData>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#05: nsRefPtr<mozilla::MediaData>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
6. Release 2 -> 1
#06: mozilla::RefPtr<MacIOSurface>::unref(MacIOSurface*) (RefPtr.h:322, in XUL)
#06: mozilla::RefPtr<MacIOSurface>::~RefPtr() (RefPtr.h:244, in XUL)
#06: mozilla::RefPtr<MacIOSurface>::~RefPtr() (RefPtr.h:244, in XUL)
#06: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#06: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#06: mozilla::layers::MacIOSurfaceImage::~MacIOSurfaceImage() (MacIOSurfaceImage.h:18, in XUL)
#06: mozilla::layers::Image::Release() (ImageContainer.h:132, in XUL)
#06: nsRefPtr<mozilla::layers::Image>::AddRefTraits<mozilla::layers::Image>::Release(mozilla::layers::Image*) (nsRefPtr.h:367, in XUL)
#06: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#06: nsRefPtr<mozilla::layers::Image>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#06: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#06: mozilla::VideoData::~VideoData() (MediaData.cpp:130, in XUL)
#06: mozilla::VideoData::~VideoData() (MediaData.cpp:129, in XUL)
#06: mozilla::MediaData::Release() (MediaData.h:32, in XUL)
#06: nsRefPtr<mozilla::MediaData>::AddRefTraits<mozilla::MediaData>::Release(mozilla::MediaData*) (nsRefPtr.h:367, in XUL)
#06: nsRefPtr<mozilla::MediaData>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#06: nsRefPtr<mozilla::MediaData>::~nsRefPtr() (nsRefPtr.h:59, in XUL)
#06: mozilla::detail::ListenerHelper<mozilla::AbstractThread, mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>::value, mozilla::MediaEventListener>::Type mozilla::MediaEventSource<nsRefPtr<mozilla::MediaData>, (mozilla::ListenerMode)1>::ConnectInternal<mozilla::AbstractThread, mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>(mozilla::AbstractThread*, mozilla::MediaDecoderStateMachine*, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&))::'lambda'(nsRefPtr<mozilla::MediaData>&&)>::R<nsRefPtr<mozilla::MediaData> const&>::~R() (MediaEventSource.h:134, in XUL)
#06: mozilla::detail::ListenerHelper<mozilla::AbstractThread, mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>::value, mozilla::MediaEventListener>::Type mozilla::MediaEventSource<nsRefPtr<mozilla::MediaData>, (mozilla::ListenerMode)1>::ConnectInternal<mozilla::AbstractThread, mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>(mozilla::AbstractThread*, mozilla::MediaDecoderStateMachine*, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&))::'lambda'(nsRefPtr<mozilla::MediaData>&&)>::R<nsRefPtr<mozilla::MediaData> const&>::~R() (MediaEventSource.h:134, in XUL)
#06: mozilla::detail::ListenerHelper<mozilla::AbstractThread, mozilla::EnableIf<TakeArgs<void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>::value, mozilla::MediaEventListener>::Type mozilla::MediaEventSource<nsRefPtr<mozilla::MediaData>, (mozilla::ListenerMode)1>::ConnectInternal<mozilla::AbstractThread, mozilla::MediaDecoderStateMachine, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&)>(mozilla::AbstractThread*, mozilla::MediaDecoderStateMachine*, void (mozilla::MediaDecoderStateMachine::*)(nsRefPtr<mozilla::MediaData> const&))::'lambda'(nsRefPtr<mozilla::MediaData>&&)>::R<nsRefPtr<mozilla::MediaData> const&>::~R() (MediaEventSource.h:134, in XUL)
#06: nsRunnable::Release() (nsThreadUtils.cpp:32, in XUL)
#06: nsCOMPtr<nsIRunnable>::~nsCOMPtr() (nsCOMPtr.h:405, in XUL)
#06: nsCOMPtr<nsIRunnable>::~nsCOMPtr() (nsCOMPtr.h:407, in XUL)
#06: nsTArrayElementTraits<nsCOMPtr<nsIRunnable> >::Destruct(nsCOMPtr<nsIRunnable>*) (nsTArray.h:522, in XUL)


So it seems it's:
#04: mozilla::layers::TKeepAlive<MacIOSurface>::TKeepAlive(MacIOSurface*) (TextureClient.h:737, in XUL)
#04: mozilla::detail::UniqueSelector<mozilla::layers::TKeepAlive<MacIOSurface> >::SingleObject mozilla::MakeUnique<mozilla::layers::TKeepAlive<MacIOSurface>, mozilla::RefPtr<MacIOSurface>&>(mozilla::RefPtr<MacIOSurface>&&&) (UniquePtr.h:643, in XUL)

The TKeepAlive destructor is never called.

What's certain however, is that the issue isn't in the media code
Component: Audio/Video: Playback → Graphics: Layers
Flags: needinfo?(matt.woodrow)
(In reply to Jean-Yves Avenard [:jya] from comment #1094)
> If the mozilla::RefCounted<> base class isn't thread-safe ; it could
> definitely cause problem.
> Makes you wonder about other SourceSurface ; aren't MacIOSurface used in a
> similar fashion to SourceSurface?

The various See Alsos on this bug would confirm that I assume?
TKeepAlive is nical's code, he might know more.

That said, the TKeepAlive is stored in a UniquePtr on TextureChild, TextureChild is refcounted (so we'd know if it was leaking), and we never Move() the TKeepAlive out of TextureChild so I don't see how we could be missing the destructor.
Flags: needinfo?(matt.woodrow) → needinfo?(nical.bugzilla)
Keeping the needinfo on so that I keep looking into this, but my reaction so far is the same as Matt's: if the TKeepAlive object was leaked, then the TextureChild must have been leaked too, because the only thing we do with the former is create it and immediately give it to the latter through a UniquePtr (in the case of MacIOSurface).
Does the refcount logging also cover objects with the threadsafe refcounting maccro (I am talking about NS_INLINE_DECL_THREADSAFE_REFCOUNTING)?
(In reply to Nicolas Silva [:nical] from comment #1106)
> Does the refcount logging also cover objects with the threadsafe refcounting
> maccro (I am talking about NS_INLINE_DECL_THREADSAFE_REFCOUNTING)?

Yes.
Note that there are some odd corner cases, like if you create an object but never AddRef it then it won't be reported as a leak.
(In reply to Nicolas Silva [:nical] from comment #1106)
> Keeping the needinfo on so that I keep looking into this, but my reaction so
> far is the same as Matt's: if the TKeepAlive object was leaked, then the
> TextureChild must have been leaked too, because the only thing we do with
> the former is create it and immediately give it to the latter through a
> UniquePtr (in the case of MacIOSurface).
> Does the refcount logging also cover objects with the threadsafe refcounting
> maccro (I am talking about NS_INLINE_DECL_THREADSAFE_REFCOUNTING)?

TextureChild inherit from AtomicRefCountedWithFinalize
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/AtomicRefCountedWithFinalize.h#32

It doesn't appear to make use of the thread safe recounting macro and doesn't report when leaking.
I ran a try build with MOZ_COUNT_CTOR/MOZ_COUNT_DTOR to it.

But my earlier message was implicitly saying just that: TextureClient is leaking (which shows up as MacIOSurface leaking as it's the only object with refcount logging)
You're confusing TextureClient and TextureChild (and who can blame you!).

TextureClient indeed inherits from AtomicRefCountedWithFinalize, but that isn't what is holding the TKeepAlive.

TextureChild uses NS_INLINE_DECL_THREADSAFE_REFCOUNTING.
Oh my bad ; I was thrown off by where the TKeepAlive was created (In MacIOSurfaceTextureClientOGL which inherit TextureClient and the allocation being done in TextureClient.h)

Proof that doing bugzilla in bed on an ipad before coffee is always a bad idea.

But it's still leaking somewhere :)
Well, that proves it:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=750259489f71

Added MOZ_COUNT_CTOR/DTOR to KeepAlive

TEST-UNEXPECTED-FAIL | leakcheck | default process: 88 bytes leaked (KeepAlive, MacIOSurface)

So it's certainly KeepAlive that is leaking.

The only thing that would make sense would be a race on TextureChild::mKeep
The leak can also happens without KeepAlive ; out of 6 runs , the leak was due to KeepAlive not being destroyed only once.

Gerald has some extra information pointing to a race.
In TextureClient::KeepUntilFullDeallocation() the KeepAlive is moved into mActor->mKeep - http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#555

There I added an assertion that should not possibly fail:
> KeepAlive* ptr = aKeep.get();
> mActor->mKeep = Move(aKeep);
> MOZ_ASSERT(mActor->mKeep.get() == ptr);
And it failed!

Debugging there, I could see that aKeep was null, as expected after UniquePtr::operator=() does a release() on it.
And the fact that this KeepAlive doesn't get destroyed shows that it somehow didn't make it into the UniquePtr. Or maybe the UniquePtr was already destroyed by then?

I'll keep debugging, but someone who knows more about TextureClient should probably look into this.
Depends on: 1205541
Depends on: 1205557
Depends on: 1205559
Thanks a lot for the diagnostics, here is what I think is happening (thanks to your finding):

TextureChild::ActorDestroy is running concurrently with TextureClient::KeepUntilFullDeallocation which causes the race on mKeep. The way TextureClient is supposed to work, is that it is *always* the TextureClient that should trigger the TextureChild's destruction (which will eventually lead to ActorDestroy being called). But when shutting down the ImageBridge, we do have code that goes through all remaining TextureClients (which should have been released before but haven't for whatever reason), and force their destruction by calling TextureClient::ForceRemove as a last resort.
This is late in the shutdown. No code using TextureClient should still be holding references to TextureClient objects, especially if it is off the texture's IPDL thread. In retrospect I should have made sure that the shutdown of ImageBridge aggressively crashes with an angry error message when a texture is kept alive while we are shutting down, rather than try to paper over the problem.

This race can be fixed by making sure KeepUntilFullDeallocation is run from the texture's IPDL thread (ImageBridgeChild thread in this case) I'll write a patch for this. Indicentally, the cleanup I am trying to do in bug 1200595 would have fixed that. But we'll probably run into other bugs because if this is happening during shutdown, too many resources have been destroyed for it to be safe to have textures hanging around.
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)

        Attachment #8662334 -
        Flags: review?(matt.woodrow)

    
    

    
      
  

    
    

    
  


  

        Attachment #8662353 -
        Flags: review?(matt.woodrow)

    
    

    
  
Looks like we've had a race on solving that problem :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=998e47bc322b

shows that with my patch above (Make MacIOSurface refcount thread-safe) and the one in bug 1205559 the leaks are gone.

(the try runs shows leak, but that's due to the MacIOSurface refcount tracking, which I've just fixed)..

Whichever way you want to go Nicolas ; we also need the MacIOSurface refcount patch as there were two reasons we had those leaks:

1- The race in TextureChild::ActorDestroy / TextureClient::KeepUntilFullDeallocation
2- The refcount isn't thread-safe.

FWIW, I'm not entirely convinced that running KeepUntilFullDeallocation in the IPC thread will help as we still have TextureChild::ActorDestroy being called ; typically on the main thread.

So regardless of that : you have a race between two threads (be it the IPC one or another)
Flags: needinfo?(nical.bugzilla)
Flags: needinfo?(hshih)
Flags: needinfo?(ajones)

    
    

    
  
Could you add a method to test if we're on the ipdl thread?

Having asserts everywhere ensuring we run on that thread would be helpful.

    
    

    
  
Try run with Gerald's AddRef tracer and the patch of bug 1205559 (+ MacIOSurface threadsafe):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98cc00e496de
no more leak.

Try run with bug 1205559 + MacIOSurface threadsafe:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef98dccf26f5
no more leak.

    
    

    
  
Comment on attachment 8662353 [details] [diff] [review]
Make MacIOSurface refcount thread-safe.

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

::: gfx/2d/MacIOSurface.h
@@ +67,2 @@
>  public:
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MacIOSurface)

We can't use ns headers in gfx/2d since it needs to build standalone (though I don't know why MacIOSurface is in here).

Use mozilla::AtomicRefCounted instead.

        Attachment #8662353 -
        Flags: review?(matt.woodrow)

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #1146)
> Comment on attachment 8662353 [details] [diff] [review]
> Make MacIOSurface refcount thread-safe.
> 
> Review of attachment 8662353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/MacIOSurface.h
> @@ +67,2 @@
> >  public:
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(MacIOSurface)
> 
> We can't use ns headers in gfx/2d since it needs to build standalone (though
> I don't know why MacIOSurface is in here).
> 
> Use mozilla::AtomicRefCounted instead.
I went by that comment
http://mxr.mozilla.org/mozilla-central/source/mfbt/RefPtr.h#199

    
    

    
  
try run with nical's patch (+ MacIOSurface threadsafe refcount)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f33e62b2073b

all green too \o/

    
    

    
      
  

    
    

    
  


  
This class only contains static members.

        Attachment #8662720 -
        Flags: review?(matt.woodrow)

        Attachment #8658602 -
        Attachment is obsolete: true

        Attachment #8662353 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

        Attachment #8662719 -
        Flags: review?(matt.woodrow) → review+

        Attachment #8662720 -
        Flags: review?(matt.woodrow) → review+

    
    

    
  
Comment on attachment 8662334 [details] [diff] [review]
KeepUntilDeallocation on the ipdl thread to avoid the race

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

::: gfx/layers/client/TextureClient.cpp
@@ +557,5 @@
>  }
>  
>  void TextureClient::ForceRemove(bool sync)
>  {
> +  FinalizeOnIPDLThread();

Can't we have textures destroyed without this being called?

Can we add an assert somewhere that'll fire if we destroy a TextureClient without having called it?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Matt Woodrow (:mattwoodrow) from comment #1184)
> Comment on attachment 8662334 [details] [diff] [review]
> KeepUntilDeallocation on the ipdl thread to avoid the race
> 
> Review of attachment 8662334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/TextureClient.cpp
> @@ +557,5 @@
> >  }
> >  
> >  void TextureClient::ForceRemove(bool sync)
> >  {
> > +  FinalizeOnIPDLThread();
> 
> Can't we have textures destroyed without this being called?

Yes, if they were created but destroyed without being connected to an IPDL actor. In this case this won't be called because the Texture is not assigned an ipdl thread, and that's fine since it doesn't have any ipdl resource that would be handled in FinalizeOnIPDLThread.

> 
> Can we add an assert somewhere that'll fire if we destroy a TextureClient
> without having called it?

Not really because of the above case, which we need to support, for instance if the TextureClient is created but we fail to allocate memory for its underlying data.

Is it OK with you if I just add a comment in FinalizeOnIPDLThread that explains that only ipdl related stuff should be handled here because the methof won't run if there is no ipdl stuff attached to the TextureClient?
Flags: needinfo?(nical.bugzilla) → needinfo?(matt.woodrow)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Nicolas Silva [:nical] from comment #1253)
> Is it OK with you if I just add a comment in FinalizeOnIPDLThread that
> explains that only ipdl related stuff should be handled here because the
> methof won't run if there is no ipdl stuff attached to the TextureClient?

By the way, this is temporary. If no emergency gets in the way, I am rewriting the logic around TextureClient's deallocation, and the code that this fixes is going away in the process (not something I can land in the next few days though).

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8662334 [details] [diff] [review]
KeepUntilDeallocation on the ipdl thread to avoid the race

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

Sure, fine with me.

        Attachment #8662334 -
        Flags: review?(matt.woodrow) → review+

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
(In reply to Wes Kocher (:KWierso) from comment #1268)
> Backed out for various crashes like
> https://treeherder.mozilla.org/logviewer.html#?job_id=14437125&repo=mozilla-
> inbound on at least windows
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3b459de0906f

I reported the crashes (and the reason why) in bug 1205559 :(

Also, this bug will *NOT* be closed without the refcount change ; the change to TextureClient alone isn't enough.

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
ForceRemove is called twice in some cases during shutdown (on the right thread at least) which causes the assertion to blow up. Try push with updated patch https://treeherder.mozilla.org/#/jobs?repo=try&revision=685e154723f8

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Shouldn't this get uplifted?
No doubt it's leaking for plain users.

    
    

    
  
(In reply to Jean-Yves Avenard [:jya] from comment #1336)
> Shouldn't this get uplifted?
> No doubt it's leaking for plain users.

nical?
Flags: needinfo?(nical.bugzilla)

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8662334 [details] [diff] [review]
KeepUntilDeallocation on the ipdl thread to avoid the race

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: memory leaks
[Describe test coverage new/current, TreeHerder]: No specific test. Media related tests tend to trigger the issue intermittently.
[Risks and why]: pretty low. It has baked in central for little while without problems.
[String/UUID change made/needed]: none
Flags: needinfo?(nical.bugzilla)

        Attachment #8662334 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8662719 [details] [diff] [review]
P1. Make MacIOSurface refcount thread-safe.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: memory leaks
[Describe test coverage new/current, TreeHerder]: No specific test. Media related tests tend to trigger the issue intermittently.
[Risks and why]: pretty low. It has baked in central for little while without problems.
[String/UUID change made/needed]: none

        Attachment #8662719 -
        Flags: approval-mozilla-aurora?

    
    

    
  
Comment on attachment 8662720 [details] [diff] [review]
P2 Prevent instanciating MacIOSurfaceLib directly.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: memory leaks
[Describe test coverage new/current, TreeHerder]: No specific test. Media related tests tend to trigger the issue intermittently.
[Risks and why]: pretty low. It has baked in central for little while without problems.
[String/UUID change made/needed]: none

        Attachment #8662720 -
        Flags: approval-mozilla-aurora?

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
      
  

    
    

    
  
Comment on attachment 8662719 [details] [diff] [review]
P1. Make MacIOSurface refcount thread-safe.

OK, let's try uplifting this and the other patches to aurora and see how it goes.

        Attachment #8662719 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Comment on attachment 8662720 [details] [diff] [review]
P2 Prevent instanciating MacIOSurfaceLib directly.

Approved for aurora uplift; looks good on nightly so far.

        Attachment #8662720 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Comment on attachment 8662334 [details] [diff] [review]
KeepUntilDeallocation on the ipdl thread to avoid the race

Approved for uplift, avoiding memory leaks sounds good.

        Attachment #8662334 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
      
  
Flags: needinfo?(matt.woodrow)

    
    

    
  
The remaining issues should be fixed with bug 1209039. Feel free to reopen if it fails again.
Status: REOPENED → RESOLVED
Closed: 9 years ago → 8 years ago
Resolution: --- → FIXED

    
    

    
  
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: