Closed Bug 1061475 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: crash, regression)

Attachments

(1 file, 2 obsolete files)

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
Blocks: 1056213
Keywords: crash, regression
Tested on works on Mac debug
Tested on works on Mac debug
Attachment #8482586 - Attachment is obsolete: true
Attachment #8482588 - Flags: review?(gpascutto)
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.
Attachment #8482588 - Flags: review?(gpascutto) → review-
hoists the constants to MediaEngineSource
Attachment #8482588 - Attachment is obsolete: true
Attachment #8482613 - Flags: review?(gpascutto)
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
Attachment #8482613 - Flags: review?(gpascutto) → review+
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
Attachment #8482613 - Flags: approval-mozilla-aurora?
Attachment #8482613 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Target Milestone: mozilla34 → ---
Duplicate of this bug: 1061748
Duplicate of this bug: 1061765
https://hg.mozilla.org/mozilla-central/rev/e567693fa55f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flagging myself to verify this tomorrow.
Flags: qe-verify+
Flags: needinfo?(anthony.s.hughes)
QA Contact: anthony.s.hughes
Looks like I missed my own needinfo -- sorry.

I verify this is fixed now in the current Nightly, Aurora, and Beta.
Status: RESOLVED → VERIFIED
Flags: needinfo?(anthony.s.hughes)
You need to log in before you can comment on or make changes to this bug.