1233523 - TestDecodeToSurface.cpp gtest intermittent failure: "Assertion failure: int32_t(mRefCnt) > 0, at dist\include\mozilla/RefCounted.h:117", just after TEST-START | ImageDecodeToSurface.PNG

Status: RESOLVED DUPLICATE of bug 1191347

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: try log

Just encountered this test failure in a Try run -- I'm pretty sure it's intermittent & unrelated to my changes.

Test log:
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-62d1a417639efe0d4782e91c505aae2b44daecb4/try-win32-debug/try-win32-debug-bm79-try1-build1341.txt.gz

Relevant snippet:
{

12:40:47     INFO - TEST-PASS | ImageDecodeToSurface.ImageModuleAvailable | test completed (time: 0ms)
12:40:47     INFO - TEST-START | ImageDecodeToSurface.PNG
12:40:48     INFO - Assertion failure: int32_t(mRefCnt) > 0, at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\obj-firefox\dist\include\mozilla/RefCounted.h:117
12:40:48     INFO - #01: ImageDecodeToSurface_PNG_Test::TestBody (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\image\test\gtest\testdecodetosurface.cpp:98)
12:40:48     INFO - #02: testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test,void> (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:2075)
12:40:48     INFO - #03: testing::Test::Run (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:2162)
12:40:48     INFO - #04: testing::TestCase::Run (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:2444)
12:40:48     INFO - #05: testing::internal::UnitTestImpl::RunAllTests (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:4236)
12:40:48     INFO - #06: testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl,bool> (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:2075)
12:40:48     INFO - #07: testing::UnitTest::Run (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\testing\gtest\gtest\src\gtest.cc:3874)
12:40:48     INFO - #08: XREMain::XRE_mainStartup (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\toolkit\xre\nsapprunner.cpp:3704)
12:40:48     INFO - #09: XREMain::XRE_main (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\toolkit\xre\nsapprunner.cpp:4359)
12:40:48     INFO - #10: XRE_main (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\toolkit\xre\nsapprunner.cpp:4476)
12:40:48     INFO - #11: do_main (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\browser\app\nsbrowserapp.cpp:212)
12:40:48     INFO - #12: NS_internal_main (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\browser\app\nsbrowserapp.cpp:352)
12:40:48     INFO - #13: wmain (c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\src\toolkit\xre\nswindowswmain.cpp:138)
12:40:48     INFO - #14: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255)
12:40:48     INFO - #15: BaseThreadInitThunk[kernel32 +0x1338a]
12:40:48     INFO - #16: RtlInitializeExceptionChain[ntdll +0x397f2]
12:40:48     INFO - #17: RtlInitializeExceptionChain[ntdll +0x397c5]
12:40:51  WARNING - gtest TEST-UNEXPECTED-FAIL | gtest | test failed with return code 1
}

Comment 1

[Daniel Holbert [:dholbert]]

The line pointed to by the test (line 98) is:
98 TEST(ImageDecodeToSurface, PNG) { RunDecodeToSurface(GreenPNGTestCase()); }

I'm assuming that the issue is really happening inside of RunDecodetoSurface(), since that has a RefPtr<SourceSurface>, and SourceSurface derives from RefCounted (and the failing assertion is in RefCounted::Release).

I think I might see the problem -- so, it looks like RunDecodetoSurface() intends to instantiate a SourceSurface, and then decode it off-main-thread via a DecodeToSurfaceRunnable. And presumably the DecodeToSurfaceRunnable should have its own refcounted pointer to the surface. BUT, it does not have its own -- it has a C++ reference to the RefPtr-on-the-stack:
> 35 class DecodeToSurfaceRunnable : public nsRunnable
> 36 {
> 37 public:
> 38   DecodeToSurfaceRunnable(RefPtr<SourceSurface>& aSurface,
> 39                           nsIInputStream* aInputStream,
> 40                           const ImageTestCase& aTestCase)
> 41     : mSurface(aSurface)
[...]
> 70   RefPtr<SourceSurface>& mSurface;


That seems bad, since I think this runnable outlives the stack frame... (it gets dispatched to a helper-thread).

Seth, should this member-var in the runnable be a RefPtr instead of a RefPtr& ?

Comment 2

[Daniel Holbert [:dholbert]]

(Given the use of threads here, I'm assuming that there's a race condition involved with this failure, and that's why this is sporadic & hasn't been noticed/fixed yet.)

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> That seems bad, since I think this runnable outlives the stack frame... (it
> gets dispatched to a helper-thread).

Ah, though we do use DISPATCH_SYNC for the Dispatch call, which means the Dispatch call won't return until the runnable has completed its Run() method.  So the runnable doesn't necessarily outlive the stack frame after all... (not sure how long the thread holds onto it)

So I'm not sure why things are going wrong here after all. Still seems like it might be worthwhile to give mSurface its own dedicated refcounted pointer to the SourceSurface, in any case... (whether that's the local variable's forget()'ed reference, or an additional second reference)

Seth, do you see any clues to what might be broken here? Would you be OK giving DecodeToSurfaceRunnable a dedicated RefPtr instead of a RefPtr&?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Related to bug 1191347?

Comment 5

[Daniel Holbert [:dholbert]]

Ah, yes -- seems like this might be a dupe of that bug.

(The assertion failure looks like the one mentioned in bug 1191347 comment 46 -- "Assertion failure: int32_t(mRefCnt) > 0, at ../../../dist/include/mozilla/RefPtr.h:124" -- but the log is gone so I can't see any more than that.)

I guess I'll dupe this to that bug.