Closed Bug 1133493 Opened 9 years ago Closed 9 years ago

mozLoop should supply window IDs whilst tab sharing is active

Categories

(Hello (Loop) :: Client, defect, P1)

defect
Points:
5

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
mozilla39
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 --- fixed
firefox39 --- fixed
backlog backlog+

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [screensharing])

Attachments

(4 files, 9 obsolete files)

2.35 KB, patch
standard8
: review+
Details | Diff | Splinter Review
2.48 KB, patch
jesup
: review+
Details | Diff | Splinter Review
13.26 KB, patch
standard8
: review+
Details | Diff | Splinter Review
7.51 KB, patch
florian
: review+
Details | Diff | Splinter Review
To fetch the outer window ID of a browsers' content window, we'll need to implement an API that

 1. Allows retrieving the window ID of the currently active tabs' contentWindow
 2. Dispatches an event to the in-content Loop client when that active tab changes with its corresponding window ID as event data. This way, the client can switch the window to share via the SDK.
Flags: qe-verify-
Flags: firefox-backlog+
Target Milestone: --- → mozilla38
Group: mozilla-employee-confidential, core-security
Attached patch Step 1: show a dropdown (obsolete) — Splinter Review
I marked this bug private for now, because I'm touching the OpenTok SDK to make the whole thing work.

Brad, when you apply the three patches I posted here and try to 'Share my Tabs' within a Hello conversation, you'll see an instant crash. This is what I saw in my terminal window:

<snip>

console.debug: Loop
  notifyStatusChanged with reason:
null
CONSTRAINTS::{
  "video": {
    "mediaSource": "browser",
    "browserWindow": 2147483649,
    "scrollWithPage": true
  }
}
Assertion failure: !mIsSome, at ../../dist/include/mozilla/Maybe.h:385
#01: mozilla::dom::NavigatorBinding::mozGetUserMedia(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Navigator*, JSJitMethodCallArgs const&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x12dff7c]
#02: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1ba9b92]
#03: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x38741ad]
#04: js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e5e44d]
#05: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x38741ad]
#06: Interpret(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3890f7e]
#07: js::RunScript(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3884c8c]
#08: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x387414e]
#09: js_fun_apply(JSContext*, unsigned int, JS::Value*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e5bf15]
#10: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x38741ad]
#11: Interpret(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3890f7e]
#12: js::RunScript(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3884c8c]
#13: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x387414e]
#14: js_fun_apply(JSContext*, unsigned int, JS::Value*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3e5bf15]
#15: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x38741ad]
#16: Interpret(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3890f7e]
#17: js::RunScript(JSContext*, js::RunState&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3884c8c]
#18: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x387414e]
#19: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x385a9df]
#20: JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x3de63aa]
#21: mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1981a03]
#22: void mozilla::dom::Function::Call<nsCOMPtr<nsISupports> >(nsCOMPtr<nsISupports> const&, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe0b6e8]
#23: nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe01c72]
#24: nsGlobalWindow::RunTimeout(nsTimeout*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xdf398b]
#25: nsGlobalWindow::TimerCallback(nsITimer*, void*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xe017f7]
#26: nsTimerImpl::Fire()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xdfb8b]
#27: nsTimerEvent::Run()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xdff20]
#28: nsThread::ProcessNextEvent(bool, bool*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0xdadf0]
#29: NS_ProcessPendingEvents(nsIThread*, unsigned int)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x10a08f]
#30: nsBaseAppShell::NativeEventCallback()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2558607]
#31: nsAppShell::ProcessGeckoEvents(void*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25ac85e]
#32: __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x80661]
#33: __CFRunLoopDoSources0[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x727ed]
#34: __CFRunLoopRun[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x71e1f]
#35: CFRunLoopRunSpecific[/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation +0x71838]
#36: RunCurrentEventLoopInMode[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x2e43f]
#37: ReceiveNextEventCommon[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x2e0be]
#38: _BlockUntilNextEventMatchingListInModeWithFilter[/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/HIToolbox.framework/Versions/A/HIToolbox +0x2dffb]
#39: _DPSNextEvent[/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x246d1]
#40: -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x23e80]
#41: -[GeckoNSApplication nextEventMatchingMask:untilDate:inMode:dequeue:][/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25abae6]
#42: -[NSApplication run][/System/Library/Frameworks/AppKit.framework/Versions/C/AppKit +0x17e23]
#43: nsAppShell::Run()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x25acea0]
#44: nsAppStartup::Run()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x2feece9]
#45: XREMain::XRE_mainRun()[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x305c7f4]
#46: XREMain::XRE_main(int, char**, nsXREAppData const*)[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x305cf42]
#47: XRE_main[/Users/mikedeboer/Projects/fx-team/obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/XUL +0x305d34c]
#48: main[/Users/mikedeboer/Projects/fx-team/./obj-x86_64-apple-darwin14.0.0/dist/NightlyDebug.app/Contents/MacOS/firefox-bin +0x20fb]

