Crash [@ webrtc::DesktopCaptureImpl::Run ] provoked by {frameRate: {max: 0}} (divide by zero)

RESOLVED FIXED in Firefox 65

Status

()

defect
P2
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: jib, Assigned: dminor)

Tracking

56 Branch
mozilla66
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox64 wontfix, firefox65- fixed, firefox66- fixed)

Details

(crash signature)

Attachments

(1 attachment)

STRs:
 1. Open https://jsfiddle.net/jib1/j21wyx86/ and share screen

Expected result:
  Screen sharing

Actual result:
  Nightly: bp-4aeda29d-331a-44a6-b860-7e7410181220
  Release: bp-49af8557-3334-4920-b373-079bb0181220

Divide by zero:

#if !defined(_WIN32)
  const uint32_t processTime =
      ((uint32_t)(rtc::TimeNanos() - startProcessTime)) /
      rtc::kNumNanosecsPerMillisec;
  // Use at most x% CPU or limit framerate
->const uint32_t maxFPSNeeded = 1000 / _requestedCapability.maxFPS;

Not a regression. Only tested on OSX.

Found while writing wpt for bug 1321221. Hopefully a simple fix.
This clamps requests for FPS that are below 1 to 1. As far as I can tell,
the getDisplayMedia specification does not provide any guidance on how to
interpret negative or zero FPS constraints, so this seems like a reasonable
limitation.

I'm not adding a test here as there will be a wpt test that covers this
as part of Bug 1321221.
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/60f1310f5d25
Fix potential divide by zero in DesktopCaptureImpl; r=jib
https://hg.mozilla.org/mozilla-central/rev/60f1310f5d25
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Given that this is a longstanding issue, I don't think this needs to be tracked. That said, I'll happily entertain a Beta uplift request :)
Comment on attachment 9032705 [details]
Bug 1515548 - Fix potential divide by zero in DesktopCaptureImpl; r=jib!

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: Browser crash with divide by zero if user requests 0 FPS

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low risk because this adds code to handle zero and negative FPS. Positive FPS should be unaffected.

String changes made/needed: None
Flags: needinfo?(dminor)
Attachment #9032705 - Flags: approval-mozilla-beta?
Comment on attachment 9032705 [details]
Bug 1515548 - Fix potential divide by zero in DesktopCaptureImpl; r=jib!

[Triage Comment]
Better edge case handling to reduce WebRTC crashes. Approved for 65.0b8.
Attachment #9032705 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Dan Minor [:dminor] from comment #5)

> Is this code covered by automated tests?: Yes
> Has the fix been verified in Nightly?: Yes
> Needs manual test from QE?: No

Setting this as qe-verify-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.