Add memory of last SDP offer/answer created [chromium/src : master]
Harald Alvestrand (Gerrit)
Quick change. There's still a problem with checking for unmodified SDP before checking for state, which results in the wrong error in one case.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/w3c/web-platform-tests/pull/10075.
If this CL lands and Travis CI upstream is green, we will auto-merge the PR.
Note: Please check the Travis CI status (at the bottom of the PR) before landing this CL and only land this CL if the status is green. Otherwise a human needs to step in and resolve it manually. (This may be automated in the future, see https://crbug.com/711447)
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/web_platform_tests.md#Automatic-export-process
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Note generated SDP in both codepaths": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Harald Alvestrand removed a vote from this change.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Added state checking before SDP checking": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Harald Alvestrand uploaded patch set #4 to this change.
Add memory of last SDP offer/answer created
This adds internal slots "LastOffer" and "LastAnswer", and uses
those to check that SetLocalDescription uses an unchanged SDP offer/answer.
Bug: chromium:823036
Change-Id: I0c978d5ff3e63b0afab3ec02334c57a5aaa94cdd
---
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-answer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-pranswer-expected.txt
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestImpl.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.cpp
10 files changed, 77 insertions(+), 18 deletions(-)
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10075
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Added checks to callback-style setLocal": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Harald Alvestrand uploaded patch set #7 to this change.
Add memory of last SDP offer/answer created
This adds internal slots "LastOffer" and "LastAnswer", and uses
those to check that SetLocalDescription uses an unchanged SDP offer/answer.
Because modification of SDP is suspected to be used in a number of places,
this change does not reject the modified SDP, but merely counts the usage.
Bug: chromium:823036
Change-Id: I0c978d5ff3e63b0afab3ec02334c57a5aaa94cdd
---
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-answer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-pranswer-expected.txt
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestImpl.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.cpp
M third_party/WebKit/public/platform/web_feature.mojom
M tools/metrics/histograms/enums.xml
12 files changed, 105 insertions(+), 23 deletions(-)
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Switched from failure to counter for SDP mods": https://github.com/w3c/web-platform-tests/pull/10075
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Harald Alvestrand would like Kentaro Hara to review this change.
Add memory of last SDP offer/answer created
This adds internal slots "LastOffer" and "LastAnswer", and uses
those to check that SetLocalDescription uses an unchanged SDP offer/answer.
Because modification of SDP is suspected to be used in a number of places,
this change does not reject the modified SDP, but merely counts the usage.
Bug: chromium:823036
Change-Id: I0c978d5ff3e63b0afab3ec02334c57a5aaa94cdd
---
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-answer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-pranswer-expected.txt
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestImpl.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.cpp
M third_party/WebKit/public/platform/web_feature.mojom
M tools/metrics/histograms/enums.xml
12 files changed, 105 insertions(+), 23 deletions(-)
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Adding haraken for histograms
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Rebase": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Harald Alvestrand uploaded patch set #10 to this change.
Add memory of last SDP offer/answer created
This adds internal slots "LastOffer" and "LastAnswer", and uses
those to check that SetLocalDescription uses an unchanged SDP offer/answer.
Because modification of SDP is suspected to be used in a number of places,
this change only rejects SDP with modified fingerprints (which would fail
anyway), but merely counts the usage for other modifications.
Bug: chromium:823036
Change-Id: I0c978d5ff3e63b0afab3ec02334c57a5aaa94cdd
---
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-answer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-pranswer-expected.txt
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestImpl.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.cpp
M third_party/WebKit/public/platform/web_feature.mojom
M tools/metrics/histograms/enums.xml
12 files changed, 128 insertions(+), 23 deletions(-)
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Edit commit message": https://github.com/w3c/web-platform-tests/pull/10075
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Adding Tommi for content/test/data/media
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Fix a test that checked message text": https://github.com/w3c/web-platform-tests/pull/10075
Tommi (Gerrit)
lgtm (more of a rubberstamp).
Do we have a sense for if this will make a big splash with web apps or minimal/no impact?
Patch set 11:Code-Review +1
Harald Alvestrand (Gerrit)
This version of the CL (where we only count SDP modifications) will have minimal impact. The things that will fail (fingerprint modification on DTLS connections) already failed, because this error was caught at the WebRTC layer.
(The main practical reason for changing this was to get the fingerprints out of the error messages generated by WPT - those fingerprints caused the tests to be listed as "not conforming to expectations" on every run, which harmed the ability to catch other errors.)
When we tighten the check to be according to spec, with no modifications allowed, this will have a big impact, I think; the counter installed here will be able to tell us just how big.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Don't check SDP changes when no SDP generated": https://github.com/w3c/web-platform-tests/pull/10075
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Changed handling for non-DTLS offer/answers": https://github.com/w3c/web-platform-tests/pull/10075
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Error code modified again": https://github.com/w3c/web-platform-tests/pull/10075
Harald Alvestrand (Gerrit)
Now it seems to be ready for general review.
Kentaro Hara (Gerrit)
LGTM
Patch set 14:Code-Review +1
Henrik Boström (Gerrit)
1 comment:
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Patch Set #14, Line 793:
// TODO(https://crbug.com/823036): Return failure for all modification.Shouldn't this be implemented in third_party/webrtc? We don't want to multiple layers to give their own assessments of the SDP and hope they agree with each other. Unless there is good reason for putting this logic here.
Having multiple implementations of SDP validation is a problem. This can also be a problem since it occurs on different threads, sometimes the states are not up-to-date since operations have been queued on the thread task queues. SLD/SRD are asynchronous so we can't rely on any state that can be modified by the result of SLD operation (e.g. signalingState) without being ensured that any pending operation has completed.
These problems might only cause edge-case problems, and if the SDP is incorrectly forwarded to the lower layer the SDP could be rejected by the lower layer later anyway and fail. But that's the thing, if we have to handle all errors by the lower layer anyway, then the lower layer should surface the error, and adding logic here is both redundant and error-prone.
- Is this supposed to be the right approach going forward or a quick fix to cover the majority of cases and get use counters in?
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
1 comment:
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Patch Set #14, Line 819:
ScriptPromise RTCPeerConnection::setLocalDescription(If you call SLD twice in a row, both will test against "last_offer_" being the default value, because by the time the second SLD is called we have not yet surfaced the result of the first SLD from the webrtc signaling thread:
setLocalDescription("first offer");
setLocalDescription("second offer");However, on the webrtc signaling thread, by the time the second SLD occurs, the "last_offer_" will be "first offer", not the default.
The layers are out-of-sync.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
1 comment:
Patch Set #14, Line 819:
ScriptPromise RTCPeerConnection::setLocalDescription(
If you call SLD twice in a row, both will test against "last_offer_" being the default value, becaus […]
You might blame the user for calling two async operations in a row without await/then but in any case the resulting implementation will be observably different from the spec assuming "in parallel" is not racing with other "in parallel" operations. If this is undefined spec behavior then perhaps it does not matter, but because our implementation is well-defined, if it truly does not make a difference I want to hear an argument for that and include "SLD twice in a row" test coverage.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Comments on comments, no code changes.
2 comments:
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Patch Set #14, Line 793:
// TODO(https://crbug.com/823036): Return failure for all modification.Shouldn't this be implemented in third_party/webrtc? We don't want to multiple layers to give their […]
Well, it isn't done now in Webrtc..... and perhaps they don't want to. There are reasons to do SDP munging, and the C++ API may be used by other clients too.
the opposite argument is that the particular restrictions in the W3C spec should be enforced as close to the API as possible. given the complexity of the SDP handling in the lower layers, enforcing the simple things where things are simple seems like a good approach.
I would not argue for parsing the SDP in the upper layers (the hackaround I'm doing here is because fingerprint modification is easy to detect and never a valid thing to do, so it will catch some erroneous usages of the API).
Checking the fingerprint is a temporary hack that will make tests pass and lets me get the counters in.
Patch Set #14, Line 819:
ScriptPromise RTCPeerConnection::setLocalDescription(
You might blame the user for calling two async operations in a row without await/then but in any cas […]
There's actually a test in the WPT that calls SLD twice in a row. It's legal, and it works. (The test fails because it looks for the resulting SDP in a place where we don't expose it yet, but that's another bug to fix).
CreateOffer() => first
CreateOffer() => <unresolved>
SetLocalDescription(first)
CO resolves => second
SLD resolves => checks against second???that's a good question.
Setting [[LastOffer]] is in the "fire a task" part of CreateOffer, but the way SetLocalDescription is written, there is first a setting that might fail or succeed, and then one fires a task that tries to figure out what happened - which will then be queued after the result of the second CreateOffer, even if the failure was relative to the result of the first CreateOffer.This is a spec bug, I think.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Adding to a refreshed memory:
CreateOffer and SetLocalDescription are enqueued on the connection's operations queue.
So we're guaranteed that one is completed before the next one is started.
But it means that doing the checking in the initial synchronous part is not obviously the Right Thing; if we have
CreateOffer() => old offer
CreateOffer() => unresolved
SetLocalDescription(old offer)
=> new offer
then setLocalDescription(old offer) will go on the operations queue, and it should only be done after the second CreateOffer() completes - which is not what this CL will achieve.
Oh well. Back to the drawing board.
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Error code modified again": https://github.com/w3c/web-platform-tests/pull/10075
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
LGTM % comments. Please note there are problems with this design where different layers disagree on the state of the last offer, but considering they are only apparent in edge case problems (racy API usages) I think this is fine for now.
CC deadbeef FYI, see comment.
Patch set 15:Code-Review +1
4 comments:
Patch Set #14, Line 793:
// TODO(https://crbug.com/823036): Return failure for all modification.
Well, it isn't done now in Webrtc..... and perhaps they don't want to. […]
Acknowledge.
Bigger picture: The "webrtc-pc in blink / JSEP in webrtc" split sounds appealing to me, but do note that this is not how our codebase looks today, and doing some parts of the operation in one place on one thread and other parts of the operation in another place on a different thread is error-prone and has caused bugs in the past and this. If we want to push for this logic split going forward we should coordinate with deadbeef@ et. al. so everyone is on the same page. CC deadbeef.
This CL: This generally works, but not in edge cases, see below.
Patch Set #14, Line 819:
ScriptPromise RTCPeerConnection::setLocalDescription(
There's actually a test in the WPT that calls SLD twice in a row. It's legal, and it works. […]
Spec bug or not, I think it's an implementation bug to compare the "last offer" to one value when checking whether or not to perform the operation, and then when we actually perform the operation the "last offer" is something different.
let offer1 = await pc.createOffer();
pc.createOffer().then(offer2 => { ... });
pc.setLocalDescription(offer1);Because of the Chrome's threading model, this will...
MAIN THREAD
Task execution cycle #1:
1. Create the first offer and await it, last_offer_ = offer1;
Task execution cycle #2:
2. Queue a task to preform createOffer() in signaling thread.
3. setLocalDescription() in blink
a. Test argument against last_offer_.
"Last offer" is ALWAYS offer1 on this thread.
b. Queue a task to preform setLocalDescription() in signaling thread
(which will occur when "last offer" is offer2, see below).
Task execution cycle #3 (assuming signaling thread is done):
4. last_offer_ = offer2, resolve createOffer() promise.
5. Resolve SLD promise.SIGNALING THREAD
1. Create offer1, queue a task to resolve the promise on main thread (the one we await).
2. Create offer2, queue a task to resolve the promise on main thread.
3. setLocalDescription() in webrtc
a. "Last offer" is ALWAYS offer2 on this thread.
b. Queue a task to resolve the promise on the main thread."Last offer" is not the same for blink queuing the operation and webrtc performing the operation whenever you do not await a promise. That is, even if the spec is not racy we are comparing apples and oranges.
---
There is no way to check this correctly without either waiting for all pending operations to finish or perform checkSdpForStateErrors() on the signaling thread.
If we want to do this in Chrome and not third_party/webrtc we would have to increase the complexity of the content layer (put this in rtc_peer_connection_handler.cc - which goes against "removing content layer") or give blink access to the webrtc signaling thread directly, in either case this involves refactoring.
I think doing this correctly is not worth the work effort at the moment, but we need to think about stuff like this in our design. And you should probably put a couple of TODOs or file a bug about it.
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Patch Set #15, Line 455:
return false;This will pass if the old_sdp has a fingerprint but new_sdp doesn't. This should only return false if old_fingerprint_pos is also == kNotFound.
Patch Set #15, Line 467:
new_fingerprint_pos) == kNotFound;nit: Prefer to get the new_fingerprint_end and do new_sdp.Substring() too and and compare fingerprints by equality.
Not sure we care but otherwise it would return false if the fingerprint string is sneaked in somewhere not as a fingerprint (e.g. a track ID = fingerprint).
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Henrik Boström (Gerrit)
Discussion to be had: Should third_party/webrtc be everyting webrtc-pc + JSEP or just JSEP, with webrtc-pc stuff migrated into blink?
Harald Alvestrand (Gerrit)
Thanks! Now submitting.
3 comments:
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Spec bug or not, I think it's an implementation bug to compare the "last offer" to one value when ch […]
I don't think this is right, because I haven't found a memory of "last offer" that is remembered on the signalling thread - the local_description that is stored in the webrtc layer is the local description set on the *previous* offer/answer cycle (CurrentLocalDescription in spec parlance), so SetLocalDescription() will use the SDP that is passed in its argument, not what's remembered.
But it is racy, and the Right Solution is to implement the operations queue (at some layer).
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:
Patch Set #15, Line 455:
return false;
This will pass if the old_sdp has a fingerprint but new_sdp doesn't. […]
That was my original design, but it turns out that SDP mangling is used by entities using SDES keys - they strip out the a=fingerprint and replace it with a=crypto (if they don't construct the SDP from whole cloth - I also had to allow the case where createOffer was not called at all).
Patch Set #15, Line 467:
old_fingerprint_end - old_fingerprint_pos) !=nit: Prefer to get the new_fingerprint_end and do new_sdp. […]
Good point. Will fix.
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.
Harald Alvestrand (Gerrit)
Patch set 16:Commit-Queue +2
Commit Bot (Gerrit)
CQ is trying the patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Stricter fingerprint check" https://chromium-review.googlesource.com/c/966441/16
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/966441/16
Bot data: {"action": "start", "triggered_at": "2018-03-22T16:42:57.0Z", "cq_cfg_revision": "5b6c43e4d6b0297aa92e118e785d640c42297271", "revision": "d7120326f7502cf655bba87c6dcc077b59683e1b"}
Blink WPT Bot (Gerrit)
Successfully updated WPT GitHub pull request with new revision "Stricter fingerprint check": https://github.com/w3c/web-platform-tests/pull/10075
Commit Bot (Gerrit)
Commit Bot merged this change.
Add memory of last SDP offer/answer created
This adds internal slots "LastOffer" and "LastAnswer", and uses
those to check that SetLocalDescription uses an unchanged SDP offer/answer.
Because modification of SDP is suspected to be used in a number of places,
this change only rejects SDP with modified fingerprints (which would fail
anyway), but merely counts the usage for other modifications.
Bug: chromium:823036
Change-Id: I0c978d5ff3e63b0afab3ec02334c57a5aaa94cdd
Reviewed-on: https://chromium-review.googlesource.com/966441
Commit-Queue: Harald Alvestrand <h...@chromium.org>
Reviewed-by: Henrik Boström <hb...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Reviewed-by: Tommi <to...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545175}
---
M content/test/data/media/peerconnection-call.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-answer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer-expected.txt
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-offer.html
M third_party/WebKit/LayoutTests/external/wpt/webrtc/RTCPeerConnection-setLocalDescription-pranswer-expected.txt
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescription.h
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestImpl.cpp
M third_party/WebKit/Source/modules/peerconnection/RTCSessionDescriptionRequestPromiseImpl.cpp
M third_party/WebKit/public/platform/web_feature.mojom
M tools/metrics/histograms/enums.xml
13 files changed, 141 insertions(+), 27 deletions(-)
Blink WPT Bot (Gerrit)
The WPT PR for this CL has been merged upstream! https://github.com/w3c/web-platform-tests/pull/10075
Henrik Boström (Gerrit)
1 comment:
I don't think this is right, because I haven't found a memory of "last offer" that is remembered on […]
Per-spec it might be racy, per-implementation it might not be racy. Most async operations are implemented synchronously on the signaling thread and are executed in the order posted. setLocalDescription is this way. getStats is an exception. Maybe createOffer is too due to certificate generation? Not sure.
Whether or not there is a "last offer" variable on the signaling thread, you are evaluating whether or not to perform an operation based on the state of a "last offer" that will not be the last offer when the operation is executed. This is like deciding whether or not to start your car now based on if it was green 10 seconds ago (or am I missing something?).
To view, visit change 966441. To unsubscribe, or for help writing mail filters, visit settings.