1100502 - about:webrtc Start Debug Log/etc don't work on Nightly (e10s)

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

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

start debug log (and start AEC log I think) don't work on Nightly, perhaps only in e10s.  However, it might be a more general/existing problem as I saw some react errors:

JavaScript strict warning: chrome://browser/content/utilityOverlay.js, line 144: ReferenceError: reference to undefined property e.button
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 7209: ReferenceError: reference to undefined property Constructor.propTypes
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 6180: ReferenceError: reference to undefined property Constructor.propTypes
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 16543: ReferenceError: reference to undefined property event.fromElement
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 772: ReferenceError: reference to undefined property elem.nodeName
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 18886: ReferenceError: reference to undefined property elem.nodeName
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 1005: ReferenceError: reference to undefined property elem.nodeName
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 2787: ReferenceError: reference to undefined property bankForRegistrationName[id]
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 5527: ReferenceError: reference to undefined property nextDescriptor.props.ref
JavaScript strict warning: chrome://browser/content/loop/shared/libs/react-0.11.2.js, line 19769: ReferenceError: reference to undefined property prevDescriptor.props.key

Comment 1

[Maire Reavy [:mreavy]]

Hi Paul, Fixing this is going to be important for the debug tool we want for this quarter; so I've made this block bug 1100508 and reassigned it to you.

Comment 2

[Paul Kerr [:pkerr]]

When running Hello on OS X Nightly and calling yourself on the single host, the call to WebrtcGlobalInformation only returns information on one pair of the sessions set up by Hello. (The TokBox library sets up both a send-only and recv-only WebRTC session for each room/call participant so four total peer-connections should be listed.) It appears to be the session pair of the first end to join the room: Hello app or the stand-alone app. If e10s is disabled, both pairs of peer-connections are represented in the data returned by the call to WebrtcGlobalInformation.

Comment 3

[Byron Campen [:bwc]]

So the problem here is that WebrtcGlobalInformation resides in the child process. about:webrtc will have stuff from whatever child process it happens to be in, but not other child processes. To really fix this, we would need to move WebrtcGlobalInformation to the parent process, and set up IPC to handle access (both reads and writes).

Comment 4

[Paul Kerr [:pkerr]]

Attachment: WIP not for checkin

Work in progress

Comment 5

[Paul Kerr [:pkerr]]

Attachment: WIP not for checkin

working AEC and Debug mode controls

Comment 6

[Paul Kerr [:pkerr]]

Attachment: WIP not for checkin

Comment 7

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix

fixed protocol shutdown sequence issue

Comment 8

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix

bit rot handled

Comment 9

[Paul Kerr [:pkerr]]

Change summary:

Most of the changes can be found in WebrtcGlobalInformation.cpp. Each request: GetAllStats, GetLoggging, SetDebugLevel and SetAecDebug now gathers information from or send commands to the chrome process and one (or more) content processes that have an instance of PeerConnectionCtx. (The WebrtcGlobalInformation binding is only available in chrome javascript.) This is done using a StatsRequest or LogRequest class instance used to keep track of the WebrtcGlobalChild instances from which to request a stats report or a log, along will collecting the data from each child then the chrome process before executing the callback to the javascript requestor.

Two small changes have been make to PeerConntionCtx.cpp. First, it creates an IPC PWebrtcGlobal protocol instance when it is created in a non-gecko process. The second change is to add a GetPeerConnections() getter to allow both the WebrtcGlobalInformation class and WebrtcChild/Parent classes access to the list of live PeerConnectionImpls in each process.

New stuff has been added to create the PWebrtcGlobal IPC protocol. This runs as a protocol managed by PContent. This is the pipe used to send commands and extract results from content processes. It lasts for the lifetime of a PeerConnectionCtx instance. WebrtcGlboal.h contains serialization templates that allow the RTCStatsReportInternal data structure to be passed through IPC.

Comment 10

[Paul Kerr [:pkerr]]

Comment on attachment 8593741 [details] [diff] [review]
about:webrtc e10s fix

Could you give me feedback on the IPC and general e10s handling that I have added to the code behind about:webrtc. This is my first experience with e10s and IPC; I am looking for feedback and guidance before I wrap this up for review.

Comment 11

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix

Added mutexes to handle threaded access to Request maps and id generation.

Comment 12

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix

Added RefCounting to PWebrtcGlobalParent implementation. Fixed Linux build issue.

Comment 13

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix

mochitest process shutdown fix

Comment 14

[Paul Kerr [:pkerr]]

Comment on attachment 8595619 [details] [diff] [review]
about:webrtc e10s fix

