Closed Bug 1430856 Opened 6 years ago Closed 6 years ago

Crash in std::_Function_handler<T>::_M_invoke

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Platform:
Unspecified
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- verified
firefox60 --- verified

People

(Reporter: bbouvier, Assigned: pehrsons)

Details

(Keywords: crash)

Crash Data

Attachments

(4 files, 1 obsolete file)

Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.
59 bytes, text/x-review-board-request
jib
: review+
lizzard
: approval-mozilla-beta+
Details
Bug 1430856 - Default to two default capabilities when a camera can handle anything.
59 bytes, text/x-review-board-request
jib
: review+
lizzard
: approval-mozilla-beta+
Details
[beta 59] Cache CaptureCapability in CapabilityCandidate
13.19 KB, patch
Details | Diff | Splinter Review
[beta 59] Default to two default capabilities when a camera can ha ndle anything
4.94 KB, patch
Details | Diff | Splinter Review 
STR:

- go to a room on talky.io/anyroom
- accept sharing cam/mic
- kaboom

mchiang, jib: Paul told me to ni? you.


This bug was filed from the Socorro interface and is
report bp-1a2d89b0-9054-4121-9b21-ffc1e0180116.
=============================================================

Top 10 frames of crashing thread:

0 libxul.so std::_Function_handler<void> >::_M_invoke dom/media/systemservices/CamerasParent.cpp:885
1 libxul.so mozilla::camera::VideoEngine::WithEntry gcc/include/c++/6.4.0/functional:2127
2 libxul.so mozilla::media::LambdaRunnable<mozilla::camera::CamerasParent::RecvStartCapture> >::Run dom/media/systemservices/CamerasParent.cpp:931
3 libxul.so nsThread::ProcessNextEvent 
4 libxul.so NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:517
5 libxul.so mozilla::ipc::MessagePumpForNonMainThreads::Run 
6 libxul.so MessageLoop::Run 
7 libxul.so base::Thread::ThreadMain ipc/chromium/src/base/thread.cc:181
8 libxul.so ThreadFunc ipc/chromium/src/base/platform_thread_posix.cc:38
9 libpthread-2.26.so libpthread-2.26.so@0x77fb 

=============================================================
Flags: needinfo?(mchiang)
Flags: needinfo?(jib)
Assignee: nobody → mchiang
Flags: needinfo?(mchiang)
Could you repro the bug 100%?
Flags: needinfo?(bbouvier)
Rank: 15
Priority: -- → P2
Here you go:

Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 1
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 2
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 3
Munro: RecvGetCaptureCapability: PCI:0000:04:00.0 0 4
Munro: RecvStartCapture: PCI:0000:04:00.0 0
ExceptionHandler::GenerateDump cloned child 4566
ExceptionHandler::SendContinueSignalToChild sent continue signal to child
ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[GFX1-]: Receive IPC close with reason=AbnormalShutdown
[Child 4463, Chrome_ChildThread] WARNING: pipe error (3): Connexion ré-initialisée par le correspondant: file /builds/worker/workspace/build/src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 353
Flags: needinfo?(bbouvier)
I'm also seeing this with 100% reproducibility. This is recent — I'm using Firefox Developer Edition (about:support reports my update channel as "aurora"), and I'm pretty certain this was working within the last couple of weeks.

As an additional data point: I'm using the facetimehd V4L2 module, which is not part of the mainline kernel; its source is at https://github.com/patjak/bcwc_pcie. v4l2-dbg has its capabilities as follows:

[aharvey@aharvey-mbp-linux ~]$ v4l2-dbg -D
Driver info:
	Driver name   : facetimehd
	Card type     : Apple Facetime HD
	Bus info      : PCI:0000:04:00.0
	Driver version: 4.14.13
	Capabilities  : 0x85200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format
		Device Capabilities
