Crash in mozilla::camera::ResolutionFeasibilityDistance

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
critical
Rank:
11
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: philipp, Assigned: pehrsons)

Tracking

({crash, regression})

59 Branch
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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
(Assignee)

Comment 3

a year ago
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.
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
Flags: needinfo?(jib)
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
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 9

a year ago
mozreview-review-reply
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

a year ago
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/879aa8e4a7ab
Allow zeroes to CamerasParent's FeasibilityDistance functions. r=jib
(Assignee)

Comment 11

a year ago
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?
(Assignee)

Comment 12

a year ago
(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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/879aa8e4a7ab
Status: ASSIGNED → RESOLVED
Last Resolved: a year 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+
(Assignee)

Updated

a year ago
Depends on: 1439529
You need to log in before you can comment on or make changes to this bug.