Requesting feedback in IPC and e10s operation from e10s peer.
Requesting review on changes to WebRTC code from platform peer.

Comment 15

[Paul Kerr [:pkerr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c846e5dc2db4

Comment 16

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

Comment on attachment 8595619 [details] [diff] [review]
about:webrtc e10s fix

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

r+, though resolving the duplication would be nice.  If so, a part 2 or include an interdiff for re-review (should be easy).

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobal.h
@@ +9,5 @@
> +#include "mozilla/dom/BindingDeclarations.h"
> +#include "mozilla/dom/RTCStatsReportBinding.h"
> +
> +typedef mozilla::dom::RTCStatsReportInternal StatsReport;
> +typedef nsTArray< nsAutoPtr< StatsReport > > RTCReports;

remove extra spaces

@@ +50,5 @@
> +    return true;
> +  }
> +};
> +
> +#if 0 //FIXME(PRK) verify that the fast form works on different platform builds

Is this resolved?  Either remove, or file a followup

::: media/webrtc/signaling/src/peerconnection/WebrtcGlobalInformation.cpp
@@ +47,5 @@
> +class StatsRequest
> +{
> +public:
> +  typedef nsMainThreadPtrHandle<WebrtcGlobalStatisticsCallback> Callback;
> +  std::queue<RefPtr<WebrtcGlobalParent> > mContactList;

> > is no longer needed since we updated compilers some time ago
>>

@@ +76,5 @@
> +    mStats.mReports.Construct();
> +  }
> +};
> +
> +mozilla::StaticMutex StatsRequest::sMutex;

comment what it protects

@@ +93,5 @@
> +  if (!result.second) {
> +    return nullptr;
> +  }
> +
> +  return &(result.first)->second;

result.first.second doesn't work?

@@ +145,5 @@
> +class LogRequest
> +{
> +public:
> +  typedef nsMainThreadPtrHandle<WebrtcGlobalLoggingCallback> Callback;
> +  std::queue<RefPtr<WebrtcGlobalParent> > mContactList;

>>

@@ +159,5 @@
> +  RefPtr<WebrtcGlobalParent> GetNextParent();
> +  void Complete();
> +
> +private:
> +  static mozilla::StaticMutex sMutex;

comment what's protected

@@ +172,5 @@
> +    , mPattern(aPattern)
> +  {}
> +};
> +
> +mozilla::StaticMutex LogRequest::sMutex;

repeat comment

@@ +189,5 @@
> +  if (!result.second) {
> +    return nullptr;
> +  }
> +
> +  return &(result.first)->second;

ditto -- there's a seriously repeated pattern here with LoqRequest and StatsRequest; perhaps unify?  ManageRequest<LoqRequest> and ManageRequest<StatsRquest>?

@@ +375,4 @@
>      Sequence<nsString> nsLogs;
> +
> +    if (!aLogList->empty()) {
> +      for (auto& l : *aLogList) {

Would prefer something more readable than 'l'

@@ +396,4 @@
>    }
> +
> +  if (!aLogList->empty()) {
> +    for (auto& l : *aLogList) {

ditto

@@ +592,5 @@
> +  // destroyed on main.
> +  LogRequest::Callback callbackHandle(
> +    new nsMainThreadPtrHolder<WebrtcGlobalLoggingCallback>(&aLoggingCallback));
> +
> +  //std::string pattern(NS_ConvertUTF16toUTF8(aPattern).get());

?

@@ +728,5 @@
> +  // Content queries complete, run chrome instance query if applicable
> +  nsresult rv = RunLogQuery(request->mPattern, nullptr, aRequestId);
> +
> +  if (NS_FAILED(rv)) {
> +    //Unable to get gecko process log. Return what has been collected.

"// "

Comment 17

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix. Content and chrome connections are reported

updated based on review feedback

Comment 18

[Paul Kerr [:pkerr]]

Attachment: about:webrtc e10s fix. Content and chrome connections are reported

Comment 19

[Paul Kerr [:pkerr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a03210953e7

Comment 20

[Paul Kerr [:pkerr]]

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

Comment 21

[Paul Kerr [:pkerr]]

Comment on attachment 8598272 [details] [diff] [review]
about:webrtc e10s fix. Content and chrome connections are reported

Carry forward r=rjesup.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Daniel Holbert [:dholbert]]

This broke --disable-webrtc builds -- see bug 1159320.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Why did the code in ParamTraits<mozilla::dom::Sequence<T>> need dynamic_cast?

Comment 26

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

Sorry, missed this one.  Paul left a year ago or so.  I'm not sure about dynamic_cast here...
I'll file a bug to remove it (builds green on try)