Re: Fix issue of localDescription and remoteDescription getter. (issue 1010393002 by changbin.shao@intel.com)

[joc...@chromium.org]

+jl for bindings / idl

i'm not entirely sure what the semantics of the RTC objects is supposed to
be
:-/

https://codereview.chromium.org/1010393002/

[j...@opera.com]

On 2015/04/17 11:26:46, jochen wrote:
> RTCSessionDescription should be an active dom object to keep its wrapper
> alive

+haraken, so he can correct me if I'm totally wrong.

It doesn't really seem to me that making RTCSessionDescription an
ActiveDOMObject is the right approach. It's not really active, AFAICT; it's
just
referenced from another object (that happens to be active.)

Looks to me like doing anything other than simply returning the same
RTCSessionDescription pointer from the getter (e.g.
RTCPeerConnection::localDescription()) every time should be enough. The IDL
compiler automatically adds code to keep the V8 wrapper alive by adding a
reference from the RTCPeerConnection wrapper to the RTCSessionDescription
wrapper.


https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode347
Source/modules/mediastream/RTCPeerConnection.cpp:347:
m_localDescription->setWebSessionDescription(webSessionDescription);
I think the semantics might be somewhat incorrect here (and in
setLocalDescription() + same for remote).

I don't know what might make the description change (since I don't know
WebRTC at all) but if it were to change, it seems more reasonable to
return a new RTCSessionDescription object, instead of mutating the one
we have, and might have returned earlier.

The spec definitely says to return the last description successfully via
setLocalDescription(), which implies that setLocalDescription() should
make this setter later return a different object. That is, that
setLocalDescription() should simply store the description argument in
m_localDescription so that we can return it later.

And then we should perhaps have an update mechanism here so that if we
have an existing description in m_localDescription but what
m_peerHandler->localDescription() returns doesn't match it, that we
create a new RTCSessionDescription object with that information and
store it in m_localDescription.

This is all somewhat complicated though by the fact that
RTCSessionDescription objects aren't immutable, meaning whatever
description we return to the script (or that a script sets via
setLocalDescription()) could later be modified directly by the script,
which the specification doesn't mention (or at least not where I'd
expect to find it mentioned.)

https://codereview.chromium.org/1010393002/

[j...@opera.com]

> +haraken, so he can correct me if I'm totally wrong.

And now actually adding haraken as reviewer.

https://codereview.chromium.org/1010393002/

[har...@chromium.org]

On 2015/04/28 11:00:13, Jens Widell wrote:
> > +haraken, so he can correct me if I'm totally wrong.

> And now actually adding haraken as reviewer.

Would you help me understand what the problem is? :)

> It doesn't really seem to me that making RTCSessionDescription an
> ActiveDOMObject is the right approach. It's not really active, AFAICT;
> it's
just
> referenced from another object (that happens to be active.)

FWIW, I believe that hasPendingActivity should be a method of
ScriptWrappable,
not ActiveDOMObject. If you're using ActiveDOMObject just to use
hasPendingActivity and feel it strange, that's because our architecture is
just
wrong.


https://codereview.chromium.org/1010393002/

[j...@opera.com]

On 2015/04/28 11:18:35, haraken wrote:
> On 2015/04/28 11:00:13, Jens Widell wrote:
> > > +haraken, so he can correct me if I'm totally wrong.
> >
> > And now actually adding haraken as reviewer.

> Would you help me understand what the problem is? :)

Basically a set of attribute getters that currently return different objects
every time they're accessed but should return the same, and more
specifically
how to implement returning the same object every time in such a way that
the V8
wrapper is also kept around, so the same actual V8 object is returned every
time.

https://codereview.chromium.org/1010393002/

[har...@chromium.org]

Does it mean that the wrapper is kept alive forever even though the DOM
object
is gone? Won't it cause a leak?

If the wrapper lifetime is determined by some DOM thing, I think it would be
reasonable to use ActiveDOMObject::hasPendingActivity(), which is a
mechanism to
keep a wrapper alive while DOM is doing something.


https://codereview.chromium.org/1010393002/

[j...@opera.com]

