1433552 - Crash in mozilla::camera::ResolutionFeasibilityDistance

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is
report bp-1ae92264-4688-4edc-9fc5-ae17d0180126.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::camera::ResolutionFeasibilityDistance dom/media/systemservices/CamerasParent.cpp:53
1 xul.dll <lambda_a66b284e2d0cc80b0635996a19a881ce>::operator dom/media/systemservices/CamerasParent.cpp:902
2 xul.dll mozilla::camera::VideoEngine::WithEntry dom/media/systemservices/VideoEngine.cpp:220
3 xul.dll <lambda_0c87b2798ca57512eacffea772d09b94>::operator dom/media/systemservices/CamerasParent.cpp:852
4 xul.dll MessageLoop::DoWork ipc/chromium/src/base/message_loop.cc:535
5 xul.dll mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop ipc/glue/MessagePump.cpp:403
6 xul.dll base::MessagePumpWin::Run ipc/chromium/src/base/message_pump_win.h:80
7 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319
8 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:299
9 xul.dll base::Thread::ThreadMain ipc/chromium/src/base/thread.cc:181

=============================================================

this is a DIVIDE_BY_ZERO crash starting in the 59 cycle related to bug 1388667.

Comment 1

[Paul Adenot (:padenot)]

jib, can you have a look at this one?

Comment 2

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

[Tracking Requested - why for this release]: easy crash

Comment 3

[Andreas Pehrson [:pehrsons]]

I'll take this tentatively. If someone has cycles and it's still in NEW, feel free to steal it.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking just so we remember to uplift if someone lands a fix and it seems safe for 59.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

Getting zeroes here is rare, but the numbers come from a platform API so no
guarantees are given for them. This patch makes it as permissive as possible.

Review commit: https://reviewboard.mozilla.org/r/217548/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217548/

Comment 6

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

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

https://reviewboard.mozilla.org/r/217548/#review223626

::: dom/media/systemservices/CamerasParent.cpp:51
(Diff revision 1)
> -  uint32_t distance;
> -  if (candidate >= requested) {
> -    distance = (candidate - requested) * 1000 / std::max(candidate, requested);
> -  } else {
> +  if (candidate == requested) {
> +    // Full match, zero distance. This also catches the corner case where we
> +    // otherwise end up dividing by zero.
> +    return 0;
> -    distance = 10000 + (requested - candidate) *
> -      1000 / std::max(candidate, requested);
>    }
> +
> +  uint32_t distance =
> +    std::abs(candidate - requested) * 1000 / std::max(candidate, requested);

I haven't tested it, but this would appear not to protect against negative values. E.g. in theory if requested is -1 and candidate is 0, then we'd still get DIVIDE_BY_ZERO here.

We should either protect against negative values, or assert against them if they're protected against at a higher level (I could be wrong, but I don't think they are for `requested`).

Comment 7

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217548/diff/1-2/

Comment 8

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

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

https://reviewboard.mozilla.org/r/217548/#review224854

Comment 9

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

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

https://reviewboard.mozilla.org/r/217548/#review223626

> I haven't tested it, but this would appear not to protect against negative values. E.g. in theory if requested is -1 and candidate is 0, then we'd still get DIVIDE_BY_ZERO here.
> 
> We should either protect against negative values, or assert against them if they're protected against at a higher level (I could be wrong, but I don't think they are for `requested`).

Agree that since we're dealing with calculated fitness distances, they should never be negative - even for crazy input - according to the spec algorithm and ours.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/879aa8e4a7ab
Allow zeroes to CamerasParent's FeasibilityDistance functions. r=jib

Comment 11

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1388219
[User impact if declined]: Child process might crash with some camera hardware
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: Unclear how to trigger this, but we should see crashes go down on crash-stats.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This makes an existing function more permissive (returns something instead of crashing) in terms of its input. The existing logic hasn't changed. The input values that caused this crash now return what they did before this regressed.
[String changes made/needed]: None

Comment 12

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] from comment #11)
> Comment on attachment 8948314 [details]
> Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: bug 1388219
> [User impact if declined]: Child process might crash with some camera
> hardware
> [Is this code covered by automated tests?]: No
> [Has the fix been verified in Nightly?]: Not yet
> [Needs manual test from QE? If yes, steps to reproduce]: Unclear how to
> trigger this, but we should see crashes go down on crash-stats.
> [List of other uplifts needed for the feature/fix]: None
> [Is the change risky?]: No
> [Why is the change risky/not risky?]: This makes an existing function more
> permissive (returns something instead of crashing) in terms of its input.
> The existing logic hasn't changed. The input values that caused this crash
> now return what they did before this regressed.
> [String changes made/needed]: None

Sorry, the regressor here is bug 1388667, though bug 1388219 is related.

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/879aa8e4a7ab

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/879aa8e4a7ab

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

Fixes a new crash in 59, let's land this for beta 10.

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/10d1af7903cb