1216758 - getUserMedia crashes in FF42 (on Windows at least) with NSPR_LOG_MODULES=MediaManager:5

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This bug was filed from the Socorro interface and is 
report bp-f95c3a17-dfb2-4930-914e-c51fc2151020.
=============================================================

This is caused by int64 argument to %ld in a printf in MediaEngineWebRTCVideoSource::NotifyPull. I thought we had analysis for this. Code is in a LOGFRAME macro, which maybe spares it?

Affected code is removed in 43 by Bug 1104616, so this crashes only in 42 (and earlier). Worth fixing before release?

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: change %ld to %lld for int64 printf argument to fix invalid memory access

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

http://mxr.mozilla.org/mozilla-beta/source/dom/media/webrtc/MediaEngineWebRTCVideo.cpp?rev=0a8484884b6b#132

Comment 3

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

Comment on attachment 8676494 [details] [diff] [review]
change %ld to %lld for int64 printf argument to fix invalid memory access

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

Sure, though "... %" PRIi64 "..." would be more canonically correct than %lld.  Either one

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Thanks! I considered that, but mxr suggests we overwhelmingly use lld and llu so I stuck with that.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

PRIi64 required including inttypes.h, so since this is for beta request I'm going with minimal change. I'll use PRIi64 in Bug 1216769.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[Tracking Requested - why for this release]: Turning on logging causes getUserMedia to always crash in debug-builds on Windows.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8676494 [details] [diff] [review]
change %ld to %lld for int64 printf argument to fix invalid memory access

Approval Request Comment
[Feature/regressing bug #]: Not a regression, but potential illegal memory write
[User impact if declined]:
Turning on logging causes getUserMedia to crash in debug-builds on Windows.

[Describe test coverage new/current, TreeHerder]:
Verified locally.

[Risks and why]:
Very low. A replace of "%ld" with "%lld" in logging format-string used solely for logging that is off by default.

[String/UUID change made/needed]: none

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I was wrong, this crashes in opt build as well. In 41 even: bp-3bdc3402-5212-4ad1-9882-c51202151022

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

[User impact if declined]:
Turning on logging causes getUserMedia to crash in both opt and debug builds on Windows.

Comment 10

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> [User impact if declined]:
> Turning on logging causes getUserMedia to crash in both opt and debug builds
> on Windows.

I'll note it's not "turning on logging", it's "turning logging up to VERBOSE" i.e. 5.  4 shouldn't crash.  We have a habit from before erahm's landing of using :5 for logging, but that's now the equivalent of :4.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8676494 [details] [diff] [review]
change %ld to %lld for int64 printf argument to fix invalid memory access

Should be an issue for the release :) Taking it. Should be in 42 beta 9.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/807268f4e3c5