Closed Bug 1085208 Opened 10 years ago Closed 6 years ago

dom/media/tests/identity causes ASAN leaks after being reordered into mochitest-2 suite

Categories

(Core :: WebRTC, defect, P5)

Product:
Core
Component:
WebRTC
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: poiru, Assigned: poiru)

References

Details

Attachments

(2 files, 3 obsolete files)

Move dom/media/tests/ipc into dom/media/tests/mochitest to force it to run with the other Mochitests
2.60 KB, patch
jib
: review+
Details | Diff | Splinter Review
Move dom/media/tests/identity into dom/media/tests/mochitest to force it to run with the other Mochitests
8.57 KB, patch
jib
: review+
Details | Diff | Splinter Review 
Bug 946065 causes dom/media/tests/identity to be moved from mochitest-3 to mochitest-2. This in turn causes the following ASAN leaks:

SUMMARY: AddressSanitizer: 3443 byte(s) leaked in 31 allocation(s).
TEST-UNEXPECTED-FAIL | LeakSanitizer | leak at webrtc::VideoEngine::Create, mozilla::MediaEngineWebRTC::EnumerateVideoDevices, nsTArray, mozilla::GetUserMediaTask::SelectDevice

08:36:19     INFO -  Direct leak of 368 byte(s) in 1 object(s) allocated from:
08:36:19     INFO -      #0 0x4725d1 in operator new(unsigned long) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:53
08:36:19     INFO -      #1 0x7fecc0182d50 in webrtc::VideoEngine::Create() /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_impl.cc:26
08:36:19     INFO -      #2 0x7fecbe7295cc in mozilla::MediaEngineWebRTC::EnumerateVideoDevices(mozilla::MediaSourceType, nsTArray<nsRefPtr<mozilla::MediaEngineVideoSource> >*) /builds/slave/try-l64-asan-00000000000000000/build/dom/media/webrtc/MediaEngineWebRTC.cpp:188
08:36:19     INFO -      #3 0x7fecbe559078 in nsTArray<nsCOMPtr<nsIMediaDevice> >* mozilla::GetSources<mozilla::MediaEngineVideoSource, mozilla::VideoTrackConstraintsN>(mozilla::MediaEngine*, mozilla::VideoTrackConstraintsN&, void (mozilla::MediaEngine::*)(mozilla::MediaSourceType, nsTArray<nsRefPtr<mozilla::MediaEngineVideoSource> >*), char const*) /builds/slave/try-l64-asan-00000000000000000/build/dom/media/MediaManager.cpp:945
08:36:19     INFO -      #4 0x7fecbe56038f in mozilla::GetUserMediaTask::SelectDevice(mozilla::MediaEngine*) /builds/slave/try-l64-asan-00000000000000000/build/dom/media/MediaManager.cpp:1196
08:36:19     INFO -  Indirect leak of 1128 byte(s) in 1 object(s) allocated from:
08:36:19     INFO -      #0 0x4725d1 in operator new(unsigned long) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:53
08:36:19     INFO -      #1 0x7fecc01a0cbd in webrtc::ViESharedData::ViESharedData(webrtc::Config const&) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_shared_data.cc:23
08:36:19     INFO -      #2 0x7fecc012ff16 in webrtc::ViEBaseImpl::ViEBaseImpl(webrtc::Config const&) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_base_impl.cc:65
08:36:19     INFO -      #3 0x7fecc0182e51 in webrtc::VideoEngineImpl::VideoEngineImpl(webrtc::Config const*, bool) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/../../webrtc/video_engine/vie_impl.h:98
08:36:19     INFO -      #4 0x7fecc0182df5 in webrtc::VideoEngine::Create() /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_impl.cc:26
08:36:19     INFO -  Indirect leak of 192 byte(s) in 1 object(s) allocated from:
08:36:19     INFO -      #0 0x4725d1 in operator new(unsigned long) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_new_delete.cc:53
08:36:19     INFO -      #1 0x7fecc01a0c80 in webrtc::ViESharedData::ViESharedData(webrtc::Config const&) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_shared_data.cc:23
08:36:19     INFO -      #2 0x7fecc012ff16 in webrtc::ViEBaseImpl::ViEBaseImpl(webrtc::Config const&) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_base_impl.cc:65
08:36:19     INFO -      #3 0x7fecc0182e51 in webrtc::VideoEngineImpl::VideoEngineImpl(webrtc::Config const*, bool) /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/../../webrtc/video_engine/vie_impl.h:98
08:36:19     INFO -      #4 0x7fecc0182df5 in webrtc::VideoEngine::Create() /builds/slave/try-l64-asan-00000000000000000/build/media/webrtc/trunk/webrtc/video_engine/vie_impl.cc:26

... and many more: https://tbpl.mozilla.org/?rev=d3a4d686fe1c&tree=Try
Without this patch, test_zmedia_cleanup.html is run twice after the tests in dom/media/tests/mochitests. With this patch, test_zmedia_identity_cleanup.html is run after the identity tests.
Attachment #8507652 - Flags: review?(jib)
This "fixes" the ASAN leaks by forcing dom/media/tests/identity to be run in the same suite as the dom/media/tests/mochitest tests: https://tbpl.mozilla.org/?tree=Try&rev=45c419af2929

Since this issue blocks bug 946065 (which I would like to land ASAP), I hope this solution is OK for now.
Attachment #8507653 - Flags: review?(jib)
Comment on attachment 8507652 [details] [diff] [review]
Copy test_zmedia_cleanup.html into dom/media/tests/identity to ensure that it is run after the identity tests

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

::: dom/media/tests/identity/mochitest.ini
@@ +17,5 @@
>  [test_fingerprints.html]
>  
> +# Bug 950317: Hack for making a cleanup hook after finishing all WebRTC cases.
> +# This is a copy of dom/media/tests/mochitest/test_zmedia_cleanup.html.
> +[test_zmedia_identity_cleanup.html]

I would expect hg cp here if we wanted to do this, but...

What does duplicating this file accomplish functionally? Maybe I'm missing something about mochitests, but did "../mochitest/test_zmedia_cleanup.html" not run at the exact same place - the end of the identity test?

