Closed Bug 1216758 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: WebRTC, defect, critical)

42 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox41 --- affected
firefox42 + fixed
firefox43 --- unaffected
firefox44 --- unaffected

People

(Reporter: jib, Assigned: jib)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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?
See Also: → 1216769
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
Attachment #8676494 - Flags: review?(rjesup) → review+
Thanks! I considered that, but mxr suggests we overwhelmingly use lld and llu so I stuck with that.
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.
[Tracking Requested - why for this release]: Turning on logging causes getUserMedia to always crash in debug-builds on Windows.
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
Attachment #8676494 - Flags: approval-mozilla-beta?
I was wrong, this crashes in opt build as well. In 41 even: bp-3bdc3402-5212-4ad1-9882-c51202151022
Summary: getUserMedia crashes in FF42 debug-only (on Windows at least) with NSPR_LOG_MODULES=MediaManager:5 → getUserMedia crashes in FF42 (on Windows at least) with NSPR_LOG_MODULES=MediaManager:5
[User impact if declined]:
Turning on logging causes getUserMedia to crash in both opt and debug builds on Windows.
(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 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.
Attachment #8676494 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/807268f4e3c5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.