Florian Quèze [:florian] Reporter
	Description • 8 years ago

See the intermittent failures on Linux32 on https://treeherder.mozilla.org/#/jobs?repo=try&revision=35aec329e6b6

That's with the test I'm working on in bug 1313324.

	Florian Quèze [:florian] Reporter
	Comment 1 • 8 years ago

The test was failing frequently enough on Linux opt that I disabled it there, but it's also failing less frequently on Linux debug, see bug 1324303 and bug 1324261.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 2 • 8 years ago

The bugs depending on this are getting frequent. Best would be to fix the root cause or the tests will be wallpapered.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 3 • 7 years ago

when we do fix this or test on try, please ensure you revert:
https://bug1338038.bmoattachments.org/attachment.cgi?id=8847189

I have added leave-open to bug 1338038, bug 1324261, and bug 1324303 to verify when the test is turned on again.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 9 • 7 years ago

My latest try push with some extra debugging patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=35cd1fe45a182eca08d83224545625aa1a0427e7

The browser_downloads_panel_block.js test is worrying but most likely unrelated to these changes.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

Liking the refactor! A lot of code moving around here though, making review tough. Various nits and questions. r- for release of domStream on wrong thread in a case of error.

::: dom/media/MediaManager.cpp:293
(Diff revision 1)
> -  // The task will reset this to false. MainThread only.
> +  PrincipalHandle mPrincipalHandle;
> -  bool mChromeNotificationTaskPosted;
>  
> +  // Weak pointer to the window listener that owns us. MainThread only.
> +  GetUserMediaWindowListener* mWindowListener;
> + 

trailing space

::: dom/media/MediaManager.cpp:375
(Diff revision 1)
> -          source->SetPullEnabled(true);
> -          source->AdvanceKnownTracksTime(STREAM_TIME_MAX);
> +  // implement in .cpp to avoid circular dependency with MediaOperationTask
> +  // Can be invoked from EITHER MainThread or MSG thread

Should stop referring to MediaOperationTask now that you've obliterated it. ;)

::: dom/media/MediaManager.cpp:395
(Diff revision 1)
> -          NS_ASSERTION(!NS_IsMainThread(), "Never call on main thread");
> -          if (mAudioDevice) {
> -            mAudioDevice->GetSource()->Stop(source, kAudioTrack);
> -            mAudioDevice->Deallocate();
> +    RefPtr<MediaManager> mgr = MediaManager::GetInstance();
> +    if (!mgr) {
> +      MOZ_ASSERT(false, "MediaManager should stay until everything is removed");
> +      return;
> -          }
> +    }

MediaManager::GetInstance() cannot return null, as it will (re)create MediaManager if necessary, so this assert is dead code, and should be removed IMHO.

If you specifically wish to avoid re-creating MediaManager (e.g. in code that you worry could be called late during shutdown), then use GetIfExists() instead.

The latter plus keeping the assert may make most sense here.

