Closed Bug 1320170 Opened 3 years ago Closed 3 years ago

Screensharing previews are broken in current nightlies

Categories

(Core :: WebRTC: Audio/Video, defect, P1, major)

53 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla54
Iteration:
54.1 - Feb 6
Tracking Status
firefox52 --- unaffected
firefox53 + verified
firefox54 --- verified

People

(Reporter: florian, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(1 file)

The getUserMedia call made by webrtcUI.jsm fails and calls the error callback.

Steps to reproduce:
1. Load https://mozilla.github.io/webrtc-landing/gum_test.html
2. Click "Screen"
3. In the door hanger, select a screen instead of "No Screen"

Expected result: a preview of the screen should appear

Actual result: no preview, but clicking the "Allow" button works, and the page receives a stream as expected.

mozregression says https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=39efc7bfb839484ef121a2f275481770906d9021&tochange=5230f39d8f21270414aea3cdfb97914fc6acf4c1 -> bug 1319045

One thing I can't explain is that the preview works fine in the browser-chrome mochitest I'm writing to cover the screensharing UI. I would like us to have test coverage for this.
I tested this issue. with and without my patch, the origin, as a string, is the same.
Wondering how my patch is connected with this issue. Unfortunately I'm not familiar enough with this code.
Rank: 14
Priority: -- → P1
[Tracking Requested - why for this release]: We can't ship with this regression on 53 after adding the screensharing previews and removing the screensharing whitelist in 52.
Tracking 53+ for the reason in Comment 2.
Duplicate of this bug: 1331609
See Also: → 1329496
I plan to look at this today
Flags: needinfo?(rjesup)
Assignee: nobody → amarchesini
Attached patch media.patchSplinter Review
Attachment #8831220 - Flags: review?(rjesup)
Attachment #8831220 - Flags: review?(ehsan)
Comment on attachment 8831220 [details] [diff] [review]
media.patch

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

r+ with the assumption that Assign() should be Append().  Thanks!!

::: dom/media/systemservices/MediaChild.h
@@ +15,5 @@
>  namespace mozilla {
> +
> +namespace ipc {
> +class PrincipalInfo;
> +} // ipc namespace

The comment seems unnecessary.  Or you could do "class ipc::PrincipalInfo;"

::: dom/media/systemservices/MediaParent.cpp
@@ +148,5 @@
> +
> +            aString.Append(str);
> +          }
> +
> +          aString.Assign("]]");

Shouldn't this be aString.Append()?
Attachment #8831220 - Flags: review?(rjesup) → review+
Flags: needinfo?(rjesup)
Andrea, which part of this patch would you like me to review?  I don't know this media code and I couldn't figure out what the problem was and what this patch is trying to do.  Can you explain please?
Flags: needinfo?(amarchesini)
Can you please check the IsPrincipalInfoPrivate? And it's use. Mainly that one. Thanks
Flags: needinfo?(amarchesini)
"and how it is used"
Comment on attachment 8831220 [details] [diff] [review]
media.patch

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

The IsPincipalInfoPrivate() part looks correct!
Attachment #8831220 - Flags: review?(ehsan) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e72459157cf9
dom/media should use nsIPrincipal (and PrincipalInfo) instead origin as string, r=rjesup, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/e72459157cf9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Iteration: --- → 54.1 - Feb 6
Flags: qe-verify?
Depends on: 1335250
Want to request uplift to aurora? 53 is marked as affected.
Flags: needinfo?(amarchesini)
Comment on attachment 8831220 [details] [diff] [review]
media.patch

Approval Request Comment
[Feature/Bug causing the regression]: OriginAttributes porting in DOM Media
[User impact if declined]: the preview of a media stream is not visible.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, follow the description of the bug 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: we are passing from a nsString origin to a PrincipalInfo object. The code is big because it requires to change this pattern everywhere in the DOM media code.
[String changes made/needed]:
Flags: needinfo?(amarchesini)
Attachment #8831220 - Flags: approval-mozilla-aurora?
(In reply to Florian Quèze [:florian] [:flo] from comment #0)

> One thing I can't explain is that the preview works fine in the
> browser-chrome mochitest I'm writing to cover the screensharing UI. I would
> like us to have test coverage for this.

Do you know why this bug didn't occur while running http://searchfox.org/mozilla-central/source/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js ? It would be nice to have this covered by tests.
Flags: needinfo?(amarchesini)
Comment on attachment 8831220 [details] [diff] [review]
media.patch

May be risky but it would be good to fix this regression from 53.
Attachment #8831220 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Build ID: 20170213030206
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on on Windows 10 x 64, Mac OS X 10.11 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1 and Aurora 53.0a2.
Status: RESOLVED → VERIFIED
Depends on: 1340163
Flags: needinfo?(amarchesini)
Blocks: 1384800
You need to log in before you can comment on or make changes to this bug.