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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

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

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.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1515548 - Fix potential divide by zero in DesktopCaptureImpl; r=jib!

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.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60f1310f5d25
Fix potential divide by zero in DesktopCaptureImpl; r=jib

Comment 3

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/60f1310f5d25

Comment 4

[Ryan VanderMeulen [:RyanVM]]

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 5

[Dan Minor [:dminor]]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b41ed6f2639a

Comment 8

[Cornel Ionce [:noni] [Hubs QA]]

(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-.