I believe Benjamin is also using a Macbook Pro running Linux. Benjamin, can you confirm or infirm this?
Flags: needinfo?(bbouvier)
That's right, and I'm using the same driver.
Flags: needinfo?(bbouvier)
jib, can you find an owner for this one, now that munro is gone?
Andreas, can you take a look at this one?
Assignee: bonchiang → apehrson
Flags: needinfo?(jib)
I wonder if this is related to bug 1434439.
I can. Hopefully in time, I'm pretty swamped atm.
I reproed this by selecting the v4l2loopback device when starting a capture on linux: https://crash-stats.mozilla.com/report/index/22f201c5-8461-479e-b4e2-ee9ac0180207
On your Macbook with Linux on it yeah?
Flags: needinfo?(apehrson)
Yes. Note that the v4l2loopback device hadn't been set up with gstreamer yet, so it was presenting 0 capabilities to us. That's what caused the crash for me.

Benjamin, what does `v4l2-ctl --list-formats-ext` give you for this video-device?
Flags: needinfo?(apehrson) → needinfo?(bbouvier)
Not sure which one is the camera, so posting the entire output of this command:

ioctl: VIDIOC_ENUM_FMT
        Index       : 0
        Type        : Video Capture
        Pixel Format: 'YUYV'
        Name        : YUYV 4:2:2
                Size: Stepwise 320x240 - 1280x720 with step 8/1

        Index       : 1
        Type        : Video Capture
        Pixel Format: 'YVYU'
        Name        : YVYU 4:2:2
                Size: Stepwise 320x240 - 1280x720 with step 8/1
Flags: needinfo?(bbouvier)
I was pretty sure that this working before, so I run mozregression, and found the following changeset that introduced the regression:

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=29cd2a9fe2cc5b3aa06a2d5e20a43bda740cfbaf&tochange=d057ff6cbcfb9ba3ca09dac21c7140292cb0b166

