Closed
Bug 1100502
Opened 10 years ago
Closed 10 years ago
about:webrtc Start Debug Log/etc don't work on Nightly (e10s)
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: jesup, Assigned: pkerr)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 9 obsolete files)
(deleted),
patch
|
pkerr
:
review+
|
Details | Diff | Splinter Review |
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
Updated•10 years ago
|
tracking-e10s:
--- → +
Comment 1•10 years ago
|
||
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.
Assignee: rgauthier → pkerr
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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).
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•10 years ago
|
Attachment #8591942 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8592003 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8593025 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8593145 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
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.
Assignee | ||
Comment 10•10 years ago
|
||
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.
Attachment #8593741 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 11•10 years ago
|
||
Added mutexes to handle threaded access to Request maps and id generation.
Assignee | ||
Updated•10 years ago
|
Attachment #8594236 -
Flags: feedback?(wmccloskey)
Assignee | ||
Updated•10 years ago
|
Attachment #8593741 -
Attachment is obsolete: true
Attachment #8593741 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 12•10 years ago
|
||
Added RefCounting to PWebrtcGlobalParent implementation. Fixed Linux build issue.
Assignee | ||
Updated•10 years ago
|
Attachment #8594956 -
Flags: feedback?(wmccloskey)
Assignee | ||
Updated•10 years ago
|
Attachment #8594236 -
Attachment is obsolete: true
Attachment #8594236 -
Flags: feedback?(wmccloskey)
Assignee | ||
Updated•10 years ago
|
Attachment #8594956 -
Attachment is obsolete: true
Attachment #8594956 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 14•10 years ago
|
||
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.
Attachment #8595619 -
Flags: review?(rjesup)
Attachment #8595619 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 15•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c846e5dc2db4
Reporter | ||
Comment 16•10 years ago
|
||
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. "// "
Attachment #8595619 -
Flags: review?(rjesup) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8595619 -
Attachment is obsolete: true
Attachment #8595619 -
Flags: feedback?(wmccloskey)
Assignee | ||
Comment 18•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8598265 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a03210953e7
Assignee | ||
Comment 20•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ceef1cb43b32
Assignee | ||
Comment 21•10 years ago
|
||
Comment on attachment 8598272 [details] [diff] [review] about:webrtc e10s fix. Content and chrome connections are reported Carry forward r=rjesup.
Attachment #8598272 -
Flags: review+
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ceef1cb43b32
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 25•7 years ago
|
||
Why did the code in ParamTraits<mozilla::dom::Sequence<T>> need dynamic_cast?
Flags: needinfo?(rjesup)
Flags: needinfo?(paulrkerr)
Reporter | ||
Comment 26•7 years ago
|
||
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)
Flags: needinfo?(rjesup)
Flags: needinfo?(paulrkerr)
You need to log in
before you can comment on or make changes to this bug.
Description
•