1061475 - Crash in Loop on Mac in getUserMedia - Refresh() is passing null for the uniqueId

Status: VERIFIED FIXED

(Core :: WebRTC, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Regression from bug 1056213 - passing null for uniqueId isn't allowed on Mac (likely is allowed on other platforms, which is why it worked on Linux and Windows for me).

Fix is simple; pass uniqueId

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix crash in Loop calls on Macs due to Refresh() change

Tested on works on Mac debug

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix crash in Loop calls on Macs due to Refresh() change

Tested on works on Mac debug

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8482588 [details] [diff] [review]
fix crash in Loop calls on Macs due to Refresh() change

Review of attachment 8482588 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineWebRTCVideo.cpp
@@ +706,5 @@
>  #ifdef MOZ_B2G_CAMERA
>    // Caller looked up this source by uniqueId; since deviceName == uniqueId nothing else changes
>  #else
>    // Caller looked up this source by uniqueId, so it shouldn't change
> +  const uint32_t kMaxDeviceNameLength = 128;

Just unsigned int according to the API.

@@ +707,5 @@
>    // Caller looked up this source by uniqueId; since deviceName == uniqueId nothing else changes
>  #else
>    // Caller looked up this source by uniqueId, so it shouldn't change
> +  const uint32_t kMaxDeviceNameLength = 128;
> +  const unsigned int kMaxUniqueIdLength = 256;

As opposed to: http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTC.h#223
or maybe http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTCVideo.cpp#658

This screams like it has to go in the API/header proper.

@@ +719,5 @@
>    }
>  
>    CopyUTF8toUTF16(deviceName, mDeviceName);
> +#ifdef DEBUG
> +  nsString uniqueName;

Given that the device also has a name, but this won't contain that, it is a very confusing variable name.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: fix crash in Loop calls on Macs due to Refresh() change

hoists the constants to MediaEngineSource

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8482613 [details] [diff] [review]
fix crash in Loop calls on Macs due to Refresh() change

Review of attachment 8482613 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngine.h
@@ +83,5 @@
>  class MediaEngineSource : public nsISupports
>  {
>  public:
> +  // code inside webrtc.org assumes these sizes; don't use anything smaller
> +  // vithout verifying it's ok

spelling

Also what to do with http://dxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTC.h#223 : it isn't webrtc.org

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e567693fa55f

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8482613 [details] [diff] [review]
fix crash in Loop calls on Macs due to Refresh() change

Approval Request Comment
[Feature/regressing bug #]: Bug 1056213

[User impact if declined]:  Can't uplift bug 1056213, which means windowsharing names will be wrong in 33

[Describe test coverage new/current, TBPL]: Added internal assert.  Will look for additional ways to verify, but depends on multiple users of a screenshare which isn't easy to test in  tbpl

[Risks and why]: low risk, easy-to-manual check regression

[String/UUID change made/needed]: none

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbd4447ae0a5

If the inbound landing doesn't make it to m-c before the uplift, the flags are already set correctly on this bug to get it uplifted to Aurora34 afterwards.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e567693fa55f

Comment 12

[u279076]

Flagging myself to verify this tomorrow.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34d3c9faaacb

Comment 14

[u279076]

Looks like I missed my own needinfo -- sorry.

I verify this is fixed now in the current Nightly, Aurora, and Beta.