923468 - Intermittent test_dataChannel_basicVideo.html | application crashed [@ msvcr100.dll + 0x1e2ad]

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28715673&tree=Mozilla-Inbound
Windows XP 32-bit mozilla-inbound opt test mochitest-3 on 2013-10-03 07:57:46 PDT for push de607a40a7fd
slave: t-xp32-ix-116

08:01:24     INFO -  23827 INFO TEST-INFO | /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | Run step: PC_REMOTE_CREATE_ANSWER
08:01:24     INFO -  0[5a91b40]: [CCAPP Task|def] ccapi.c:1243: SIPCC-CC_API: 1/10, send_message_helper: UI -> GSM: CREATEANSWER
08:01:24     INFO -  0[5a91c90]: [GSM Task|def] dcsm.c:532: SIPCC-DCSM: dcsm_process_event: DCSM 19  :(DCSM_READY:CREATEANSWER )
08:01:24     INFO -  0[5a91c90]: [GSM Task|fsm_sm] sm.c:46: SIPCC-FSM: sm_process_event: DEF 10  : 029FA4E0x: sm entry: (HAVE_REMOTE_OFFER:CREATEANSWER)
08:01:24     INFO -  0[5a91c90]: [GSM Task|fsm_sm] fsmdef.c:3337: SIPCC-FSM: fsmdef_ev_createanswer: Entered.
08:01:24     INFO -  0[5a91c90]: [GSM Task|sdp_config] sdp_config.c:105: SDP: Initialized config pointer: 058AF880 (magic=0xABCDABCD)
08:01:24     INFO -  (ice/ERR) ICE(PC:c4f01b72630c9798): All could not pair new trickle candidate
08:01:24     INFO -  0[5a91b40]: [CCAPP Task|def] ccapi.c:1164: SIPCC-CC_API: 1/10, cc_int_feature2: UI -> GSM: FOUNDICECANDIDATE
08:01:24     INFO -  0[5a91b40]: [CCAPP Task|def] ccapi.c:1164: SIPCC-CC_API: 1/10, cc_int_feature2: UI -> GSM: FOUNDICECANDIDATE
08:01:24     INFO -  (ice/ERR) ICE(PC:c4f01b72630c9798): All could not pair new trickle candidate
08:01:24     INFO -  0[5a91b40]: [CCAPP Task|def] ccapi.c:1164: SIPCC-CC_API: 1/10, cc_int_feature2: UI -> GSM: FOUNDICECANDIDATE
08:01:25     INFO -  (ice/ERR) ICE(PC:c4f01b72630c9798): All could not pair new trickle candidate
08:01:25     INFO -  0[5a91b40]: [CCAPP Task|def] ccapi.c:1164: SIPCC-CC_API: 1/10, cc_int_feature2: UI -> GSM: FOUNDICECANDIDATE
08:01:25     INFO -  (ice/ERR) ICE(PC:c4f01b72630c9798): All could not pair new trickle candidate
08:01:25     INFO -  (ice/ERR) ICE(PC:c4f01b72630c9798): All could not pair new trickle candidate
08:01:25     INFO -  INFO | runtests.py | exit 3221225477
08:01:25     INFO -  INFO | runtests.py | Application ran for: 0:00:56.125000
08:01:25     INFO -  INFO | zombiecheck | Reading PID log: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmpu8n66tpidlog
08:01:25     INFO -  ==> process 1956 launched child process 3608 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=1956.103ca4c0.712105845 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 1956 "\\.\pipe\gecko-crash-server-pipe.1956" tab)
08:01:25     INFO -  ==> process 1956 launched child process 1632 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=1956.5aa96a0.642198621 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 1956 "\\.\pipe\gecko-crash-server-pipe.1956" tab)
08:01:25     INFO -  ==> process 1956 launched child process 1120 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=1956.afd8b70.676631259 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 1956 "\\.\pipe\gecko-crash-server-pipe.1956" tab)
08:01:25     INFO -  ==> process 1956 launched child process 2108 ("C:\slave\test\build\application\firefox\plugin-container.exe" --channel=1956.5aa86b0.1284992183 -greomni "C:\slave\test\build\application\firefox\omni.ja" -appomni "C:\slave\test\build\application\firefox\browser\omni.ja" -appdir "C:\slave\test\build\application\firefox\browser" - 1956 "\\.\pipe\gecko-crash-server-pipe.1956" tab)
08:01:25     INFO -  mozcrash INFO | Downloading symbols from: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1380809042/firefox-27.0a1.en-US.win32.crashreporter-symbols.zip
08:01:40  WARNING -  PROCESS-CRASH | /tests/dom/media/tests/mochitest/test_dataChannel_basicVideo.html | application crashed [@ msvcr100.dll + 0x1e2ad]
08:01:40     INFO -  Crash dump filename: c:\docume~1\cltbld~1.t-x\locals~1\temp\tmp6i6_vs\minidumps\750226ef-0c44-4de2-80d2-e5db24b348a5.dmp
08:01:40     INFO -  Operating system: Windows NT
08:01:40     INFO -                    5.1.2600 Service Pack 3
08:01:40     INFO -  CPU: x86
08:01:40     INFO -       GenuineIntel family 6 model 30 stepping 5
08:01:40     INFO -       8 CPUs
08:01:40     INFO -  Crash reason:  EXCEPTION_ACCESS_VIOLATION_WRITE
08:01:40     INFO -  Crash address: 0x453a3643
08:01:40     INFO -  Thread 0 (crashed)
08:01:40     INFO -   0  msvcr100.dll + 0x1e2ad
08:01:40     INFO -      eip = 0x78abe2ad   esp = 0x0012ef10   ebp = 0x0012f198   ebx = 0x02fab919
08:01:40     INFO -      esi = 0x0012ef74   edi = 0x0012f264   eax = 0x00000063   ecx = 0x0012f1bc
08:01:40     INFO -      edx = 0x453a3643   efl = 0x00010216
08:01:40     INFO -      Found by: given as instruction pointer in context
08:01:40     INFO -   1  msvcr100.dll + 0x1e628
08:01:40     INFO -      eip = 0x78abe629   esp = 0x0012f1a0   ebp = 0x0012f1dc
08:01:40     INFO -      Found by: previous frame's frame pointer
08:01:40     INFO -   2  msvcr100.dll + 0x67584
08:01:40     INFO -      eip = 0x78b07585   esp = 0x0012f1e4   ebp = 0x0012f208
08:01:40     INFO -      Found by: previous frame's frame pointer
08:01:40     INFO -   3  msvcr100.dll + 0x675e9
08:01:40     INFO -      eip = 0x78b075ea   esp = 0x0012f210   ebp = 0x0012f228
08:01:40     INFO -      Found by: previous frame's frame pointer
08:01:40     INFO -   4  xul.dll!snprintf [util.c:de607a40a7fd : 759 + 0x14]
08:01:40     INFO -      eip = 0x02b1fe41   esp = 0x0012f230   ebp = 0x0012f250
08:01:40     INFO -      Found by: previous frame's frame pointer
08:01:40     INFO -   5  xul.dll!nr_ice_format_candidate_attribute [ice_candidate.c:de607a40a7fd : 799 + 0x2f]
08:01:40     INFO -      eip = 0x02b13437   esp = 0x0012f258   ebp = 0x0012f2d0
08:01:40     INFO -      Found by: call frame info
08:01:40     INFO -   6  xul.dll!nr_ice_media_stream_get_attributes [ice_media_stream.c:de607a40a7fd : 196 + 0x10]
08:01:40     INFO -      eip = 0x02b16a8a   esp = 0x0012f2d8   ebp = 0x0012f2f8
08:01:40     INFO -      Found by: call frame info
08:01:40     INFO -   7  xul.dll!mozilla::NrIceMediaStream::GetCandidates() [nricemediastream.cpp:de607a40a7fd : 355 + 0x1f]
08:01:40     INFO -      eip = 0x029bac15   esp = 0x0012f300   ebp = 0x0012f3f8
08:01:40     INFO -      Found by: call frame info
08:01:40     INFO -   8  xul.dll!vcmRxAllocICE_m [VcmSIPCCBinding.cpp:de607a40a7fd : 533 + 0xb]
08:01:40     INFO -      eip = 0x029c98f0   esp = 0x0012f400   ebp = 0x0012f484
08:01:40     INFO -      Found by: call frame info

Comment 1

[Kyle Wooten]

Thanks!

Comment 2

[Legacy TBPL/Treeherder Robot]

Gijs
https://tbpl.mozilla.org/php/getParsedLog.php?id=28857987&tree=UX
Windows 7 32-bit ux pgo test mochitest-3 on 2013-10-08 21:13:31
revision: 859ff4a1af98
slave: t-w732-ix-077

PROCESS-CRASH | /tests/dom/media/tests/mochitest/test_peerConnection_basicAudio.html | application crashed [@ msvcr100.dll + 0x1e2ad]
Return code: 1

Comment 3

[Adrian Cruceru]

Attachment: safety.patch

Crash is likely fixed by:
- http://hg.mozilla.org/mozilla-central/diff/994c850a583a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
- This moved "vcmRxAllocICE_m" call from Main thread to STS thread
- Can't look for more details due to permission issues: "You are not authorized to access bug #922068."

Issue looked like a race condition:
- "vcmRxAllocICE_m" was run from main thread. (this reads elements from a queue)
- ICE code runs on STS thread.                (this can alter elements in that queue)

Actual crash:
- ice_media_stream.c - function nr_ice_media_stream_get_attributes.
- This counts how many elements we need to process & allocates space for them.
- After allocation it proceeds to write to each element.
- IF any new element is added to queue with proper state, we'll overflow.