Yes, I guess RTCPeerConnection here (which is the object with the getters)
is
active. But the RTCSessionDescription objects returned by the getters are
simple, stupid data containers. If they're made active too, then their
"activity" would simply be that of the owning RTCPeerConnection, so you'd
just
have one complex object keeping two simple objects alive. And
ActiveDOMObject
seems like an odd approach for that.

Also, RTCPeerConnection should only stay alive unreferenced while it's
active,
but it should keep the RTCSessionDescription objects alive regardless of
whether
there's activity. That is, and inactive RTCPeerConnection object,
referenced by
a script, should still return the same RTCSessionDescription objects from
its
getters.

https://codereview.chromium.org/1010393002/

[har...@chromium.org]

Can we use [SetWrapperReferenceTo] or [SetWrapperRefernceFrom] to indicate
that
RTCSessionDescription needs to be alive as long as RTCPeerConnection is
alive?


https://codereview.chromium.org/1010393002/

[j...@opera.com]

Yeah, I guess

[
SetWrapperReferenceTo(RTCSessionDescription localDescription,
RTCSessionDescription remoteDescription)
] interface RTCPeerConnection {

would work, except that those attributes have [RaisesException=Getter] and
thus
needs an ExceptionState. (Which in this case happens to be easily fixed by
dropping [RaisesException]; the getters don't actually raise any
exceptions.)

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

Hi, all, thanks a lot for your detailed review comments!
I have updated patch to use [SetWrapperReferenceTo] to make
RTCSessionDescription and RTCPeerConnection share the same life cycle,
please
talk a look.

https://codereview.chromium.org/1010393002/

[j...@opera.com]

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode369
Source/modules/mediastream/RTCPeerConnection.cpp:369:
m_remoteDescription = sessionDescription;
I'm unsure exactly how things work here, but I suspect it would be more
correct to delay this setting (and the one in setLocalDescription()) to
when RTCVoidRequestImpl::requestSucceeded() is called.

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

Update CL. PTAL.

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode369
Source/modules/mediastream/RTCPeerConnection.cpp:369:
m_remoteDescription = sessionDescription;

Agree and thanks:)
Current implemention will give a wrong value for m_remoteDescription if
peer handler fails to set inside. Though it's fixed by the
remoteDescription getter function.

https://codereview.chromium.org/1010393002/

[j...@opera.com]

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode369
Source/modules/mediastream/RTCPeerConnection.cpp:369:
m_remoteDescription = sessionDescription;

On 2015/05/04 10:12:23, changbin wrote:
> Agree and thanks:)
> Current implemention will give a wrong value for m_remoteDescription
if peer
> handler fails to set inside. Though it's fixed by the
remoteDescription getter
> function.

Now you no longer keep the object passed to setRemoteDescription(). My
reading of the specification is that the getter should return that
actual object (once successfully set), rather than an object identical
to it.

You could store it in an m_pendingRemoteDescription here and then move
that over to m_remoteDescription once the description has been updated
successfully.

But really, an owner of this code should comment on these details,
instead of me.

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

PTAL. jl@

Also ping the owners.

https://codereview.chromium.org/1010393002/