</snip>

Do you have an idea what this might be about?
Base these patches on top of latest fx-team.
Attachment #8565038 - Attachment is obsolete: true
Attachment #8565039 - Attachment is obsolete: true
I've fixed the crasher, but now I only see a black screen that should be the browser contents. Is there more that we need to do to get some nifty moving pictures?
Attachment #8565041 - Attachment is obsolete: true
Flags: needinfo?(blassey.bugs)
Group: core-security
The first thing I'd check is if you've got the correct window ID. The second thing I'd try is adding an advanced section with a min and max width and height. If that fixes it, there is a but we need to fix in the tab source allocation code.
Flags: needinfo?(blassey.bugs)
I figured out what it is. Everything works as expected (with and without 'advanced' section) for _non_ e10s tabs/ contentWindows.

I verified that the window ID is correct when using `contentWindowAsCPOW` and with proper message passing in content.js (see updated patch). In fact, the sharing works with window IDs that the content script returns for non-e10s windows.

So back to you, Brad ;-)
Flags: needinfo?(blassey.bugs)
Attachment #8565441 - Attachment is obsolete: true
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> I figured out what it is. Everything works as expected (with and without
> 'advanced' section) for _non_ e10s tabs/ contentWindows.
> 
> I verified that the window ID is correct when using `contentWindowAsCPOW`
> and with proper message passing in content.js (see updated patch). In fact,
> the sharing works with window IDs that the content script returns for
> non-e10s windows.
> 
> So back to you, Brad ;-)

Try logging the window ID (put a printf here https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/MediaEngineTabVideoSource.cpp#126) you get when trying to mirror the tab (you can use the Roku simulator, prebuilt for mac http://people.mozilla.org/~rbarker/roku-sim.tgz) and compare it to the window ID you're passing in (same logging will work).
Flags: needinfo?(blassey.bugs)
Group: mozilla-employee-confidential
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> Try logging the window ID (put a printf here
> https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> MediaEngineTabVideoSource.cpp#126) you get when trying to mirror the tab
> (you can use the Roku simulator, prebuilt for mac
> http://people.mozilla.org/~rbarker/roku-sim.tgz) and compare it to the
> window ID you're passing in (same logging will work).

I logged the ID there too and I get the same windowID all the way down to `MediaEngineTabVideoSource`.

Now I have no idea how the Roku simulator is supposed to work :( I downloaded the package, unpacked and ran `start.sh` from the command line. This yields the following output:

DYLD_LIBRARY_PATH=/Users/mikedeboer/Downloads/roku-sim/bin
Starting Simulator
Starting SSDP thread

This seems correct, but I don't see an app launching or any other kind of UI. Is there something that needs to happen in Fx? Some prefs to set? Or do I need Fx for Android to use this?

I don't think it'd be weird if this just doesn't work yet for remote (e10s) browsers, as this was developed before e10s was enabled on startup for all tabs. Much has changed since then, so it's possible we need to fix something after all.
Flags: needinfo?(blassey.bugs)
Depends on: 1131584
Attachment #8565439 - Attachment is obsolete: true
Attachment #8565898 - Attachment is obsolete: true
Attachment #8567053 - Flags: review?(standard8)
Florian, would you be willing to review the WebRTC message passing of this patch?

Mark, could you review the mozLoop and the tests?
Attachment #8567055 - Flags: review?(standard8)
Attachment #8567055 - Flags: review?(florian)
Forgot a test.
Attachment #8567064 - Attachment is obsolete: true
Attachment #8567064 - Flags: review?(standard8)
Attachment #8567067 - Flags: review?(standard8)
(In reply to Mike de Boer [:mikedeboer] from comment #11)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> > Try logging the window ID (put a printf here
> > https://mxr.mozilla.org/mozilla-central/source/dom/media/webrtc/
> > MediaEngineTabVideoSource.cpp#126) you get when trying to mirror the tab
> > (you can use the Roku simulator, prebuilt for mac
> > http://people.mozilla.org/~rbarker/roku-sim.tgz) and compare it to the
> > window ID you're passing in (same logging will work).
> 
> I logged the ID there too and I get the same windowID all the way down to
> `MediaEngineTabVideoSource`.
> 
> Now I have no idea how the Roku simulator is supposed to work :( I
> downloaded the package, unpacked and ran `start.sh` from the command line.
> This yields the following output:
> 
> DYLD_LIBRARY_PATH=/Users/mikedeboer/Downloads/roku-sim/bin
> Starting Simulator
> Starting SSDP thread
> 
> This seems correct, but I don't see an app launching or any other kind of
> UI. Is there something that needs to happen in Fx? Some prefs to set? Or do
> I need Fx for Android to use this?

Yes, in Firefox you need to go to tools -> Mirror Tab  and select the Roku simulator. That will start mirroring your current tab to the roku simulator. 
> 
> I don't think it'd be weird if this just doesn't work yet for remote (e10s)
> browsers, as this was developed before e10s was enabled on startup for all
> tabs. Much has changed since then, so it's possible we need to fix something
> after all.

Tab mirroring works with e10s, there's no reason this shouldn't
Flags: needinfo?(blassey.bugs)
Attachment #8567054 - Flags: review?(rjesup) → review+
Attachment #8567053 - Flags: review?(standard8) → review+
Attachment #8567055 - Flags: review?(standard8) → review+
Comment on attachment 8567067 [details] [diff] [review]
Patch 4.1: add necessary actions to start sharing a browser tab and pass the necessary parameters to the OpenTok SDK

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

Looking good, not far off, but a few things to address.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +405,5 @@
> +        state: SCREEN_SHARE_STATES.PENDING
> +      }));
> +
> +      var options = {
> +        videoSource: actionData.type || "window"

actionData.type is a required item, so lets drop the fallback.

@@ +407,5 @@
> +
> +      var options = {
> +        videoSource: actionData.type || "window"
> +      };
> +      if (options.videoSource == "browser") {

nit: ===

@@ +410,5 @@
> +      };
> +      if (options.videoSource == "browser") {
> +        this._mozLoop.getActiveTabWindowId(function(err, windowId) {
> +          if (err || !windowId) {
> +            return;

We should either set the screen sharing state to inactive here or not set it to pending until we've got a windowId (if we need one). Otherwise, we could get a one-off error that the user wouldn't be able to get out of until they restart.

::: browser/components/loop/content/shared/js/otSdkDriver.js
@@ +123,5 @@
>        this.screenshare.off("accessAllowed accessDenied");
>        this.screenshare.destroy();
>        delete this.screenshare;
>        this.dispatcher.dispatch(new sharedActions.ScreenSharingState({
>          state: SCREEN_SHARE_STATES.INACTIVE

You either need to remove this from here, or not have the activeRoomStore dispatch it.

::: browser/components/loop/test/shared/activeRoomStore_test.js
@@ +705,5 @@
> +          state: SCREEN_SHARE_STATES.PENDING
> +        }));
> +    });
> +
> +    it("should invoke the SDK driver with the correct options", function() {

Please split this into two, one for window and the other for browser.

::: browser/components/loop/test/shared/otSdkDriver_test.js
@@ +147,5 @@
>        sinon.assert.calledOnce(sdk.initPublisher);
> +      sinon.assert.calledWithMatch(sdk.initPublisher, fakeElement, options);
> +
> +      options = {
> +        videoSource: "browser",

I think you could just collapse this down into one test - i.e. with options videSource as browser, and the browserWindow & scrollWithPage.

Since the function isn't doing anything specific with videoSource, we don't need to test both forms.
Attachment #8567067 - Flags: review?(standard8) → review-
backlog: --- → backlog+
Rank: 2
Whiteboard: [screensharing]
Blocks: 1131574
Carrying over r=Standard8.
Attachment #8567055 - Attachment is obsolete: true
Attachment #8567055 - Flags: review?(florian)
Attachment #8568474 - Flags: review?(florian)
Attachment #8568034 - Flags: review?(standard8) → review+
Attachment #8568474 - Flags: review?(florian) → review+
https://hg.mozilla.org/mozilla-central/rev/ce370d6642e0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla38 → mozilla39
(In reply to Wes Kocher (:KWierso) from comment #22)
> https://hg.mozilla.org/mozilla-central/rev/ce370d6642e0

> Bug 1133493: add e10s-friendly API to fetch a tab's outer window ID

The commit message got my hopes up but it turns out it was just misleading and a solution only for scrren sharing instead of one for any code dealing with <tab>…
Ugh, I apologize for the inconvenience wrt the ill-formatted commit messages. I'll do better next time. Lesson learned.
Not sure if it matters in here, but in bug 1077168, I added a outerWindowID read only property to <xul:browser>'s, which works with remote (e10s). browsers as well.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #26)
> Not sure if it matters in here, but in bug 1077168, I added a outerWindowID
> read only property to <xul:browser>'s, which works with remote (e10s).
> browsers as well.

Cool! I'll see to it that we can revert our workaround asap.
Comment on attachment 8567054 [details] [diff] [review]
Patch 2: allow Loop content pages to access gUM constraint data

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello screensharing milestone
[User impact if declined]: User will see a new option that allows her/ him to share a window or Firefox tabs inside a room (aka. conversation).
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a
Attachment #8567054 - Flags: approval-mozilla-aurora?
Comment on attachment 8568474 [details] [diff] [review]
Patch 3.1: add e10s-friendly API to fetch a tab's outer window ID

Please see comment 28 for approval request.
Attachment #8568474 - Flags: approval-mozilla-aurora?
Comment on attachment 8568034 [details] [diff] [review]
Patch 4.2: add necessary actions to start sharing a browser tab and pass the necessary parameters to the OpenTok SDK

Please see comment 28 for approval request.
Attachment #8568034 - Flags: approval-mozilla-aurora?
Attachment #8567054 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8568034 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8568474 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.