Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1010393002:
Fix issue of localDescription and remoteDescription getter. (Closed)
|
Created:
5 years, 9 months ago by changbin Modified:
5 years, 7 months ago Reviewers:
Tommy Widenflycht, Yuki, haraken, jochen (gone - plz use gerrit), Jens Widell, tommi (sloooow) - chröme, bashi CC:
blink-reviews, tommyw+watchlist_chromium.org, blink-reviews-bindings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionFix issue of localDescription and remoteDescription getter.
Current implementation returns a new RTCSessionDescription instance
when get localDescription and remoteDescription value. This causes error
of Javascript equality check.
The CL fixes above issue.
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195576
Patch Set 1 #Patch Set 2 : ActiveDomObject. #
Total comments: 3
Patch Set 3 : Use SetWrapperReferenceTo. #
Total comments: 3
Patch Set 4 : Set sessionDescription value when RTCVoidRequestImpl::requestSucceeded(). #Patch Set 5 : Add m_pendingLocalDescription. #
Total comments: 7
Patch Set 6 : Update. #
Total comments: 2
Patch Set 7 : Rebease and add GC in test case. #Patch Set 8 : Fix test case crash. #Messages
Total messages: 52 (7 generated)
changbin.shao@intel.com changed reviewers: + tommyw@chromium.org
PTAL. Thanks! Step to reproduce the issue, 1. open Chrome debug console, 2. # 'var p = new webkitRTCPeerConnection(null)' # 'var q = p.localDescription' # 'q === p.localDescription' it returns false.
PTAL. Thanks! Step to reproduce the issue, 1. open Chrome debug console, 2. # 'var p = new webkitRTCPeerConnection(null)' # 'var q = p.localDescription' # 'q === p.localDescription' it returns false.
ping
changbin.shao@intel.com changed reviewers: + jochen@chromium.org, tommi@chromium.org
Hi all, do you have any comments? This issue blocks our downstream testing.
RTCSessionDescription should be an active dom object to keep its wrapper alive
On 2015/04/17 11:26:46, jochen wrote: > RTCSessionDescription should be an active dom object to keep its wrapper alive hi jochen@ I don't understand your comments, could you please give some explanations? Thanks a lot!
On 2015/04/20 at 01:55:24, changbin.shao wrote: > On 2015/04/17 11:26:46, jochen wrote: > > RTCSessionDescription should be an active dom object to keep its wrapper alive > > hi jochen@ > I don't understand your comments, could you please give some explanations? Thanks a lot! there's some doc about this here: https://www.chromium.org/blink/activedomobject
On 2015/04/20 at 01:55:24, changbin.shao wrote: > On 2015/04/17 11:26:46, jochen wrote: > > RTCSessionDescription should be an active dom object to keep its wrapper alive > > hi jochen@ > I don't understand your comments, could you please give some explanations? Thanks a lot! there's some doc about this here: https://www.chromium.org/blink/activedomobject
On 2015/04/20 06:49:18, jochen wrote: > On 2015/04/20 at 01:55:24, changbin.shao wrote: > > On 2015/04/17 11:26:46, jochen wrote: > > > RTCSessionDescription should be an active dom object to keep its wrapper > alive > > > > hi jochen@ > > I don't understand your comments, could you please give some explanations? > Thanks a lot! > > there's some doc about this here: https://www.chromium.org/blink/activedomobject Thanks for the doc, jochen! so do you mean that I should make RTCSessionDescription be an active dom object and remain previous way to get localDescription and remoteDescription value in RTCPeerConnection, right?
On 2015/04/21 at 06:07:09, changbin.shao wrote: > On 2015/04/20 06:49:18, jochen wrote: > > On 2015/04/20 at 01:55:24, changbin.shao wrote: > > > On 2015/04/17 11:26:46, jochen wrote: > > > > RTCSessionDescription should be an active dom object to keep its wrapper > > alive > > > > > > hi jochen@ > > > I don't understand your comments, could you please give some explanations? > > Thanks a lot! > > > > there's some doc about this here: https://www.chromium.org/blink/activedomobject > > Thanks for the doc, jochen! > so do you mean that I should make RTCSessionDescription be an active dom object > and remain previous way to get localDescription and remoteDescription value in RTCPeerConnection, right? right, that's the way we solve such lifetime issues usually
Try to make RTCSessionDescription an active dom object. PTAL. jochen@ Could you please point out whether the change is heading on the correct path? Thanks!
https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescription.cpp (right): https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescription.cpp:110: return false; hasPendingActivity should return true as long as this object can potentially be exposed to javascript. note that it won't die as long as it returns true, so you can't just blanket return true here
https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastr... File Source/modules/mediastream/RTCSessionDescription.cpp (right): https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastr... Source/modules/mediastream/RTCSessionDescription.cpp:110: return false; On 2015/04/22 14:48:40, jochen wrote: > hasPendingActivity should return true as long as this object can potentially be > exposed to javascript. > > note that it won't die as long as it returns true, so you can't just blanket > return true here Then one factor that determines RTCSessionDescription object's life is RTCPeerConnection, right? I mean if RTCPeerConnection is no long alive, then its member m_localDescription and m_localDescription should also be released. If above is true, then why do we make RTCSessionDescription an active dom object? Since RTCPeerConnection is active dom object, I think its member values won't be released until the object isn't alive, is that right? Please forgive me that I am still confused on why have to make RTCPeerConnection active dom object:)
jochen@chromium.org changed reviewers: + jl@opera.com
+jl for bindings / idl i'm not entirely sure what the semantics of the RTC objects is supposed to be :-/
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/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/20001/Source/modules/mediastr... 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.)
jl@opera.com changed reviewers: + haraken@chromium.org
> +haraken, so he can correct me if I'm totally wrong. And now actually adding haraken as reviewer.
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.
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.
On 2015/04/28 11:21:45, Jens Widell wrote: > 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. 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.
On 2015/04/28 11:26:43, haraken wrote: > On 2015/04/28 11:21:45, Jens Widell wrote: > > 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. > > 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. 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.
On 2015/04/28 11:30:46, Jens Widell wrote: > On 2015/04/28 11:26:43, haraken wrote: > > On 2015/04/28 11:21:45, Jens Widell wrote: > > > 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. > > > > 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. > > 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. Can we use [SetWrapperReferenceTo] or [SetWrapperRefernceFrom] to indicate that RTCSessionDescription needs to be alive as long as RTCPeerConnection is alive?
On 2015/04/28 11:39:44, haraken wrote:
> On 2015/04/28 11:30:46, Jens Widell wrote:
> > On 2015/04/28 11:26:43, haraken wrote:
> > > On 2015/04/28 11:21:45, Jens Widell wrote:
> > > > 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.
> > >
> > > 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.
> >
> > 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.
>
> Can we use [SetWrapperReferenceTo] or [SetWrapperRefernceFrom] to indicate
that
> RTCSessionDescription needs to be alive as long as RTCPeerConnection is alive?
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.)
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/diff/40001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastr... 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.
Update CL. PTAL. https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastr... 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/diff/40001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/40001/Source/modules/mediastr... 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.
PTAL. jl@ Also ping the owners.
https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/diff/80001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/diff/80001/Source/modules/mediastr... File Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/diff/80001/Source/modules/mediastr... File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right): https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... 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/m...
On 2015/05/05 07:32:19, changbin wrote: > https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... > File Source/modules/mediastream/RTCVoidRequestImpl.cpp (right): > > https://codereview.chromium.org/1010393002/diff/80001/Source/modules/mediastr... > 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/m... Sorry that I misunderstand your idea. Will change per your suggestion:)
PTAL.
LGTM
jl is happy so this CL lgtm
lgtm
LGTM https://codereview.chromium.org/1010393002/diff/100001/LayoutTests/fast/media... File LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html (right): https://codereview.chromium.org/1010393002/diff/100001/LayoutTests/fast/media... LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html:37: function requestSucceeded1() You might want to call a GC to verify that the GC won't mis-collect the localDescription object. https://codereview.chromium.org/1010393002/diff/100001/LayoutTests/fast/media... File LayoutTests/fast/mediastream/RTCPeerConnection-remoteDescription.html (right): https://codereview.chromium.org/1010393002/diff/100001/LayoutTests/fast/media... LayoutTests/fast/mediastream/RTCPeerConnection-remoteDescription.html:40: Ditto.
Add GC test in test case to test whether localDescription is mis-collected.
However, it encountered crash in render process. Remove the
[SetWrapperReferenceTo] lines in RTCPeerConnection.idl will solve the crash
issue.
haraken@, could you give some hint for this? Thanks!
Error message:
Fatal error in ../../v8/src/global-handles.cc, line 1032
# Check failed: !Node::FromLocation(child)->is_independent().
Dump stack:
0 content_shell + 0x1da28ef
rax = 0x0000000000000000 rdx = 0x0000000000000000
rcx = 0xffffffffffffffff rbx = 0x0000000006fc0858
rsi = 0x00007fd04765a9d0 rdi = 0x00007fd0476591c0
rbp = 0x0000000000000408 rsp = 0x00007fff4ada5780
r8 = 0x00007fd04e653a00 r9 = 0x0000000000000001
r10 = 0x00007fd047656be0 r11 = 0x0000000000000000
r12 = 0x0000066191970780 r13 = 0x0000000000000002
r14 = 0x0000000006fa43d6 r15 = 0x00007fd047659868
rip = 0x00000000021a28ef
Found by: given as instruction pointer in context
1 content_shell + 0x1d9e3fc
rbx = 0x0000000006fc0858 rbp = 0x0000000000000408
rsp = 0x00007fff4ada5790 r12 = 0x0000066191970780
r13 = 0x0000000000000002 r14 = 0x0000000006fa43d6
r15 = 0x00007fd047659868 rip = 0x000000000219e3fc
Found by: call frame info
2 content_shell!SetReference [global-handles.cc : 1032 + 0x21]
rbx = 0x0000066191970780 rbp = 0x00007fff4ada58d0
rsp = 0x00007fff4ada5880 r12 = 0x0000066191970780
r13 = 0x0000000000000002 r14 = 0x00000661918d38e0
r15 = 0x0000066191970800 rip = 0x0000000001c0c68a
Found by: call frame info
3 content_shell + 0x203eba2
rbx = 0x0000066191970780 rbp = 0x00007fff4ada58d0
rsp = 0x00007fff4ada58b0 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000243eba2
Found by: call frame info
4 content_shell + 0x203ead9
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5900
rsp = 0x00007fff4ada58e0 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000243ead9
Found by: call frame info
5 content_shell + 0x203ea7a
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5940
rsp = 0x00007fff4ada5910 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000243ea7a
Found by: call frame info
6 content_shell + 0x203c3b1
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5970
rsp = 0x00007fff4ada5950 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000243c3b1
Found by: call frame info
7 content_shell!visitDOMWrapper [V8RTCPeerConnection.cpp : 956 + 0x8]
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5a50
rsp = 0x00007fff4ada5980 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x00000000024fdad5
Found by: call frame info
8 content_shell + 0x323a237
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5a90
rsp = 0x00007fff4ada5a60 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000363a237
Found by: call frame info
9 content_shell!VisitPersistentHandle [V8GCController.cpp : 288 + 0x16]
rbx = 0x0000066191970780 rbp = 0x00007fff4ada5bf0
rsp = 0x00007fff4ada5aa0 r12 = 0x000000000000003b
r13 = 0x0000000000000002 r14 = 0x00007fff4ada5c48
r15 = 0x0000066191970020 rip = 0x000000000363a07c
haraken@chromium.org changed reviewers: + bashi@chromium.org, yukishiino@chromium.org
The problem is that a DOM object that is pointed to by [SetWrapperReferenceTo]
(i.e., RTCSessionDescription) needs to be a dependent object. However, it seems
that RTCSessionDescription is marked as independent. This seems to be a bug of
the IDL compiler.
("Independent" means that there is no DOM-side reference to the wrapper object.
[SetWrapperReferenceTo B] on A means that there is a DOM-side reference from A
to B. Thus B must be a dependent object.)
bashi-san or shiino-san: Would you mind taking a look at this?
On 2015/05/19 09:28:01, haraken wrote:
> The problem is that a DOM object that is pointed to by [SetWrapperReferenceTo]
> (i.e., RTCSessionDescription) needs to be a dependent object. However, it
seems
> that RTCSessionDescription is marked as independent. This seems to be a bug of
> the IDL compiler.
>
> ("Independent" means that there is no DOM-side reference to the wrapper
object.
> [SetWrapperReferenceTo B] on A means that there is a DOM-side reference from A
> to B. Thus B must be a dependent object.)
>
> bashi-san or shiino-san: Would you mind taking a look at this?
I don't know well in this area, but the current implementation marks an
interface as dependent lifetime if it's explicitly annotated with one of:
- [DependentLifetime]
- [ActiveDOMObject]
- [SetWrapperReferenceFrom]
- [SetWrapperReferenceTo]
I don't yet understand what exactly [SetWrapperReference{From,To}] do, but my qq
is "don't we need to put [SetWrapperReferenceFrom] at
RTCSessionDescription.idl?".
Otherwise, we could use [DependentLifetime] as another option.
Having said that, I'm not fully understand the issue here including whether this
is a bug of IDL compiler or not. Should we unconditionally mark interfaces as
dependent lifetime if they are listed in [SetWrapperReferenceTo]?
On 2015/05/19 13:06:07, Yuki wrote:
> On 2015/05/19 09:28:01, haraken wrote:
> > The problem is that a DOM object that is pointed to by
[SetWrapperReferenceTo]
> > (i.e., RTCSessionDescription) needs to be a dependent object. However, it
> seems
> > that RTCSessionDescription is marked as independent. This seems to be a bug
of
> > the IDL compiler.
> >
> > ("Independent" means that there is no DOM-side reference to the wrapper
> object.
> > [SetWrapperReferenceTo B] on A means that there is a DOM-side reference from
A
> > to B. Thus B must be a dependent object.)
> >
> > bashi-san or shiino-san: Would you mind taking a look at this?
>
> I don't know well in this area, but the current implementation marks an
> interface as dependent lifetime if it's explicitly annotated with one of:
> - [DependentLifetime]
> - [ActiveDOMObject]
> - [SetWrapperReferenceFrom]
> - [SetWrapperReferenceTo]
>
> I don't yet understand what exactly [SetWrapperReference{From,To}] do, but my
qq
> is "don't we need to put [SetWrapperReferenceFrom] at
> RTCSessionDescription.idl?".
>
> Otherwise, we could use [DependentLifetime] as another option.
>
> Having said that, I'm not fully understand the issue here including whether
this
> is a bug of IDL compiler or not. Should we unconditionally mark interfaces as
> dependent lifetime if they are listed in [SetWrapperReferenceTo]?
Sounds reasonable to me. It would be better if the IDL compiler automatically
treat X as dependent lifetime if X is listed in [SetWrapperReferenceTo=X]
somewhere, but it wouldn't be mandatory.
changbin: A quick workaround would be to add [DependentLifetime] on
RTCSessionDescription. Would you try it and see how it goes?
On 2015/05/19 22:56:34, haraken wrote:
> On 2015/05/19 13:06:07, Yuki wrote:
> > On 2015/05/19 09:28:01, haraken wrote:
> > > The problem is that a DOM object that is pointed to by
> [SetWrapperReferenceTo]
> > > (i.e., RTCSessionDescription) needs to be a dependent object. However, it
> > seems
> > > that RTCSessionDescription is marked as independent. This seems to be a
bug
> of
> > > the IDL compiler.
> > >
> > > ("Independent" means that there is no DOM-side reference to the wrapper
> > object.
> > > [SetWrapperReferenceTo B] on A means that there is a DOM-side reference
from
> A
> > > to B. Thus B must be a dependent object.)
> > >
> > > bashi-san or shiino-san: Would you mind taking a look at this?
> >
> > I don't know well in this area, but the current implementation marks an
> > interface as dependent lifetime if it's explicitly annotated with one of:
> > - [DependentLifetime]
> > - [ActiveDOMObject]
> > - [SetWrapperReferenceFrom]
> > - [SetWrapperReferenceTo]
> >
> > I don't yet understand what exactly [SetWrapperReference{From,To}] do, but
my
> qq
> > is "don't we need to put [SetWrapperReferenceFrom] at
> > RTCSessionDescription.idl?".
> >
> > Otherwise, we could use [DependentLifetime] as another option.
> >
> > Having said that, I'm not fully understand the issue here including whether
> this
> > is a bug of IDL compiler or not. Should we unconditionally mark interfaces
as
> > dependent lifetime if they are listed in [SetWrapperReferenceTo]?
>
> Sounds reasonable to me. It would be better if the IDL compiler automatically
> treat X as dependent lifetime if X is listed in [SetWrapperReferenceTo=X]
> somewhere, but it wouldn't be mandatory.
>
> changbin: A quick workaround would be to add [DependentLifetime] on
> RTCSessionDescription. Would you try it and see how it goes?
haraken@ and Yuki@, thanks you very much for helping analyzing this issue:)
The fix by adding [DependentLifetime] on RTCSessionDescription will solve the
crash problem.
Thanks!
The CQ bit was checked by changbin.shao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jl@opera.com, jochen@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1010393002/#ps140001 (title: "Fix test case crash.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1010393002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=195576
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/1152173003/ by phoglund@chromium.org. The reason for reverting is: This patch breaks WebRTC browser tests (and, indeed, most WebRTC functionality, for instance apprtc.appspot.com calls).. |