::: dom/media/MediaManager.cpp:400
(Diff revision 1)
> -          if (mVideoDevice) {
> -            mVideoDevice->GetSource()->Stop(source, kVideoTrack);
> +    RefPtr<GetUserMediaWindowListener> windowListener =
> +      mgr->GetWindowListener(mWindowID);

Nit: Redundant RefPtr. There's no handoff here, and having to refcount just to reference something which lifespan is invariant in this run of the event loop seems excessive, expensive, and obfuscating.

I know there's a war on raw pointers, so YMMV, but you're using a raw pointer for the globalWindow reference below FWIW, but then for `window` you use a RefPtr again...

::: dom/media/MediaManager.cpp:406
(Diff revision 1)
> +      RefPtr<nsPIDOMWindowInner> window = globalWindow ? globalWindow->AsInner()
> +                                                       : nullptr;
> +      if (window) {
> +        RefPtr<GetUserMediaRequest> req =
> +          new GetUserMediaRequest(window, NullString(), NullString());

Nit: Doesn't this boil down to:

    if (globalWindow) {
        RefPtr<GetUserMediaRequest> req =
          new GetUserMediaRequest(globalWindow->AsInner(), NullString(), NullString());

?

::: dom/media/MediaManager.cpp:427
(Diff revision 1)
> -            mVideoDevice->GetSource()->SetDirectListeners(mBool);
> -          }
> +    if (!mInactiveListeners.RemoveElement(aListener) &&
> +        !mActiveListeners.RemoveElement(aListener)) {
> +      return false;
> -        }
> +    }

I think this assumes aListener can never be in both, or this would fail to remove it from both. Is this asserted explicitly anywhere?

::: dom/media/MediaManager.cpp:438
(Diff revision 1)
> +      nsString removedSourceType;
> +      removedDevice->GetRawId(removedRawId);
> +      removedDevice->GetMediaSource(removedSourceType);

These could move closer to their usage.

::: dom/media/MediaManager.cpp:455
(Diff revision 1)
> +        RefPtr<nsPIDOMWindowInner> window = globalWindow ? globalWindow->AsInner()
> +                                                         : nullptr;
> +        RefPtr<GetUserMediaRequest> req =
> +          new GetUserMediaRequest(window, removedRawId, removedSourceType);

Redundant RefPtr again.

::: dom/media/MediaManager.cpp:466
(Diff revision 1)
> +      nsString removedSourceType;
> +      removedDevice->GetRawId(removedRawId);
> +      removedDevice->GetMediaSource(removedSourceType);

same here (move closer to usage)

::: dom/media/MediaManager.cpp:483
(Diff revision 1)
> +        RefPtr<nsPIDOMWindowInner> window = globalWindow ? globalWindow->AsInner()
> +                                                         : nullptr;
> +        RefPtr<GetUserMediaRequest> req =
> +          new GetUserMediaRequest(window, removedRawId, removedSourceType);

same here (redundant RefPtr)

::: dom/media/MediaManager.cpp:491
(Diff revision 1)
> +    if (mInactiveListeners.Length() == 0 &&
> +        mActiveListeners.Length() == 0) {
> +      LOG(("GUMWindowListener %p Removed the last SourceListener. "
> +           "Cleaning up.", this));
> +      RemoveAll();

Won't this send "recording-device-stopped" twice?

::: dom/media/MediaManager.cpp:491
(Diff revision 1)
> +    if (mInactiveListeners.Length() == 0 &&
> +        mActiveListeners.Length() == 0) {

The reason I mentioned asserting on the invariant earlier that listeners were never in both lists is that otherwise we could arrive here where a listener only got removed from one list and not the other, and this if-condition would not be met and RemoveAll would not get called.

::: dom/media/MediaManager.cpp:1025
(Diff revision 1)
> -    StreamListeners* listeners = mManager->GetWindowListeners(mWindowID);
> -    if (!listeners || !window || !window->GetExtantDoc()) {
> +    RefPtr<GetUserMediaWindowListener> listener =
> +      mManager->GetWindowListener(mWindowID);

Unnecessary RefPtr introduction

::: dom/media/MediaManager.cpp:1182
(Diff revision 1)
>          onFailure->OnError(error);
>        }
>        return NS_OK;
>      }
>  
> -    // The listener was added at the beginning in an inactive state.
> +    // Activate our source listener. We'll call Start() on the source when get a

Do you mean: when we get a ?

::: dom/media/MediaManager.cpp:1187
(Diff revision 1)
> -    mListener->Activate(stream.forget(), mAudioDevice, mVideoDevice);
> +    /**
> +     * Ugly helper to let us pass an OnTracksAvailableCallback into a lambda.
> +     */
> +    class CallbackHolder
> +    {
> +    public:
> +      explicit CallbackHolder(UniquePtr<OnTracksAvailableCallback>&& aCallback)
> +        // The following dud code exists to avoid 'unused typedef' error on linux.
> +        : mTracksAvailableCallback(CallbackHolder::HasThreadSafeRefCnt::value
> +            ? Forward<UniquePtr<OnTracksAvailableCallback>>(aCallback)
> +            : nullptr)
> +      {}
> +
> +      NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CallbackHolder)
> +      UniquePtr<OnTracksAvailableCallback> mTracksAvailableCallback;
> +    protected:
> +      ~CallbackHolder() {}
> +    };

Please consider reusing the Refcountable<UniquePtr<T>> helper [1] instead, if possible, like here [2]. I wrote it pretty much to try to solve this exact problem.

If you tried that and it didn't work, feel free to modify that class to fit your needs, e.g. maybe it needs a move constructor?

Btw, I got deja vu about that unused typedef warning... ;) [3]

[1] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/systemservices/MediaUtils.h#331-339

[2] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/MediaManager.cpp#2302-2315

[3] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/webrtc/MediaTrackConstraints.cpp#447-448

::: dom/media/MediaManager.cpp:1208
(Diff revision 1)
> +      ~CallbackHolder() {}
> +    };
>  
>      // Note: includes JS callbacks; must be released on MainThread
> -    TracksAvailableCallback* tracksAvailableCallback =
> -      new TracksAvailableCallback(mManager, mOnSuccess, mWindowID, domStream);
> +    RefPtr<CallbackHolder> callbackHolder = new CallbackHolder(
> +    //callbackHolder->mTracksAvailableCallback =

Did you mean to leave this commented-out code in?

::: dom/media/MediaManager.cpp:1213
(Diff revision 1)
>      // Pass ownership of domStream to the MediaOperationTask
>      // to ensure it's kept alive until the MediaOperationTask runs (at least).

Should stop referring to MediaOperationTask now that you've obliterated it. ;)

+1 for this doing this btw!

::: dom/media/MediaManager.cpp:1215
(Diff revision 1)
> -    RefPtr<Runnable> mediaOperation =
> -        new MediaOperationTask(MEDIA_START, mListener, domStream,
> -                               tracksAvailableCallback,
> +    RefPtr<SourceListener> sourceListener = mSourceListener;
> +    RefPtr<AudioDevice> audioDevice = mAudioDevice;
> +    RefPtr<VideoDevice> videoDevice = mVideoDevice;

Technically redundant. Is this just for readability, to avoid capturing variables with m-prefixed names?

I know m-prefix is our style guide, just wondering is there a precedent or guidance here? I asked in #developers but didn't get any answers.

I'm tempted to say it's ok to lambda capture mVariables directly, for the code savings alone. Lambda functions already inherit other member properties like being able to reference private members even through a self pointer, e.g. self->mPrivateMember. Embrace the closure! maybe :)

::: dom/media/MediaManager.cpp:1251
(Diff revision 1)
> +      if (error) {
> +        nsCOMPtr<nsIDOMGetUserMediaSuccessCallback> onSuccess;
> +        NS_DispatchToMainThread(do_AddRef(
> +          new ErrorCallbackRunnable<nsIDOMGetUserMediaSuccessCallback>(
> +            onSuccess, onFailure, *error, windowID)));
> +        return NS_OK;

Looks like this will free domStream on the wrong thread when an error happens. I think we lost this code [1] here.

I think technically we need s/onFailure/onFailure.forget()/ as well, since I think it's unlikely but possible in theory for the main thread to run and finish this dispatch immediately, leaving us with the last refcount here, freeing it on the wrong thread. I see that this was true before this patch as well, but since you're here. 

Btw, love this refactor! Someday I'd love to take it all the way by unwinding GetUserMediaNotificationEvent::STARTING as well and breaking this part out as a sub-function returning a pledge, so that onSuccess, onFailure and domStream wouldn't need to swing by the media thread at all, e.g. like we do elsewhere [2] in this file. But maybe not in patch.

[1] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/MediaManager.cpp#901-902

[2] https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/dom/media/MediaManager.cpp#2575,2581

::: dom/media/MediaManager.cpp
(Diff revision 1)
> -  props->SetPropertyAsBool(NS_LITERAL_STRING("isAudio"), aIsAudio);
> -  props->SetPropertyAsBool(NS_LITERAL_STRING("isVideo"), aIsVideo);

Aren't these properties needed? [1]

What sets them properties now?

[1] https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/browser/base/content/test/webrtc/browser_devices_get_user_media.js#96-97

::: dom/media/MediaManager.cpp
(Diff revision 1)
> -  // Forward recording events to parent process.
> -  // The events are gathered in chrome process and used for recording indicator
> -  if (!XRE_IsParentProcess()) {
> -    Unused <<
> -      dom::ContentChild::GetSingleton()->SendRecordingDeviceEvents(aMsg,
> -                                                                   requestURL,
> -                                                                   aIsAudio,
> -                                                                   aIsVideo);
> -  }

I see this was added in https://hg.mozilla.org/mozilla-central/rev/7d02b81f691b for b2g. Are we sure it's no longer needed, even in e10s?

::: dom/media/MediaManager.cpp:2603
(Diff revision 1)
>  
> -  StreamListeners* listeners = AddWindowID(windowId);
> -
>    nsIPrincipal* principal = aWindow->GetExtantDoc()->NodePrincipal();
>  
>    // Create a disabled listener to act as a placeholder

Update or move comment

::: dom/media/MediaManager.cpp:3119
(Diff revision 1)
>        continue;
>      }
> -    // mActiveWindows contains both windows that have requested device
> -    // access and windows that are currently capturing media. We want
> -    // to return only the latter. See bug 975177.
>      bool capturing = false;

Nit: Can factor out this auto now.

::: dom/media/MediaManager.cpp:3259
(Diff revision 1)
>  {
> -  if (aListeners) {
> -    auto length = aListeners->Length();
> -    for (size_t i = 0; i < length; ++i) {
> -      aListeners->ElementAt(i)->StopSharing();
> +  // Iterate the docshell tree to find all the child windows, and for each
> +  // invoke the callback
> +  if (aWindow) {
> +    uint64_t windowID = aWindow->WindowID();
> +    RefPtr<GetUserMediaWindowListener> listener = GetWindowListener(windowID);

-RefPtr

::: dom/media/MediaManager.cpp:3454
(Diff revision 1)
> +  MOZ_ASSERT(NS_IsMainThread(), "Only call on main thread");
> +
> +  RefPtr<MediaDevice> device;
> +  RefPtr<SourceMediaStream> source;
> +
> +  if (aTrackID == kAudioTrack) {

Nit: Maybe use switch for consistency with SourceListener::GetSettings ?

::: dom/media/MediaManager.cpp:3483
(Diff revision 1)
> +  MediaManager::PostTask(NewTaskFrom([device, source, aTrackID]() {
> +    MOZ_ASSERT(MediaManager::IsInMediaThread());

I personally find this assert a bit redundant, but ok.

::: dom/media/MediaManager.cpp:3662
(Diff revision 1)
> +  RefPtr<VideoDevice> videoDevice = mVideoDevice;
> +  MediaManager::PostTask(NewTaskFrom([videoDevice, aHasListeners]() {
> +    MOZ_ASSERT(MediaManager::IsInMediaThread());

Redundant auto depending on which way we go on mFoo lambda captures. Also, assert seems redundant to me.

::: dom/media/MediaManager.cpp
(Diff revision 1)
>    case STARTING:
>      msg = NS_LITERAL_STRING("starting");
> -    stream->OnTracksAvailable(mOnTracksAvailableCallback.forget());
> +    stream->OnTracksAvailable(mOnTracksAvailableCallback.release());
>      break;
>    case STOPPING:
> -  case STOPPED_TRACK:

Looks like it's been a while since STOPPED_TRACK did anything different from STOPPING?

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review130170

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8855306 [details]
Bug 1320994 - Unify MediaManager logging macros.

https://reviewboard.mozilla.org/r/127182/#review130172

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8855307 [details]
Bug 1320994 - Improve SourceListener logging.

https://reviewboard.mozilla.org/r/127184/#review130174

	Andreas Pehrson [:pehrsons] Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> Nit: Doesn't this boil down to:
> 
>     if (globalWindow) {
>         RefPtr<GetUserMediaRequest> req =
>           new GetUserMediaRequest(globalWindow->AsInner(), NullString(), NullString());
> 
> ?

If `GetInnerWindowWithId` guarantees to return a window where `AsInner` doesn't return null, yes. That appears to be the case, from looking at the code.

> I think this assumes aListener can never be in both, or this would fail to remove it from both. Is this asserted explicitly anywhere?

They're only added to inactive in one place, and transfered to active in one other. But a debug assert here cannot be wrong, I'll add one.

> These could move closer to their usage.

The source type, yes, but not the raw id. And I do see value in keeping them together since they have the same scope.

> Won't this send "recording-device-stopped" twice?

The second one will only be sent if the window listener has been removed from MediaManager, but that happens later. This is the same behavior as before. Perhaps a bit more obscure now :/

> Do you mean: when we get a ?

Yes!

> Please consider reusing the Refcountable<UniquePtr<T>> helper [1] instead, if possible, like here [2]. I wrote it pretty much to try to solve this exact problem.
> 
> If you tried that and it didn't work, feel free to modify that class to fit your needs, e.g. maybe it needs a move constructor?
> 
> Btw, I got deja vu about that unused typedef warning... ;) [3]
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/systemservices/MediaUtils.h#331-339
> 
> [2] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/MediaManager.cpp#2302-2315
> 
> [3] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/webrtc/MediaTrackConstraints.cpp#447-448

Oh, I didn't know that existed! I did see your typedef warning comment and stole the fix though.

> Did you mean to leave this commented-out code in?

My mistake.

> Technically redundant. Is this just for readability, to avoid capturing variables with m-prefixed names?
> 
> I know m-prefix is our style guide, just wondering is there a precedent or guidance here? I asked in #developers but didn't get any answers.
> 
> I'm tempted to say it's ok to lambda capture mVariables directly, for the code savings alone. Lambda functions already inherit other member properties like being able to reference private members even through a self pointer, e.g. self->mPrivateMember. Embrace the closure! maybe :)

I'll capture this and a self then, because capturing the members directly doesn't compile.

> Looks like this will free domStream on the wrong thread when an error happens. I think we lost this code [1] here.
> 
> I think technically we need s/onFailure/onFailure.forget()/ as well, since I think it's unlikely but possible in theory for the main thread to run and finish this dispatch immediately, leaving us with the last refcount here, freeing it on the wrong thread. I see that this was true before this patch as well, but since you're here. 
> 
> Btw, love this refactor! Someday I'd love to take it all the way by unwinding GetUserMediaNotificationEvent::STARTING as well and breaking this part out as a sub-function returning a pledge, so that onSuccess, onFailure and domStream wouldn't need to swing by the media thread at all, e.g. like we do elsewhere [2] in this file. But maybe not in patch.
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/ec8d1d3db50c85037e8077c32c8403570a5df493/dom/media/MediaManager.cpp#901-902
> 
> [2] https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/dom/media/MediaManager.cpp#2575,2581

Great catch, thanks!

onFailure is actually being `swap()`ed by the `ErrorCallbackRunnable` constructor, so it takes ownership. I'm however changing it to use `Move` to be explicit at the callsite.

> Aren't these properties needed? [1]
> 
> What sets them properties now?
> 
> [1] https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/browser/base/content/test/webrtc/browser_devices_get_user_media.js#96-97

These properties were needed on B2G, but I intentionally broke support for B2G here. The B2G frontend code is still there FWIW.

The two calls you reference are completely separate. The first waits for "recording-device-events" (doesn't check properties). The second (`getMediaCaptureState()`) calls async back into MediaManager to query for the state. See `MediaManager::MediaCaptureWindowState()`.

> I see this was added in https://hg.mozilla.org/mozilla-central/rev/7d02b81f691b for b2g. Are we sure it's no longer needed, even in e10s?

https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/dom/ipc/ContentParent.cpp#3948

It ends up sending a "recording-device-ipc-events" which is b2g only: https://dxr.mozilla.org/mozilla-central/search?q=%22recording-device-ipc-events%22&redirect=true

> Nit: Can factor out this auto now.

I don't think I follow.

> Redundant auto depending on which way we go on mFoo lambda captures. Also, assert seems redundant to me.

If you're claiming that members can be directly captured I'd like an example :-)

I'm getting
> MediaManager.cpp:3704:39: error: capture of non-variable ‘mozilla::SourceListener::mVideoDevice’

	Florian Quèze [:florian] Reporter
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8855305 [details]
Bug 1320994 - Remove chrome-test hacks that accomodated the misaligned MediaManager model.

https://reviewboard.mozilla.org/r/127180/#review130348

Thanks for cleaning up all of this! :-)

::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:417
(Diff revision 2)
>      info("Stop the screen share, mic+cam should continue");
>      yield stopSharing("screen", true);
>      yield check({video: true, audio: true});
>  
>      info("Stop the camera, everything should stop.");
> -    yield stopSharing("camera", false, true);
> +    yield stopSharing("camera", false);

Please remove the 'false' parameter too, it's optional.

::: browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:430
(Diff revision 2)
>      info("... and add camera and microphone in a second request.");
>      yield share(true, true);
>      yield check({video: true, audio: true, screen: "Screen"});
>  
>      info("Stop the camera, this should stop everything.");
> -    yield stopSharing("camera", false, true);
> +    yield stopSharing("camera", false);

Same here.

	Florian Quèze [:florian] Reporter
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8855303 [details]
Bug 1320994 - Re-enable tests.

https://reviewboard.mozilla.org/r/127170/#review130354

Very happy r+, however to be fully confident I would like to see a try push with a syntax like this:
 try: -b do -p linux64,linux64-asan,linux,macosx64,win32,win64 -u mochitest-bc,mochitest-e10s-bc -t none --rebuild 10 --try-test-paths browser-chrome:browser/base/content/test/webrtc

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 21 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> I don't think I follow.

I just meant the capturing boolean was needed when we had a for-loop, but seems superfluous now:

    if (winListener) {
      if (winListener->CapturingVideo() || winListener->CapturingAudio() ||
          winListener->CapturingScreen() || winListener->CapturingWindow() ||
          winListener->CapturingApplication()) {
        array->AppendElement(window, /*weak =*/ false);
      }
    }

> If you're claiming that members can be directly captured I'd like an example :-)
> 
> I'm getting
> > MediaManager.cpp:3704:39: error: capture of non-variable ‘mozilla::SourceListener::mVideoDevice’

Yeah I really should test my assumptions before making broad claims in public, is the problem. :*)

Found an interesting workaround to skip the extranous copy though: Use an auto reference; The member still gets passed-by-copy! [1]:

    auto& videoDevice = mVideoDevice;
    MediaManager::PostTask(NewTaskFrom([videoDevice, aHasListeners]() {

[1] http://stackoverflow.com/questions/7895879/using-member-variable-in-lambda-capture-list-inside-a-member-function

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review130440

::: dom/media/MediaManager.cpp:1202
(Diff revisions 1 - 2)
> -    RefPtr<CallbackHolder> callbackHolder = new CallbackHolder(
> -    //callbackHolder->mTracksAvailableCallback =
> -      MakeUnique<TracksAvailableCallback>(mManager, mOnSuccess, mWindowID, domStream));
> +    RefPtr<Refcountable<UniquePtr<OnTracksAvailableCallback>>> tracksAvailableCallback =
> +      new Refcountable<UniquePtr<OnTracksAvailableCallback>>(
> +        new TracksAvailableCallback(mManager, mOnSuccess, mWindowID, domStream));

Cool. Maybe:

    RefPtr<Refcountable<UniquePtr<OnTracksAvailableCallback>>> tracksAvailableCallback =
      MakeAndAddRef<Refcountable<UniquePtr<OnTracksAvailableCallback>>>(mManager, mOnSuccess, mWindowID, domStream);

? Not sure it buys much, a few characters maybe.

::: dom/media/MediaManager.cpp:1211
(Diff revisions 1 - 2)
> -    RefPtr<AudioDevice> audioDevice = mAudioDevice;
> -    RefPtr<VideoDevice> videoDevice = mVideoDevice;
> -    uint64_t windowID = mWindowID;
> +    RefPtr<GetUserMediaStreamRunnable> self = this;
> +    MediaManager::PostTask(NewTaskFrom([this, self, domStream,
> +                                        tracksAvailableCallback]() mutable {

I think we have static analysis now that poo poo's capturing this when this is ref counted.

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 23 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review130440

> Cool. Maybe:
> 
>     RefPtr<Refcountable<UniquePtr<OnTracksAvailableCallback>>> tracksAvailableCallback =
>       MakeAndAddRef<Refcountable<UniquePtr<OnTracksAvailableCallback>>>(mManager, mOnSuccess, mWindowID, domStream);
> 
> ? Not sure it buys much, a few characters maybe.

I've filed bug 1354642 on adding a MakeRefPtr that should help here.

> I think we have static analysis now that poo poo's capturing this when this is ref counted.

Which means you have to add `self->` everywhere now inside lambdas. See https://hg.mozilla.org/mozilla-central/rev/9730f8b59968#l2.12

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 7 years ago
mozreview-review

Comment on attachment 8855303 [details]
Bug 1320994 - Re-enable tests.

https://reviewboard.mozilla.org/r/127170/#review131168

::: browser/base/content/test/webrtc/browser.ini:8
(Diff revision 1)
>    get_user_media.html
>    get_user_media_in_frame.html
>    get_user_media_content_script.js
>    head.js
>  
>  [browser_devices_get_user_media.js]

Can any of these other skipped tests get re-enabled too by this? Have we checked? Between this and bug 1353910, I'm hoping they're in a better place now than they've been in the past.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 25 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review130440

> I've filed bug 1354642 on adding a MakeRefPtr that should help here.

Adds a couple characters since it still needs the separate TracksAvailableCallback constructor call. It's better form perhaps.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 26 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> I just meant the capturing boolean was needed when we had a for-loop, but seems superfluous now:
> 
>     if (winListener) {
>       if (winListener->CapturingVideo() || winListener->CapturingAudio() ||
>           winListener->CapturingScreen() || winListener->CapturingWindow() ||
>           winListener->CapturingApplication()) {
>         array->AppendElement(window, /*weak =*/ false);
>       }
>     }

Right. I also moved the `if (winListener)` to a guard.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 27 • 7 years ago
mozreview-review-reply

Comment on attachment 8855303 [details]
Bug 1320994 - Re-enable tests.

https://reviewboard.mozilla.org/r/127170/#review131168

> Can any of these other skipped tests get re-enabled too by this? Have we checked? Between this and bug 1353910, I'm hoping they're in a better place now than they've been in the past.

I checked the one referring to bug 1334752 because it seemed related to my changes here. But no, I still got failures. The other two seem unrelated and in areas I don't know.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 28 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> I'll capture this and a self then, because capturing the members directly doesn't compile.

NB capturing the `self` makes the runnable be released on the media thread sometimes, and some members don't like that.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 29 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> NB capturing the `self` makes the runnable be released on the media thread sometimes, and some members don't like that.

`members` being `mOnSuccess`, which, looking at semantics in the code, should be owned by the `TracksAvailableCallback`. I'm transferring ownership via a `.forget()`.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 34 • 7 years ago
mozreview-review-reply

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review129984

> -RefPtr

Hmm, this caused a UAF: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6805ba31dce4f5b21a38348f479c2f6618947206

I'll `AddRef()` in the appropriate callbacks, so we don't do it unnecessarily.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 39 • 7 years ago

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

I had to do some changes here, could you please just check out the rev 2-4 interdiff?

Cheers

	Andreas Pehrson [:pehrsons] Assignee
	Comment 40 • 7 years ago

Here's a basic try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=259c93e0fe6c232c44ae59d15839d934f73bf396
And here's a massive one per florian's wishes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed8de530d334b071b0233277f798ebf8a28aa62

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 41 • 7 years ago
mozreview-review

Comment on attachment 8855304 [details]
Bug 1320994 - Refactor MediaManager's window management.

https://reviewboard.mozilla.org/r/127172/#review136312

lgtm.

::: dom/media/MediaManager.cpp:1222
(Diff revisions 2 - 4)
>      RefPtr<Refcountable<UniquePtr<OnTracksAvailableCallback>>> tracksAvailableCallback =
> -      new Refcountable<UniquePtr<OnTracksAvailableCallback>>(
> -        new TracksAvailableCallback(mManager, mOnSuccess, mWindowID, domStream));
> +      MakeAndAddRef<Refcountable<UniquePtr<OnTracksAvailableCallback>>>(
> +        new TracksAvailableCallback(mManager, mOnSuccess.forget(), mWindowID, domStream));

Bug 1354642 is inbound now, so you can do:

    auto runnable = MakeRefPtr<Refcountable<UniquePtr<OnTracksAvailableCallback>>>
        tracksAvailableCallback(mManager, mOnSuccess.forget(), mWindowID, domStream));

::: dom/media/MediaManager.cpp:2721
(Diff revisions 2 - 4)
>                      uint64_t aWindowID,
>                      GetUserMediaWindowListener *aListener,
>                      void *aData)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> -  if (!aListener) {
> +  RefPtr<GetUserMediaWindowListener> listener(aListener);

Maybe a comment on why this needs a hard reference here?

	Pulsebot
	Comment 52 • 7 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6d4e1736fd69
Re-enable tests. r=florian
https://hg.mozilla.org/integration/autoland/rev/78e5f8775727
Refactor MediaManager's window management. r=jib
https://hg.mozilla.org/integration/autoland/rev/31afc3fcd61f
Remove chrome-test hacks that accomodated the misaligned MediaManager model. r=florian
https://hg.mozilla.org/integration/autoland/rev/5766ba7143b4
Unify MediaManager logging macros. r=jib
https://hg.mozilla.org/integration/autoland/rev/b744a070cafb
Improve SourceListener logging. r=jib

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 53 • 7 years ago

Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=94499603&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/f2c9825b0c32

	Pulsebot
	Comment 54 • 7 years ago

Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/745f2d85212d
Backed out 5 changesets for browser_downloads_panel_block.js permafails on Win7VM a=backout CLOSED TREE

	Andreas Pehrson [:pehrsons] Assignee
	Comment 55 • 7 years ago

Odd that it didn't show up in [1] :/

I think there's some kind of bug there. The bc3 that should have been affected didn't run the test that failed on autoland, and it shows this: `Untitled data: --this-chunk=1 --total-chunks=1 -- browser/base/content/test/webrtc`

Florian, you gave me the try command, do you know what's up?


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed8de530d33

	Florian Quèze [:florian] Reporter
	Comment 56 • 7 years ago

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #55)
> Odd that it didn't show up in [1] :/
> 
> I think there's some kind of bug there. The bc3 that should have been
> affected didn't run the test that failed on autoland, and it shows this:
> `Untitled data: --this-chunk=1 --total-chunks=1 --
> browser/base/content/test/webrtc`
> 
> Florian, you gave me the try command, do you know what's up?
> 
> 
> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ed8de530d33

I gave you a try syntax that runs only the webrtc tests, and runs them hundreds of times, to see the frequency of intermittent failures of these tests, ie. verify your patches were actually fixing the intermittent failure here. It wasn't meant to verify the patch wasn't breaking something unrelated to webrtc tests; sorry if that was confusing :-(.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 57 • 7 years ago

(In reply to Florian Quèze [:florian] [:flo] from comment #56)
> I gave you a try syntax that runs only the webrtc tests, and runs them
> hundreds of times, to see the frequency of intermittent failures of these
> tests, ie. verify your patches were actually fixing the intermittent failure
> here. It wasn't meant to verify the patch wasn't breaking something
> unrelated to webrtc tests; sorry if that was confusing :-(.

Right, I think I understand.

The bc1-X chunks were confusing indeed. I'd have expected them to be all marked as bc1 since all the webrtc tests fit in one chunk.

	Pulsebot
	Comment 58 • 7 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/82056ff7f987
Re-enable tests. r=florian
https://hg.mozilla.org/integration/autoland/rev/b17aaf7a8689
Refactor MediaManager's window management. r=jib
https://hg.mozilla.org/integration/autoland/rev/2f475f8d6ffc
Remove chrome-test hacks that accomodated the misaligned MediaManager model. r=florian
https://hg.mozilla.org/integration/autoland/rev/c60c97633ac8
Unify MediaManager logging macros. r=jib
https://hg.mozilla.org/integration/autoland/rev/cf74f8bacbc1
Improve SourceListener logging. r=jib

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 59 • 7 years ago

Backed out for frequently failing enabled browser_devices_get_user_media_screen.js on Linux in non-e10s mode:

https://hg.mozilla.org/integration/autoland/rev/c3d135ac1c03da5afd949b9ea546c44d097a6c08
https://hg.mozilla.org/integration/autoland/rev/c8e03edab8384923e651ffa9d476fbf4f3de892e
https://hg.mozilla.org/integration/autoland/rev/26a797b5b7401523d58eba0122dea1ce94949490
https://hg.mozilla.org/integration/autoland/rev/f1afffd60c9603308d970131052473b4587a56cc
https://hg.mozilla.org/integration/autoland/rev/e82d28a19247ef890515aa620c9042020dd0c6bb

Push showing the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2768a87790681efc0fe64df26c363ef7381759a6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=96180932&repo=autoland

[task 2017-05-03T10:23:31.986727Z] 10:23:31     INFO - TEST-PASS | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | the 'all windows will be shared' warning should now be visible - 
[task 2017-05-03T10:23:31.988674Z] 10:23:31     INFO - Buffered messages finished
[task 2017-05-03T10:23:31.990585Z] 10:23:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | Condition didn't pass. - 
[task 2017-05-03T10:23:31.992222Z] 10:23:31     INFO - Stack trace:
[task 2017-05-03T10:23:31.994441Z] 10:23:31     INFO - chrome://mochitests/content/browser/browser/base/content/test/webrtc/head.js:waitForCondition/interval<:15
[task 2017-05-03T10:23:31.997249Z] 10:23:31     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-05-03T10:23:31.999159Z] 10:23:31     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js | the preview area is visible - 
[task 2017-05-03T10:23:32.001861Z] 10:23:32     INFO - Stack trace:
[task 2017-05-03T10:23:32.004011Z] 10:23:32     INFO -     chrome://mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:checkScreenOnly:59
[task 2017-05-03T10:23:32.007142Z] 10:23:32     INFO -     runTests@chrome://mochitests/content/browser/browser/base/content/test/webrtc/head.js:535:11
[task 2017-05-03T10:23:32.009049Z] 10:23:32     INFO -     async*test@chrome://mochitests/content/browser/browser/base/content/test/webrtc/browser_devices_get_user_media_screen.js:577:9
[task 2017-05-03T10:23:32.010753Z] 10:23:32     INFO -     Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:757:21
[task 2017-05-03T10:23:32.012765Z] 10:23:32     INFO -     TaskImpl_run@resource://gre/modules/Task.jsm:319:42
[task 2017-05-03T10:23:32.016748Z] 10:23:32     INFO -     TaskImpl@resource://gre/modules/Task.jsm:277:3
[task 2017-05-03T10:23:32.018475Z] 10:23:32     INFO -     asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-05-03T10:23:32.029487Z] 10:23:32     INFO -     Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-05-03T10:23:32.031938Z] 10:23:32     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:752:9
[task 2017-05-03T10:23:32.036444Z] 10:23:32     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:672:7
[task 2017-05-03T10:23:32.039361Z] 10:23:32     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59

	Andreas Pehrson [:pehrsons] Assignee
	Comment 60 • 7 years ago

Hmm, that's new. I'll rebase and see if I can repro.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 61 • 7 years ago

Can't repro locally, linux64 debug non-e10s.

I think we got this for free when re-enabling this test on linux. It doesn't seem related to my MediaManager changes, so I'll take a shot and attribute it to timing sensitive UI-stuff (screenshots being inconsistent seem to indicate some such).

Florian, could you take a look? I have a try push isolating the issue at https://treeherder.mozilla.org/#/jobs?repo=try&revision=67585438a4f95fcd3a019bcc9f022e7a5a5f2fee.

If you can't make sense of it I'll put the disabling back for linux and refer to bug 1325261.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 62 • 7 years ago

I just tested the only thing I could think of, increasing the timeout, and it seems to have helped: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3efcf9cdc266592a916f7de28d58dab2a2b88ec

I'll work out a proper patch.

	Andreas Pehrson [:pehrsons] Assignee
	Comment 69 • 7 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e2498a95f2ec91c5d2fd0eecd88e232983894f8

	Florian Quèze [:florian] Reporter
	Comment 70 • 7 years ago
mozreview-review

Comment on attachment 8864587 [details]
Bug 1320994 - Increase retries for screensharing preview window.

https://reviewboard.mozilla.org/r/136266/#review139676

r=me to unblock this, but a clean solution would be to get rid of this waitForCondition completely. Eg. we could observe the recording-device-events notification to know when the preview stream has started (this is all happening in the parent process when displaying the preview, so it should be relatively easy to do).

	Pulsebot
	Comment 71 • 7 years ago

Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/bce76b2df8cc
Re-enable tests. r=florian
https://hg.mozilla.org/integration/autoland/rev/3d902e043261
Refactor MediaManager's window management. r=jib
https://hg.mozilla.org/integration/autoland/rev/2056ed360550
Remove chrome-test hacks that accomodated the misaligned MediaManager model. r=florian
https://hg.mozilla.org/integration/autoland/rev/ebd5f2bb5c59
Unify MediaManager logging macros. r=jib
https://hg.mozilla.org/integration/autoland/rev/d34041d4dfe9
Improve SourceListener logging. r=jib
https://hg.mozilla.org/integration/autoland/rev/1fb2d6e0aa2d
Increase retries for screensharing preview window. r=florian

	Iris Hsiao [:ihsiao]
	Comment 72 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/bce76b2df8cc
https://hg.mozilla.org/mozilla-central/rev/3d902e043261
https://hg.mozilla.org/mozilla-central/rev/2056ed360550
https://hg.mozilla.org/mozilla-central/rev/ebd5f2bb5c59
https://hg.mozilla.org/mozilla-central/rev/d34041d4dfe9
https://hg.mozilla.org/mozilla-central/rev/1fb2d6e0aa2d

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 73 • 7 years ago

Is this something that needs manual testing? 
And, is it ok for this fix to go to release in 55 or are you hoping to get these patches into 54?

	Andreas Pehrson [:pehrsons] Assignee
	Comment 74 • 7 years ago

No manual testing needed, we have good coverage on this functionality.
And yes, 55 is fine. 54 is too risky.

Thanks!