(In reply to Birunthan Mohanathas [:poiru] from comment #1)
> Without this patch, test_zmedia_cleanup.html is run twice
> after the tests in dom/media/tests/mochitests. With this patch,
> test_zmedia_identity_cleanup.html is run after the identity tests.

But test_zmedia_cleanup.html and test_zmedia_identity_cleanup.html are identical so what difference does it make?

When both of these patches have landed you'll have:
  dom/media/tests/mochitest/test_zmedia_cleanup.html
  dom/media/tests/mochitest/identity/test_zmedia_identity_cleanup.html

which seems redundant when you could use "../test_zmedia_cleanup.html".
Attachment #8507652 - Flags: review?(jib) → review-
Comment on attachment 8507653 [details] [diff] [review]
Move dom/media/tests/identity into dom/media/tests/mochitest to force it to run with the other Mochitests

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

This patch looks good otherwise (i.e. where it doesn't refer to test_zmedia_identity_cleanup.html).
Attachment #8507653 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> What does duplicating this file accomplish functionally? Maybe I'm missing
> something about mochitests, but did "../mochitest/test_zmedia_cleanup.html"
> not run at the exact same place - the end of the identity test?

No, because Mochitests are apparently chunked into separate suites based on the absolute path.

In the logs of this[0] Try push, you will see that the tests in tests/dom/media/tests/identity were run in the mochitest-2 suite. However, you will also notice that test_zmedia_cleanup.html was not run at all in mochitest-2. If you instead look at the mochitest-3 logs, you will see that test_zmedia_cleanup.html was run twice:

03:53:26     INFO -  116 INFO TEST-OK | /tests/dom/media/tests/mochitest/test_peerConnection_toJSON.html | took 499ms
03:53:26     INFO -  117 INFO TEST-START | /tests/dom/media/tests/mochitest/test_zmedia_cleanup.html
03:53:27     INFO -  118 INFO TEST-OK | /tests/dom/media/tests/mochitest/test_zmedia_cleanup.html | took 455ms
03:53:27     INFO -  119 INFO TEST-START | /tests/dom/media/tests/mochitest/test_zmedia_cleanup.html
03:53:27     INFO -  120 INFO TEST-OK | /tests/dom/media/tests/mochitest/test_zmedia_cleanup.html | took 261ms
03:53:28     INFO -  121 INFO TEST-START | /tests/dom/media/webaudio/test/test_AudioBuffer.html

[0]: https://tbpl.mozilla.org/?tree=Try&rev=d2f96ce20748
Moving the identity tests caused the ipc tests to start intermittently failing with:

Assertion failure: CompositorParent::CompositorLoop() == MessageLoop::current() (Can only call this from the compositor thread!), at c:\builds\moz2_slave\try-w32-d-00000000000000000000\build\gfx\layers\Compositor.cpp:48
2393 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/ipc/test_ipc.html | application terminated with exit code 1 PROCESS-CRASH | /tests/dom/media/tests/ipc/test_ipc.html | application crashed [@ mozilla::layers::Compositor::AssertOnCompositorThread()] 
PROCESS-CRASH | /tests/dom/media/tests/ipc/test_ipc.html | application crashed [@ mozilla::layers::Compositor::AssertOnCompositorThread()]

So we need to move dom/media/tests/ipc into dom/media/tests/mochitest as well in order to keep everything in the same mochitest chunk...
Attachment #8508825 - Flags: review?(jib)
Comment on attachment 8508825 [details] [diff] [review]
Move dom/media/tests/ipc into dom/media/tests/mochitest to force it to run with the other Mochitests

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

::: dom/media/tests/mochitest/ipc/test_ipc.html
@@ +150,5 @@
>        }
>  
>        iframe.addEventListener("mozbrowserloadend", iframeLoadFirst);
>  
> +      // Strip this filename and two directory levels and then add "/mochitest".

Nit: You're in mochitest already, so why not just:

// Strip this filename and one directory level.
Attachment #8508825 - Flags: review?(jib) → review+
(In reply to Birunthan Mohanathas [:poiru] from comment #5)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> > What does duplicating this file accomplish functionally? Maybe I'm missing
> > something about mochitests, but did "../mochitest/test_zmedia_cleanup.html"
> > not run at the exact same place - the end of the identity test?
> 
> No, because Mochitests are apparently chunked into separate suites based on
> the absolute path.
> 
> In the logs of this[0] Try push, you will see that the tests in
> tests/dom/media/tests/identity were run in the mochitest-2 suite. However,
> you will also notice that test_zmedia_cleanup.html was not run at all in
> mochitest-2. If you instead look at the mochitest-3 logs, you will see that
> test_zmedia_cleanup.html was run twice:

Weird. Well in that case, please add hg cp to the patch and I'll r+ it. Since test_zmedia_cleanup.html is a bit of a kludge anyway, we should probably look for a cleaner way to have stuff cleaned up at the end of a test so we don't need to clone test_zmedia_cleanup.html around everywhere. drno may have thoughts.
Flags: needinfo?(drno)
Still though, as I mention at the end of comment 3, wont these files end up in the same unit even without the r- patch?
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> Weird. Well in that case, please add hg cp to the patch and I'll r+ it.

Done.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #10)
> Still though, as I mention at the end of comment 3, wont these files end up
> in the same unit even without the r- patch?

Yes, the patches moving identity and ipc into the mochitest directory should guarantee that everything under dom/media/tests/mochitest is run in the same suite (because we currently chunk directories at most 4 levels deep).

However, I still think the cleanup test should be copied because that is the only way to guarantee execution immediately after the last identity test rather than after everything in dom/media/tests/mochitest.
Attachment #8507653 - Attachment is obsolete: true
Attachment #8508937 - Flags: review?(jib)
Removed an unnecessary hunk.
Attachment #8508937 - Attachment is obsolete: true
Attachment #8508937 - Flags: review?(jib)
Attachment #8508940 - Flags: review?(jib)
(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> However, I still think the cleanup test should be copied because that is the
> only way to guarantee execution immediately after the last identity test
> rather than after everything in dom/media/tests/mochitest.

Well since test_zmedia_cleanup.html merely exists to call getNetworkUtils().tearDownNetwork perhaps it only needs to run at the very end? I'm not sure. Again, drno may know more.
Attachment #8508940 - Flags: review?(jib) → review+
Attachment #8507652 - Attachment is obsolete: true
So test_zmedia_cleanup.html only needs to run at the very end of running any tests which involve peerConnections (so test_peerConnection_* and test_dataChannel_*). But test_zmedia_cleanup.html is really only needed on B2G (it shuts down the WiFi network interface which got activated in the first execute test which uses pc.js), on anything else then the B2G emulator the code in test_zmedia_cleanup.html should do nothing. Sounds like that is not the case here. So I think the better solution would be to investigate why this not working as expected.
If this turns out to be too big of a blocker I'm ok with moving and cloning things around as long as we then have another ticket for investigating why this duplication is needed.
Flags: needinfo?(drno)
In general the whole test_zmedia_cleanup.html is only a hack, because mochitest does not provide a test setup and teardown function.
Two more remarks:
- we landed support for using loopback for ICE, which should allow use to remove that whole network interface vodoo altogether if we enable it for our mochitests
- currently no peerConnection tests get executed on B2G anyway, so we should be able to remove the test_zmedia_cleanup.html without affecting the tests (side effect of removing it should be taken care of as indicated before already)
Depends on: 1089741
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
The leave-open keyword is there and there is no activity for 6 months.
:drno, maybe it's time to close this bug?
Flags: needinfo?(drno)
I think it is okay to close this bug. If anybody wants to work on moving tests around more, and this issue still applies, a new bug can be opened.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(drno)
Keywords: leave-open
Resolution: --- → FIXED
Assignee: nobody → birunthan
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: