Closed
Bug 1155965
Opened 9 years ago
Closed 9 years ago
crash in mozilla::JsepSessionImpl::ValidateLocalDescription(mozilla::Sdp const&)
Categories
(Core :: WebRTC: Signaling, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
People
(Reporter: martijn.martijn, Assigned: bwc)
Details
(Keywords: crash, regression, testcase)
Crash Data
Attachments
(4 files, 2 obsolete files)
16.52 KB,
text/html
|
Details | |
39 bytes,
text/x-review-board-request
|
Details | |
39 bytes,
text/x-review-board-request
|
drno
:
review+
lmandel
:
approval-mozilla-beta+
|
Details |
39 bytes,
text/x-review-board-request
|
drno
:
review+
lmandel
:
approval-mozilla-beta+
|
Details |
This bug was filed from the Socorro interface and is report bp-fdd59d43-a292-45a8-8ef3-8efb92150418. ============================================================= 0 XUL mozilla::JsepSessionImpl::ValidateLocalDescription(mozilla::Sdp const&) media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp 1 XUL mozilla::JsepSessionImpl::SetLocalDescription(mozilla::JsepSdpType, std::string const&) media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp 2 XUL mozilla::PeerConnectionImpl::SetLocalDescription(int, char const*) media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp 3 XUL mozilla::dom::PeerConnectionImplBinding::setLocalDescription media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h 4 XUL mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp 5 @0x109ee598d 6 @0x123893c6f 7 @0x109a8b810 8 XUL EnterBaseline js/src/jit/BaselineJIT.cpp 9 XUL js::jit::EnterBaselineMethod(JSContext*, js::RunState&) js/src/jit/BaselineJIT.cpp 10 XUL js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp I get this crash after a while, with the attached testcase. You need to have the specialpowers extension installed to see the crash: http://people.mozilla.org/~mwargers/extensions/specialpowers/specialpowers.htm
Reporter | ||
Comment 1•9 years ago
|
||
Regression from bug 1091242?
Assignee | ||
Comment 2•9 years ago
|
||
It seems that somehow you're calling setLocal without calling CreateOffer...
Assignee | ||
Comment 3•9 years ago
|
||
/r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called. Pull down this commit: hg pull -r ecbbe2d52972f8d2d24db595bf27187376844516 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=357dd57c3ce2
Comment 5•9 years ago
|
||
/r/7261 - Test for bug 1155965 Pull down this commit: hg pull -r d684aa6d4fdef8f4cdbe19db61e51ddf7e9ada54 https://reviewboard-hg.mozilla.org/gecko/
Comment 6•9 years ago
|
||
I can verify that the signaling tests crash with my test without your patch but do not with your patch.
Assignee | ||
Comment 7•9 years ago
|
||
I will write a unit-test for this tomorrow.
Comment 8•9 years ago
|
||
(In reply to Byron Campen [:bwc] from comment #7) > I will write a unit-test for this tomorrow. You shouldn't need to. I wrote one already in signaling_unittests
Assignee | ||
Comment 9•9 years ago
|
||
(In reply to Eric Rescorla (:ekr) from comment #8) > (In reply to Byron Campen [:bwc] from comment #7) > > I will write a unit-test for this tomorrow. > > You shouldn't need to. I wrote one already in signaling_unittests Hmm. I'll look over there and try to determine why it did not catch this, and move it to jsep_session_unittest.
Comment 10•9 years ago
|
||
Sorry, I mean I attached one to this bug above and it validates that your patch removes the problem.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → docfaraday
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8594532 [details] MozReview Request: bz://1155965/bwc /r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called. Pull down this commit: hg pull -r 7287940001df4c57d4660b79c4c753af69672426 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 12•9 years ago
|
||
https://reviewboard.mozilla.org/r/7255/#review6051 > This violates the WebRTC specification which requires that the offer be valid for the duration of the CreateOffer callback. In this case, if you were to do > CreateOffer.then(function(r) { > SetLocal(r); > SetLocal(r); > }); > > This would fail This already won't work because we don't allow SetLocal more than once, nor have we ever allowed it. Once we have rollback support we can support this kind of thing easily. I guess what we want to do is clear it out once we finish offer/answer.
Assignee | ||
Comment 13•9 years ago
|
||
https://reviewboard.mozilla.org/r/7255/#review6055 > Sorry but this is wrong. The JSEP spec explicitly allows you to call SetLocal() repeatedly and this code would forbid that. > > The naive thing to do is probably just to leave this value in place and then rely on the other state checks to stop you from calling SetLocal() in other wrong states. I wrote and pushed this updated patch before I saw your earlier comment (failure after ValidateLocalDescription would require creating a new offer/answer, but failure earlier would not, which is screwy). I'm rebasing and moving this to the end of HandleNegotiatedSession now.
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8594532 [details] MozReview Request: bz://1155965/bwc /r/7257 - Bug 1155965: Verify that CreateOffer/Answer has been called when SetLocal is called. Pull down this commit: hg pull -r 701c80c461c010ddbdaee5f27f10f3377d99dc64 https://reviewboard-hg.mozilla.org/gecko/
Assignee | ||
Comment 15•9 years ago
|
||
https://reviewboard.mozilla.org/r/7259/#review6063 ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1526 (Diff revision 1) > + // wrt the Looks like this snuck in. ::: media/webrtc/signaling/test/signaling_unittests.cpp:3090 (Diff revision 1) > + agent(1)->SetLocal(TestObserver::OFFER, agent(0)->offer(), We should check that this fails, I think. Also, maybe this test belongs in jsep_session_unittest, but not too particular about that.
Assignee | ||
Updated•9 years ago
|
Attachment #8594532 -
Flags: review?(ekr)
Assignee | ||
Updated•9 years ago
|
Attachment #8594532 -
Flags: review?(ekr) → review?(drno)
Comment 16•9 years ago
|
||
Comment on attachment 8594532 [details] MozReview Request: bz://1155965/bwc https://reviewboard.mozilla.org/r/7255/#review6949 Ship It!
Attachment #8594532 -
Flags: review?(drno) → review+
Comment 17•9 years ago
|
||
Has anyone checked that we properly reject the same thing for setRemoteDescription()?
Comment 18•9 years ago
|
||
https://reviewboard.mozilla.org/r/7259/#review6951 ::: media/webrtc/signaling/test/signaling_unittests.cpp:3083 (Diff revision 1) > +TEST_F(SignalingAgentTest, SetLocal) { > + CreateAgent(TestStunServer::GetInstance()->addr(), > + TestStunServer::GetInstance()->port()); > + CreateAgent(TestStunServer::GetInstance()->addr(), > + TestStunServer::GetInstance()->port()); > + OfferOptions options; > + agent(0)->CreateOffer(options, OFFER_AUDIO); > + agent(1)->SetLocal(TestObserver::OFFER, agent(0)->offer(), > + true, PCImplSignalingState::SignalingStable); > +} > + Just to be safe I think we should have the same test for the answerer side.
Comment 19•9 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #17) > Has anyone checked that we properly reject the same thing for > setRemoteDescription()? Disregard. I meant the answerer side. So sLD right after sRD, without calling CreateAnswer() in between.
Assignee | ||
Comment 20•9 years ago
|
||
I can add that test.
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8594532 [details] MozReview Request: bz://1155965/bwc /r/8725 - Bug 1155965 - Part 1: Test-cases. /r/7257 - Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called. Pull down these commits: hg pull -r d4ee034d788fb0f7f95cb80f447a94e4ad7d309c https://reviewboard-hg.mozilla.org/gecko/
Attachment #8594532 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Attachment #8594532 -
Flags: review?(drno)
Assignee | ||
Comment 22•9 years ago
|
||
Part 1 is what needs review, I haven't changed part 2.
Comment 23•9 years ago
|
||
https://reviewboard.mozilla.org/r/8725/#review7389 Ship It!
Comment 24•9 years ago
|
||
Comment on attachment 8594532 [details] MozReview Request: bz://1155965/bwc https://reviewboard.mozilla.org/r/7255/#review7391 Ship It!
Attachment #8594532 -
Flags: review?(drno) → review+
Assignee | ||
Comment 25•9 years ago
|
||
Check back on try to see if intermittents clear up.
Flags: needinfo?(docfaraday)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Comment 26•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e82bae379ca6 https://hg.mozilla.org/integration/mozilla-inbound/rev/b57fef90c5cc
Keywords: checkin-needed
Comment 27•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e82bae379ca6 https://hg.mozilla.org/mozilla-central/rev/b57fef90c5cc
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment 28•9 years ago
|
||
Attachment #8594536 -
Attachment is obsolete: true
Comment 29•9 years ago
|
||
Assignee | ||
Comment 30•9 years ago
|
||
Attachment #8594532 -
Attachment is obsolete: true
Attachment #8620093 -
Flags: review+
Attachment #8620094 -
Flags: review+
Assignee | ||
Comment 31•9 years ago
|
||
Assignee | ||
Comment 32•9 years ago
|
||
Comment 33•9 years ago
|
||
Byron - does this need uplift to 40, or should it be WONTFIX?
Flags: needinfo?(docfaraday)
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8620093 [details] MozReview Request: Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called. I guess this has high annoyance potential, so an uplift would make sense. Approval Request Comment [Feature/regressing bug #]: sdparta [User impact if declined]: Null pointer crash can be caused by content at will, so there is a high annoyance potential here. [Describe test coverage new/current, TreeHerder]: A unit-test was added (see part 1). [Risks and why]: Extremely low, we just added some checking. [String/UUID change made/needed]: None.
Flags: needinfo?(docfaraday)
Attachment #8620093 -
Flags: approval-mozilla-beta?
Comment 35•9 years ago
|
||
Comment on attachment 8620093 [details] MozReview Request: Bug 1155965 - Part 2: Verify that CreateOffer/Answer has been called when SetLocal is called. bwc tells me that although this crash does not currently have a high incidence rate there is a real potential for abuse. Given that and that the patch is pretty safe, we'll take this in beta3. Beta+
Attachment #8620093 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Updated•9 years ago
|
Attachment #8620094 -
Flags: approval-mozilla-beta+
Comment 36•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/fd3b313c083b https://hg.mozilla.org/releases/mozilla-beta/rev/1332e4b5d0e1
Flags: in-testsuite+
Reporter | ||
Comment 37•9 years ago
|
||
I'm still crashing with the testcase attached to this bug, I filed bug 1182098 for it.
You need to log in
before you can comment on or make changes to this bug.
Description
•