[j...@opera.com]

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode644
Source/modules/mediastream/RTCPeerConnection.cpp:644: if (pending) {
This code path has essentially nothing in common with the rest of the
function, so I'd just make this bit its own (very simple) function,
called commitPendingLocalSessionDescription() or something like that.

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode63
Source/modules/mediastream/RTCVoidRequestImpl.cpp:63:
m_requester->updateLocalSessionDescriptionIfNeeded(true);
To be completely correct, I think you should keep track of whether the
request is about updating the local or remote description (or neither)
and only update the right one.

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode644
Source/modules/mediastream/RTCPeerConnection.cpp:644: if (pending) {

On 2015/05/05 06:31:20, Jens Widell wrote:
> This code path has essentially nothing in common with the rest of the
function,
> so I'd just make this bit its own (very simple) function, called
> commitPendingLocalSessionDescription() or something like that.

Therefore, your proposal would be,
1. Move out 'if(pending)' logic into a separate function, named
commitPendingLocalSessionDescription().
2. Move updateLocalSessionDescriptionIfNeeded() codes back in
localDescription() getter.
3. In RTCVoidRequestImpl::requestSucceeded(), call
commitPendingLocalSessionDescription().

Is my understanding correct?

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode63
Source/modules/mediastream/RTCVoidRequestImpl.cpp:63:
m_requester->updateLocalSessionDescriptionIfNeeded(true);

On 2015/05/05 06:31:20, Jens Widell wrote:
> To be completely correct, I think you should keep track of whether the
request
> is about updating the local or remote description (or neither) and
only update
> the right one.

I see. Then how about add argument |request_type| for RTCVoidRequestImpl
creator, and update local or remote description, or nothing per
|request_type| value?

https://codereview.chromium.org/1010393002/

[j...@opera.com]

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp
File Source/modules/mediastream/RTCPeerConnection.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCPeerConnection.cpp#newcode644
Source/modules/mediastream/RTCPeerConnection.cpp:644: if (pending) {

On 2015/05/05 07:02:29, changbin wrote:
> On 2015/05/05 06:31:20, Jens Widell wrote:
> > This code path has essentially nothing in common with the rest of
the
> function,
> > so I'd just make this bit its own (very simple) function, called
> > commitPendingLocalSessionDescription() or something like that.

> Therefore, your proposal would be,
> 1. Move out 'if(pending)' logic into a separate function, named
> commitPendingLocalSessionDescription().
> 2. Move updateLocalSessionDescriptionIfNeeded() codes back in
localDescription()
> getter.
> 3. In RTCVoidRequestImpl::requestSucceeded(), call
> commitPendingLocalSessionDescription().

> Is my understanding correct?

Correct.

Though 2 is somewhat optional. Keeping this helper might make sense
simply for the purpose of having a function name that documents what's
going on. Or not. I have no strong opinion. :-)

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode63
Source/modules/mediastream/RTCVoidRequestImpl.cpp:63:
m_requester->updateLocalSessionDescriptionIfNeeded(true);

On 2015/05/05 07:02:29, changbin wrote:
> On 2015/05/05 06:31:20, Jens Widell wrote:
> > To be completely correct, I think you should keep track of whether
the request
> > is about updating the local or remote description (or neither) and
only update
> > the right one.

> I see. Then how about add argument |request_type| for
RTCVoidRequestImpl
> creator, and update local or remote description, or nothing per
|request_type|
> value?

Something like that, yes. But then it might actually make more sense to
simply have "m_requester->requestSucceeded(request_type)" here, and thus
let RTCPeerConnection decide what to do about it.

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp
File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right):

https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastream/RTCVoidRequestImpl.cpp#newcode63
Source/modules/mediastream/RTCVoidRequestImpl.cpp:63:
m_requester->updateLocalSessionDescriptionIfNeeded(true);

On 2015/05/05 07:17:22, Jens Widell wrote:
> On 2015/05/05 07:02:29, changbin wrote:
> > On 2015/05/05 06:31:20, Jens Widell wrote:
> > > To be completely correct, I think you should keep track of whether
the
> request
> > > is about updating the local or remote description (or neither) and
only
> update
> > > the right one.
> >
> > I see. Then how about add argument |request_type| for
RTCVoidRequestImpl
> > creator, and update local or remote description, or nothing per
|request_type|
> > value?

> Something like that, yes. But then it might actually make more sense
to simply
> have "m_requester->requestSucceeded(request_type)" here, and thus let
> RTCPeerConnection decide what to do about it.

Yeah, that could be another option. But I'm afraid it brings more
efforts than the first option. There might be two concerns,
1. Do we actually want to introduce |request_type| for the interface
|RTCVoidRequest|?
2. Need to add change in Chromium implementation to send request_Type in
'm_requester->requestSucceeded(request_type)'.
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/media/rtc_peer_connection_handler.cc&l=330

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

Sorry that I misunderstand your idea. Will change per your suggestion:)

https://codereview.chromium.org/1010393002/

[changb...@intel.com]

PTAL.

https://codereview.chromium.org/1010393002/

[j...@opera.com]

LGTM

https://codereview.chromium.org/1010393002/