That's bug 1388667, which is in the webrtc component too, so I can imagine it is related.
Flags: needinfo?(jib)
(In reply to Benjamin Bouvier [:bbouvier] from comment #16)
> Not sure which one is the camera, so posting the entire output of this
> command:
> 
> ioctl: VIDIOC_ENUM_FMT
>         Index       : 0
>         Type        : Video Capture
>         Pixel Format: 'YUYV'
>         Name        : YUYV 4:2:2
>                 Size: Stepwise 320x240 - 1280x720 with step 8/1
> 
>         Index       : 1
>         Type        : Video Capture
>         Pixel Format: 'YVYU'
>         Name        : YVYU 4:2:2
>                 Size: Stepwise 320x240 - 1280x720 with step 8/1

Those are both for the same device. Could you run `v4l2-dbg -D -d /dev/videoN` until you find the N that is your camera? Then run `v4l2-ctl --list-formats-ext -d /dev/videoN` with the same N.
Flags: needinfo?(bbouvier)
I identified my decide as /dev/video0, re-run this command with this device and obtained the exact same output.

Note that the crash's backtrace has now changed:

https://crash-stats.mozilla.com/report/index/9f0919cd-b71c-407b-975e-d58aa0180212
https://crash-stats.mozilla.com/report/index/61d567a6-79d2-40d2-bcb9-f41e50180212

At a higher level perspective, I'm seeing that the cam led lights up for a second after I allow cam's usage, then only the tab crashes (it was the entire browser crashing that would crash before).
Flags: needinfo?(bbouvier)
Thanks, it's useful to get that verified.

It's likely the signature changed due to bug 1435670 landing.

I should be able to reproduce this.
Status: NEW → ASSIGNED
Flags: needinfo?(jib)
I can repro this with the vivid linux kernel module per:

This creates one device /dev/video9 with one tv-tuner input (camera input gives only discrete framesizes in capabilities, the tv-tuner gives stepwise framesizes).

> sudo modprobe vivid n_devs=1 node_types=0x1 num_inputs=1 input_types=0x1 num_outputs=0 vid_cap_nr=9
Benjamin, can you try the appropriate build at [1] and let me know how it works for you? Thanks!

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fe33c64c682e60300478c2825e28cc958eab43b
Flags: needinfo?(bbouvier)
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review225748


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:643
(Diff revision 1)
>  
> +template<typename T>
>  /* static */ void
> -MediaEngineRemoteVideoSource::TrimLessFitCandidates(nsTArray<CapabilityCandidate>& set)
> +MediaEngineRemoteVideoSource::TrimLessFitCandidates(
> +    nsTArray<T>& aSet,
> +    std::function<uint32_t(size_t)> aDistanceGetter)

Warning: The parameter 'adistancegetter' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Yes, it does fix the issue, thank you!
Flags: needinfo?(bbouvier)
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review226018


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:643
(Diff revision 1)
>  
> +template<typename T>
>  /* static */ void
> -MediaEngineRemoteVideoSource::TrimLessFitCandidates(nsTArray<CapabilityCandidate>& set)
> +MediaEngineRemoteVideoSource::TrimLessFitCandidates(
> +    nsTArray<T>& aSet,
> +    std::function<uint32_t(size_t)> aDistanceGetter)

Warning: The parameter 'adistancegetter' is copied for each invocation but only used as a const reference; consider making it a const reference [clang-tidy: performance-unnecessary-value-param]
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review226828

Works, but I think this spends a lot of complexity to preserve CapabilityCandidate, which I don't think is needed.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:672
(Diff revision 2)
> +template<>
> +/* static */ void
> +MediaEngineRemoteVideoSource::TrimLessFitCandidates(
> +    nsTArray<Pair<CapabilityCandidate, webrtc::CaptureCapability>>& aSet)

Pair<CapabilityCandidate, webrtc::CaptureCapability> looks like an elaborate piggyback onto CapabilityCandidate. Since CapabilityCandidate is already pretty much an object instantiation of an index dedicated to these functions, instead why not extend it to hold webrtc::CaptureCapability? https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/dom/media/webrtc/MediaEngineRemoteVideoSource.h#71-77
Attachment #8950686 - Flags: review?(jib) → review-
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review226882


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:72
(Diff revision 3)
>                                       public camera::FrameRelay
>  {
>    ~MediaEngineRemoteVideoSource() = default;
>  
>    struct CapabilityCandidate {
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> +    CapabilityCandidate(uint8_t index,

Error: Bad implicit conversion constructor for 'capabilitycandidate' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review227084

r- for not initializing cache. Also, this opens up more simplifications.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:72
(Diff revision 3)
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> -    : mIndex(index), mDistance(distance) {}
> +    CapabilityCandidate(uint8_t index,
> +                        uint32_t distance = 0,
> +                        webrtc::CaptureCapability&& aCapability = webrtc::CaptureCapability())

A default arg here I think is error-prone. In fact, it looks like mCapability is never set anywhere now?

Maybe put direction last and make aCapability required?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:75
(Diff revision 3)
>  
>    struct CapabilityCandidate {
> -    explicit CapabilityCandidate(uint8_t index, uint32_t distance = 0)
> -    : mIndex(index), mDistance(distance) {}
> +    CapabilityCandidate(uint8_t index,
> +                        uint32_t distance = 0,
> +                        webrtc::CaptureCapability&& aCapability = webrtc::CaptureCapability())
> +    : mIndex(index)

Do we still need mIndex?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:81
(Diff revision 3)
> +    , mDistance(distance)
> +    , mCapability(Forward<webrtc::CaptureCapability>(aCapability)) {}
>  
>      size_t mIndex;
>      uint32_t mDistance;
> +    webrtc::CaptureCapability mCapability;

maybe const?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:667
(Diff revision 3)
>    nsTArray<CapabilityCandidate> candidateSet;
>    for (size_t i = 0; i < num; i++) {
>      candidateSet.AppendElement(i);

Need to initialize mCapability here.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:676
(Diff revision 3)
>        webrtc::CaptureCapability cap;
>        GetCapability(candidate.mIndex, cap);

candidate.mCapability

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:819
(Diff revision 3)
>    for (size_t i = 0; i < num; i++) {
>      candidateSet.AppendElement(i);

Need to initialize mCapability here.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:827
(Diff revision 3)
>  
>    // First, filter capabilities by required constraints (min, max, exact).
>  
>    for (size_t i = 0; i < candidateSet.Length();) {
>      auto& candidate = candidateSet[i];
>      webrtc::CaptureCapability cap;

now unused

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:849
(Diff revision 3)
>        webrtc::CaptureCapability cap;
>        GetCapability(candidate.mIndex, cap);

candidate.mCapability

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:892
(Diff revision 3)
>      webrtc::CaptureCapability cap;
>      GetCapability(candidate.mIndex, cap);

candidate.mCapability

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:903
(Diff revision 3)
>        found = true;
>        break;
>      }
>    }
>    if (!found) {
>      GetCapability(candidateSet[0].mIndex, aCapability);

candidate.mCapability
Attachment #8950686 - Flags: review?(jib) → review-
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review227096

Oh nm, I see the initialization in the follow-on patches.
Attachment #8950686 - Flags: review- → review+
Comment on attachment 8950687 [details]
Bug 1430856 - Only GetCapability() once per capability during ChooseCapability.

https://reviewboard.mozilla.org/r/219940/#review226888

r- for NumCapabilities() ipc call inside for-loop

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp
(Diff revision 3)
> -  size_t num = NumCapabilities();
> -
>    nsTArray<CapabilityCandidate> candidateSet;
> -  for (size_t i = 0; i < num; i++) {
> +  for (size_t i = 0; i < NumCapabilities(); i++) {

This calls NumCapabilities() inside the for-loop which isn't cheap! It's an IPC call too.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:846
(Diff revision 3)
>    // Filter further with all advanced constraints (that don't overconstrain).
>  
>    for (const auto &cs : aConstraints.mAdvanced) {
>      nsTArray<CapabilityCandidate> rejects;
>      for (size_t i = 0; i < candidateSet.Length();) {
> -      auto& candidate = candidateSet[i];
> +      const webrtc::CaptureCapability& cap = candidateSet[i].mCapability;

Nit: Make mCapability const instead and use directly.
Attachment #8950687 - Flags: review?(jib) → review-
Comment on attachment 8950688 [details]
Bug 1430856 - Default to two default capabilities when a camera can handle anything.

https://reviewboard.mozilla.org/r/219942/#review226816

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:836
(Diff revision 4)
> +  if (!mHardcodedCapabilities.IsEmpty() &&
> +      mMediaSource == MediaSourceEnum::Camera) {
> +    // We have a hardcoded capability, which means this camera didn't report
> +    // discrete capabilities. It might still allow a ranged capability, so we
> +    // add a couple of default candidates based on prefs and constraints.
> +    // The chosen candidate will be propagated to StartCapture() which will fail
> +    // for an invalid candidate.
> +    MOZ_DIAGNOSTIC_ASSERT(mHardcodedCapabilities.Length() == 1);
> +    MOZ_DIAGNOSTIC_ASSERT(candidateSet.Length() == 1);
> +    candidateSet.Clear();

This is a bit of a hack, but I suppose we want a minimal patch?

Do we have a camera like this to test with? I suppose applyConstraints() on such a camera would already fail StartCapture() today with invalid values, so I guess this is no worse.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:855
(Diff revision 4)
> +      cap.width = c.mWidth.Get(prefWidth);
> +      cap.height = c.mHeight.Get(prefHeight);
> +      cap.maxFPS = c.mFrameRate.Get(aPrefs.mFPS);

Inventing capabilities from constraints... Note JS may pass in all kinds of wild values including -100, 999999. Maybe add some sanity limits?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:874
(Diff revision 4)
> +    size_t index(0);
> +    for (webrtc::CaptureCapability& cap : caps) {
> +      uint32_t distance = GetDistance(cap, aConstraints, aDeviceId, aCalculate);
> +      LogCapability("Hardcoded capability", cap, distance);
> +      if (distance != UINT32_MAX) {
> +        candidateSet.AppendElement(CapabilityCandidate(index++, distance, Move(cap)));

Nit: If we're going to check distance against UINT32_MAX here, why not move all code in this patch ahead of the required constraints for-loop?

That should simplify, as it removes the need for calling GetDistance() and testing against distance again here.
Comment on attachment 8950688 [details]
Bug 1430856 - Default to two default capabilities when a camera can handle anything.

https://reviewboard.mozilla.org/r/219942/#review227100
Attachment #8950688 - Flags: review?(jib) → review+
Comment on attachment 8950687 [details]
Bug 1430856 - Only GetCapability() once per capability during ChooseCapability.

https://reviewboard.mozilla.org/r/219940/#review226888

> Nit: Make mCapability const instead and use directly.

Hmm, no, this whole code is written with the assumption that candidate members are mutable. I'm not changing that.
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

https://reviewboard.mozilla.org/r/219938/#review227084

> A default arg here I think is error-prone. In fact, it looks like mCapability is never set anywhere now?
> 
> Maybe put direction last and make aCapability required?

My use of hg absorb probably put it in the wrong patch. Still getting used to that command.

> Do we still need mIndex?

You're right, we don't. We need it only to get the capability from the parent. When the capability is chosen it gets serialized and sent directly over

> candidate.mCapability

Well, since mCapability is now const, GetCapability can't assign directly to it.

I'll make GetCapability return the CaptureCapability instead of using an out argument.
Comment on attachment 8950687 [details]
Bug 1430856 - Only GetCapability() once per capability during ChooseCapability.

https://reviewboard.mozilla.org/r/219940/#review226888

> Hmm, no, this whole code is written with the assumption that candidate members are mutable. I'm not changing that.

Nm, I rewrote this and it wasn't too hard. Since things ended up in the wrong patch when amending earlier I'll just squash this and the previous patch for simplicity.
Comment on attachment 8950688 [details]
Bug 1430856 - Default to two default capabilities when a camera can handle anything.

https://reviewboard.mozilla.org/r/219942/#review226816

> This is a bit of a hack, but I suppose we want a minimal patch?
> 
> Do we have a camera like this to test with? I suppose applyConstraints() on such a camera would already fail StartCapture() today with invalid values, so I guess this is no worse.

I don't have a camera to test with, but I have the linux vivid driver (comment 21). :bbouvier and :achronop both have cameras to test with.

> Inventing capabilities from constraints... Note JS may pass in all kinds of wild values including -100, 999999. Maybe add some sanity limits?

Ok, I'll make it these ranges: w=[0,7680]; h=[0,4320], fps=[0,480].
Attachment #8950687 - Attachment is obsolete: true
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/445276274143
Cache CaptureCapability in CapabilityCandidate. r=jib
https://hg.mozilla.org/integration/autoland/rev/12b8e4ad2fdc
Default to two default capabilities when a camera can handle anything. r=jib
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

This approval request applies to both patches.

Approval Request Comment
[Feature/Bug causing the regression]: most likely bug 1388219
[User impact if declined]: Crashes when trying to use some cameras. Confirmed for some macbook cameras on linux but impossible to know for others as it is dependent on the driver.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No, but locally before pushing.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, QA can try on linux per comment 0 following comment 21 as a prerequisite. We should also ask :bbouvier or :achronop to verify with their camera/driver per comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: The first patch is very mechanical and doesn't change any logic. It does improve the number of IPC calls we do but that is just a nice side effect. Before the patch many of the IPC calls were duplicates and are now avoided.
The second patch is simpler from a regression standpoint as it only kicks in when we would have crashed.
[String changes made/needed]: None
Attachment #8950686 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/445276274143
https://hg.mozilla.org/mozilla-central/rev/12b8e4ad2fdc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment on attachment 8950686 [details]
Bug 1430856 - Cache CaptureCapability in CapabilityCandidate.

Let's not ship this new regression in 59. Uplift for beta 12.
Attachment #8950686 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8950688 - Flags: approval-mozilla-beta+
The patches fail to apply on beta. Please provide new ones for beta. Thank you.
Flags: needinfo?(apehrson)
Flags: qe-verify+
I have reproduced this bug using an affected Nightly build from 2018-01-17 on Ubuntu 16.04 x64 (used STR from comment 0 and followed comment 21 for prerequisites).
- https://crash-stats.mozilla.com/report/index/a57a3526-7ce5-4560-a898-e6d880180223

I can confirm that this issue is not reproducible anymore on latest Nigthly 60.0a1 (2018-02-23) and Beta 59.0b12 (20180222170353) under Ubuntu 16.04 x64/x86.

Just to make sure that this crash is also fixed on your side - :bbouvier, :achronop, could you please help us verify this bug?
Flags: needinfo?(bbouvier)
Flags: needinfo?(achronop)
I am on latest Nightly and I do not crash using the internal camera. I tested in talky.oi, appear.in and test_gun page. I consider that as fixed.
Flags: needinfo?(achronop)
Thank you, Alex!
Marking this bug as verified fixed per comment 60 and comment 59.
Status: RESOLVED → VERIFIED
status-firefox59: fixed → verified
status-firefox60: fixed → verified
Flags: qe-verify+
(confirmed verified too on my machine)
Flags: needinfo?(bbouvier)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: