Closed Bug 1433552 Opened 3 years ago Closed 3 years ago

Crash in mozilla::camera::ResolutionFeasibilityDistance

Categories

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

59 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 + fixed
firefox60 + fixed

People

(Reporter: philipp, Assigned: pehrsons)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.
jib, can you have a look at this one?
Flags: needinfo?(jib)
[Tracking Requested - why for this release]: easy crash
Rank: 11
Priority: -- → P2
I'll take this tentatively. If someone has cycles and it's still in NEW, feel free to steal it.
Assignee: nobody → apehrson
Tracking just so we remember to uplift if someone lands a fix and it seems safe for 59.
Status: NEW → ASSIGNED
Flags: needinfo?(jib)
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`).
Attachment #8948314 - Flags: review?(jib) → review-
Comment on attachment 8948314 [details]
Bug 1433552 - Allow zeroes to CamerasParent's FeasibilityDistance functions.

https://reviewboard.mozilla.org/r/217548/#review224854
Attachment #8948314 - Flags: review?(jib) → review+
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.
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/879aa8e4a7ab
Allow zeroes to CamerasParent's FeasibilityDistance functions. r=jib
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
Attachment #8948314 - Flags: approval-mozilla-beta?
(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.
https://hg.mozilla.org/mozilla-central/rev/879aa8e4a7ab
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Attachment #8948314 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.