827843 - check PeerConnection.js to make sure functions act correctly after close()

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

We need to check PeerConnection.js to make sure functions act correctly after close().  I noticed some new functions like localDescription just call this._pc.*.  The QueueOrRun code checks _closed.

Also for any functions not using QueueOrRun, check that there aren't any ordering issues (i.e. call foo() which queues a change, than call bar() that doesn't queue, which looks at data modified by foo (which hasn't run yet).

localDescription might fall into this bucket.  Also, this should be caught by mochitests; if you find a bug like this, try writing a mochitest for it.

Blocking because we need to verify this before release.

Comment 1

[Adam Roach [:abr]]

Attachment: Generate exceptions when certain operations occur after close

This patch needs to be applied on top of the one for Bug 825570.

Note that the calls to get localDescription and get remoteDescription are defined (by spec) to be sync, so we cannot queue them. The milieu here is that applications aren't expected to try to access this attribute until they get a success callback from the async call to getLocalDescription or setRemoteDescription.

The choice of throwing exceptions here rather than silently returning is based on (what I perceive to be) the emerging consensus within the WebRTC working group that programming errors generate exceptions, and access of these methods in the closed state is a programming error.

This patch passes the media mochi tests locally. I plan to push to try off the alder branch as soon as the try tree re-opens.

Comment 2

[Adam Roach [:abr]]

Attachment: Generate exceptions when certain operations occur after close, v2

Some minor clean-up from original patch

Comment 3

[Adam Roach [:abr]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=de14476854c2

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 702929 [details] [diff] [review]
Generate exceptions when certain operations occur after close, v2

Review of attachment 702929 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +399,5 @@
>  
>    setLocalDescription: function(desc, onSuccess, onError) {
> +    // TODO -- if we have two setLocalDescriptions in the
> +    // queue,this code overwrites the callbacks for the first
> +    // one with the callbacks for the second one.

Please link to a bug#, and file if not already in bugzilla

@@ +429,5 @@
>  
>    setRemoteDescription: function(desc, onSuccess, onError) {
> +    // TODO -- if we have two setRemoteDescriptions in the
> +    // queue, this code overwrites the callbacks for the first
> +    // one with the callbacks for the second one.

ditto (same bug I'd assume)

@@ +505,5 @@
>  
>    get localStreams() {
> +    if (this._closed) {
> +      throw new Error("Peer Connection is closed");
> +    }

perhaps use checkClosed(); and function checkClosed() { if (this._closed) { throw new Error("Peer Connection is closed"); } }

Minimizes boilerplate and provides a hook for any other such tests and modifying it, avoids duplicate string proliferation.  This may not be the "correct" style, however - please consult a DOM person on that.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 702929 [details] [diff] [review]
Generate exceptions when certain operations occur after close, v2

   test_peerConnection_basicVideo.html \
+  test_peerConnection_basicVideo.html \
You're adding test_peerConnection_basicVideo.html to the list although it is
there already+

We really need to figure out the localization story here.
But not in this bug.
Please discuss with Pike.

Comment 6

[Adam Roach [:abr]]

(In reply to Olli Pettay [:smaug] from comment #5)

> We really need to figure out the localization story here.
> But not in this bug.
> Please discuss with Pike.

After eliminating extensions, tests, examples, and devtools, I find at least 384 instances of "throw new Error(string)" in the tree: http://pastebin.mozilla.org/2066312 -- and that's leaving out the six specializations of Error as well as the form 'throw "Message"'.

I combed through these strings and determined, at least on a first pass, that none are localized, and all are in English. Changing this situation would be a first-class project, probably requiring a dedicated team.

Venturing into personal opinion -- and recognizing that my fluency in English does instill in me certain cultural biases on the topic -- I think that such an effort would have a very low return on investment. Exception messages are intended for application developers, not end users; and both javascript and html already have English-language biases built in.

Do note that I'm sensitive to localization issues in general. For example, in the Thunderbird add-on that I maintain, I have an in-extension facility for easy submission of new translations and corrections to existing translations, which is one step further than I've seen anyone else take. I just don't think exception messages are aimed at an audience that requires localization.

If you want to point me towards Pike (e.g., using an IRC name or email address), I'm happy to engage in this conversation with him; although I suspect we need a wider audience than just him and me, as this issue is pervasive.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Pike is Pike on IRC, [e-mail]

Comment 8

[Adam Roach [:abr]]

Attachment: Check for closed state on relevant operations

Comment 9

[Adam Roach [:abr]]

Comment on attachment 703507 [details] [diff] [review]
Check for closed state on relevant operations

Carrying forward r+ from jesup and smaug

Comment 10

[Adam Roach [:abr]]

Olli: is the _checkClosed() method (see attached patch) kosher for our overall JavaScript style? Randell wanted me to double-check with someone from DOM.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Yeah, should be ok. DOM (excluding b2g stuff) doesn't actually have that much stuff implemented in JS, 
so the style might not be that consistent.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33b1caee42d2

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fb9e771edb

https://tbpl.mozilla.org/php/getParsedLog.php?id=19071045&tree=Mozilla-Inbound

954 INFO TEST-START | /tests/dom/media/tests/mochitest/test_peerConnection_bug827843.html
955 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_peerConnection_bug827843.html | Unexpected error callback with ReferenceError: mozRTCPeerConnection is not defined
956 INFO TEST-END | /tests/dom/media/tests/mochitest/test_peerConnection_bug827843.html | finished in 1125ms

Comment 14

[Jason Smith [:jsmith]]

Comment on attachment 703507 [details] [diff] [review]
Check for closed state on relevant operations

Review of attachment 703507 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/tests/mochitest/test_peerConnection_bug827843.html
@@ +106,5 @@
> +          SimpleTest.finish();
> +        });
> +      }, unexpectedCallbackAndFinish);
> +    }, unexpectedCallbackAndFinish);
> +  });

If the test is desktop supported only, add a second parameter to runTest that is true. That will prevent the Android failure we are seeing.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a545cc984003
bustage fix, reland rs=me

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a545cc984003

Comment 17

[Jason Smith [:jsmith]]

Checked in mochitest = automatic verification.