This only happened on release builds where "assert" is disabled.
Patch attached to avoid such crashes in the future.

Questions:
- Can someone test and see if issue is already fixed by commit specified above?
- Can someone review patch and see if it's useful?
- Can bug be closed (either with or without patch I provided)

Comment 4

[Adrian Cruceru]

Attachment: safety.patch

Wtf... Can't add comments (any problem with bugzilla?)... 
To workaround that I've re-attached patch in order to add comment...

Issue with bugzilla: "You tried to change the blocking-b2g field from --- to --- , but only the assignee or reporter of the bug, or a user with the required permissions may change that field. "

Anyway, a few more bugs can be closed with the same details (Too bad I can't comment on anything...):

High probability:
1. https://bugzilla.mozilla.org/show_bug.cgi?id=923687#c2
- Similar stack trace
2. https://bugzilla.mozilla.org/show_bug.cgi?id=903256
- Corruption in malloc - likely due to snprintf corrupting random addresses.
 
Low probability (Could have corrupted something important):
- https://bugzilla.mozilla.org/show_bug.cgi?id=921156
- https://bugzilla.mozilla.org/show_bug.cgi?id=921439
- https://bugzilla.mozilla.org/show_bug.cgi?id=922846
- https://bugzilla.mozilla.org/show_bug.cgi?id=921253
- https://bugzilla.mozilla.org/show_bug.cgi?id=913533

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 832170 [details] [diff] [review]
safety.patch

Forwarding review to abr

Adam/Ekr: take a look at the analysis in the bug, and see if it's reasonable.  A safety check might not be a bad idea here, though if this was cross-thread access as described any number of bad things could happen - if there's a way to add thread-correctness asserts (even debug/test-only) that would be better.


Adrian: thanks again for jumping into some of these unresolved crashes!

Comment 6

[Eric Rescorla (:ekr)]

Adrian,

I agree that this seems like a reasonable safety check. The right error is R_INTERNAL, however.

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 832170 [details] [diff] [review]
safety.patch

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

lgtm with nits.

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +191,5 @@
>          while(cand){
>            if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) {
>              assert(index < attrct);
>  
> +            if (index >= attrct) 

Please fix trailing whitespace

@@ +192,5 @@
>            if (cand->state == NR_ICE_CAND_STATE_INITIALIZED) {
>              assert(index < attrct);
>  
> +            if (index >= attrct) 
> +              ABORT(R_INTERRUPTED);

R_INTERNAL

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Adrian: if you upload a patch with the nits fixed, mark it in keywords as checkin-needed and it will get checked in.  Thanks!

Comment 9

[Adrian Cruceru]

Attachment: safetychange.patch

Thanks for the review feedback & guidance, uploaded patch after review.

Comment 10

[Adrian Cruceru]

Randell - I do not have edit permission so can't add "checkin-needed" on "keywords" field. 

Would be great if anyone reading this can do that.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

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

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Can we nominate this for uplift to affected branches?

Comment 14

[Adrian Cruceru]

Hi Ryan,

Just FYI:

A) Actual fix was done in another submit:
Diff: http://hg.mozilla.org/mozilla-central/diff/994c850a583a/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
Change: This moved "vcmRxAllocICE_m" call from Main thread to STS thread.
Bug: 922068 (I'm not authorized to view it)

B) My change is just a safety check - we just avoid crashing / corrupting data in this place.

So, if you want to uplift to other branches:
- Please check details from (A) and pick up those changes first.
- Pick up my change for added safety.

For actual decision, I'd wait for Randell.

Thanks,
Adrian

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Bug 922068 landed on Fx27, so we're good there already. So I guess the question is whether the safety check is worth uplifting or not. Seems pretty harmless anyway.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

I don't see a need to uplift this safety fix.