Bug 1091898 - restore exception-handling around legacy callbacks. r=mt
authorJan-Ivar Bruaroey <jib@mozilla.com>
Mon, 08 Dec 2014 09:37:14 -0600
changeset 219126 3e280c309d0d9e829940b47c1ba19b30060631f7
parent 219125 34fd03c467e8df1841238f11bfd59ff51c94b7ad
child 219127 51ee431901353b9ec8edabc2458521b164663acb
push id27956
push userkwierso@gmail.com
push dateFri, 12 Dec 2014 00:47:19 +0000
treeherdermozilla-central@32a2c5bd2f68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmt
bugs1091898
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1091898 - restore exception-handling around legacy callbacks. r=mt
dom/media/PeerConnection.js
dom/media/tests/mochitest/test_peerConnection_close.html
--- a/dom/media/PeerConnection.js
+++ b/dom/media/PeerConnection.js
@@ -494,23 +494,23 @@ RTCPeerConnection.prototype = {
     // PC can close while events are firing if there is an async dispatch
     // in c++ land
     if (!this._closed) {
       this.__DOM_IMPL__.dispatchEvent(event);
     }
   },
 
   // Log error message to web console and window.onerror, if present.
-  logErrorAndCallOnError: function(msg, file, line) {
-    this.logMsg(msg, file, line, Ci.nsIScriptError.exceptionFlag);
+  logErrorAndCallOnError: function(e) {
+    this.logMsg(e.message, e.fileName, e.lineNumber, Ci.nsIScriptError.exceptionFlag);
 
     // Safely call onerror directly if present (necessary for testing)
     try {
       if (typeof this._win.onerror === "function") {
-        this._win.onerror(msg, file, line);
+        this._win.onerror(e.message, e.fileName, e.lineNumber);
       }
     } catch(e) {
       // If onerror itself throws, service it.
       try {
         this.logError(e.message, e.fileName, e.lineNumber);
       } catch(e) {}
     }
   },
@@ -544,16 +544,31 @@ RTCPeerConnection.prototype = {
   makeGetterSetterEH: function(name) {
     Object.defineProperty(this, name,
                           {
                             get:function()  { return this.getEH(name); },
                             set:function(h) { return this.setEH(name, h); }
                           });
   },
 
+  // Helper for legacy callbacks
+  thenCB: function(p, onSuccess, onError) {
+    var errorFunc = this.logErrorAndCallOnError.bind(this);
+
+    function callCB(func, arg) {
+      try {
+        func(arg);
+      } catch (e) {
+        errorFunc(e);
+      }
+    }
+    return onSuccess? p.then(result => callCB(onSuccess, result),
+                             reason => (onError? callCB(onError, reason) : null)) : p;
+  },
+
   createOffer: function(optionsOrOnSuccess, onError, options) {
 
     // TODO: Remove old constraint-like RTCOptions support soon (Bug 1064223).
     // Note that webidl bindings make o.mandatory implicit but not o.optional.
     function convertLegacyOptions(o) {
       // Detect (mandatory OR optional) AND no other top-level members.
       let lcy = ((o.mandatory && Object.keys(o.mandatory).length) || o.optional) &&
                 Object.keys(o).length == (o.mandatory? 1 : 0) + (o.optional? 1 : 0);
@@ -595,25 +610,22 @@ RTCPeerConnection.prototype = {
       onError = undefined;
     }
     if (options && convertLegacyOptions(options)) {
       this.logWarning(
           "Mandatory/optional in createOffer options is deprecated! Use " +
           JSON.stringify(options) + " instead (note the case difference)!",
           null, 0);
     }
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._createOffer,
-        args: [resolve, reject, options],
-        wait: true
-      });
-    });
-    return (onSuccess || onError)?
-      p.then(offer => { onSuccess(offer); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._createOffer,
+      args: [resolve, reject, options],
+      wait: true
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   _createOffer: function(onSuccess, onError, options) {
     this._onCreateOfferSuccess = onSuccess;
     this._onCreateOfferFailure = onError;
     this._impl.createOffer(options);
   },
 
@@ -633,25 +645,22 @@ RTCPeerConnection.prototype = {
       this._observer.onCreateAnswerError(Ci.IPeerConnection.kInvalidState,
                                          "No outstanding offer");
       return;
     }
     this._impl.createAnswer();
   },
 
   createAnswer: function(onSuccess, onError) {
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._createAnswer,
-        args: [resolve, reject],
-        wait: true
-      });
-    });
-    return (onSuccess || onError)?
-      p.then(answer => { onSuccess(answer); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._createAnswer,
+      args: [resolve, reject],
+      wait: true
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   setLocalDescription: function(desc, onSuccess, onError) {
     this._localType = desc.type;
 
     let type;
     switch (desc.type) {
       case "offer":
@@ -662,24 +671,22 @@ RTCPeerConnection.prototype = {
         break;
       case "pranswer":
         throw new this._win.DOMError("NotSupportedError", "pranswer not yet implemented");
       default:
         throw new this._win.DOMError("InvalidParameterError",
             "Invalid type " + desc.type + " provided to setLocalDescription");
     }
 
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._setLocalDescription,
-        args: [type, desc.sdp, resolve, reject],
-        wait: true
-      });
-    });
-    return (onSuccess || onError)? p.then(() => { onSuccess(); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._setLocalDescription,
+      args: [type, desc.sdp, resolve, reject],
+      wait: true
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   _setLocalDescription: function(type, sdp, onSuccess, onError) {
     this._onSetLocalDescriptionSuccess = onSuccess;
     this._onSetLocalDescriptionFailure = onError;
     this._impl.setLocalDescription(type, sdp);
   },
 
@@ -699,24 +706,22 @@ RTCPeerConnection.prototype = {
       default:
         throw new this._win.DOMError("InvalidParameterError",
             "Invalid type " + desc.type + " provided to setRemoteDescription");
     }
 
     // Have to get caller's origin outside of Promise constructor and pass it in
     let origin = Cu.getWebIDLCallerPrincipal().origin;
 
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._setRemoteDescription,
-        args: [type, desc.sdp, origin, resolve, reject],
-        wait: true
-      });
-    });
-    return (onSuccess || onError)? p.then(() => { onSuccess(); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._setRemoteDescription,
+      args: [type, desc.sdp, origin, resolve, reject],
+      wait: true
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   /**
    * Takes a result from the IdP and checks it against expectations.
    * If OK, generates events.
    * Returns true if it is either present and valid, or if there is no
    * need for identity.
    */
@@ -826,24 +831,22 @@ RTCPeerConnection.prototype = {
   },
 
   addIceCandidate: function(cand, onSuccess, onError) {
     if (!cand.candidate && !cand.sdpMLineIndex) {
       throw new this._win.DOMError("InvalidParameterError",
           "Invalid candidate passed to addIceCandidate!");
     }
 
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._addIceCandidate,
-        args: [cand, resolve, reject],
-        wait: false
-      });
-    });
-    return (onSuccess || onError)? p.then(() => { onSuccess(); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._addIceCandidate,
+      args: [cand, resolve, reject],
+      wait: false
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   _addIceCandidate: function(cand, onSuccess, onError) {
     this._onAddIceCandidateSuccess = onSuccess || null;
     this._onAddIceCandidateError = onError || null;
 
     this._impl.addIceCandidate(cand.candidate, cand.sdpMid || "",
                                (cand.sdpMLineIndex === null) ? 0 :
@@ -1010,25 +1013,22 @@ RTCPeerConnection.prototype = {
 
   changeIceConnectionState: function(state) {
     this._iceConnectionState = state;
     _globalPCList.notifyLifecycleObservers(this, "iceconnectionstatechange");
     this.dispatchEvent(new this._win.Event("iceconnectionstatechange"));
   },
 
   getStats: function(selector, onSuccess, onError) {
-    let p = new this._win.Promise((resolve, reject) => {
-      this._queueOrRun({
-        func: this._getStats,
-        args: [selector, resolve, reject],
-        wait: false
-      });
-    });
-    return (onSuccess || onError)?
-      p.then(stats => { onSuccess(stats); }, onError) : p;
+    let p = new this._win.Promise((resolve, reject) => this._queueOrRun({
+      func: this._getStats,
+      args: [selector, resolve, reject],
+      wait: false
+    }));
+    return this.thenCB(p, onSuccess, onError);
   },
 
   _getStats: function(selector, onSuccess, onError) {
     this._onGetStatsSuccess = onSuccess;
     this._onGetStatsFailure = onError;
 
     this._impl.getStats(selector);
   },
@@ -1418,24 +1418,21 @@ RTCRtpSender.prototype = {
   classDescription: "RTCRtpSender",
   classID: PC_SENDER_CID,
   contractID: PC_SENDER_CONTRACT,
   QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports]),
 
   replaceTrack: function(withTrack) {
     this._pc._checkClosed();
 
-    let p = new this._pc._win.Promise((resolve, reject) => {
-      this._pc._queueOrRun({
-        func: this._pc._replaceTrack,
-        args: [this, withTrack, resolve, reject],
-        wait: false
-      });
-    });
-    return p;
+    return new this._pc._win.Promise((resolve, reject) => this._pc._queueOrRun({
+      func: this._pc._replaceTrack,
+      args: [this, withTrack, resolve, reject],
+      wait: false
+    }));
   }
 };
 
 function RTCRtpReceiver(pc, track) {
   this.pc = pc;
   this.track = track;
 }
 RTCRtpReceiver.prototype = {
--- a/dom/media/tests/mochitest/test_peerConnection_close.html
+++ b/dom/media/tests/mochitest/test_peerConnection_close.html
@@ -20,19 +20,17 @@
     var eTimeout = null;
 
     // everything should be in initial state
     is(pc.signalingState, "stable", "Initial signalingState is 'stable'");
     is(pc.iceConnectionState, "new", "Initial iceConnectionState is 'new'");
     is(pc.iceGatheringState, "new", "Initial iceGatheringState is 'new'");
 
     var finish;
-    var finished = new Promise(function(resolve) {
-      finish = resolve;
-    });
+    var finished = new Promise(resolve => finish = resolve);
 
     pc.onsignalingstatechange = function(e) {
       clearTimeout(eTimeout);
       is(pc.signalingState, "closed", "signalingState is 'closed'");
       is(pc.iceConnectionState, "closed", "iceConnectionState is 'closed'");
 
       try {
         pc.close();
@@ -54,16 +52,49 @@
       // "invalid-on-close" are:
       // - createOffer
       // - createAnswer
       // - setLocalDescription
       // - setRemoteDescription
       // - addIceCandidate
       // - getStats
       //
+      // These legacy methods fire the error callback instead. This is not
+      // entirely to spec but is better than ignoring programming errors.
+
+      var offer = new mozRTCSessionDescription({ sdp: "sdp", type: "offer" });
+      var answer = new mozRTCSessionDescription({ sdp: "sdp", type: "answer" });
+      var candidate = new mozRTCIceCandidate({ candidate: "dummy",
+                                               sdpMid: "test",
+                                               sdpMLineIndex: 3 });
+
+      var doesFail = (p, msg) => p.then(generateErrorCallback(),
+                                        r => is(r.name, "InvalidStateError", msg));
+
+      doesFail(pc.createOffer(), "createOffer fails on close")
+      .then(() => doesFail(pc.createAnswer(), "createAnswer fails on close"))
+      .then(() => doesFail(pc.setLocalDescription(offer),
+                           "setLocalDescription fails on close"))
+      .then(() => doesFail(pc.setRemoteDescription(answer),
+                           "setRemoteDescription fails on close"))
+      .then(() => doesFail(pc.addIceCandidate(candidate),
+                           "addIceCandidate fails on close"))
+      .then(() => doesFail(new Promise((y, n) => pc.createOffer(y, n)),
+                           "Legacy createOffer fails on close"))
+      .then(() => doesFail(new Promise((y, n) => pc.createAnswer(y, n)),
+                           "Legacy createAnswer fails on close"))
+      .then(() => doesFail(new Promise((y, n) => pc.setLocalDescription(offer, y, n)),
+                           "Legacy setLocalDescription fails on close"))
+      .then(() => doesFail(new Promise((y, n) => pc.setRemoteDescription(answer, y, n)),
+                           "Legacy setRemoteDescription fails on close"))
+      .then(() => doesFail(new Promise((y, n) => pc.addIceCandidate(candidate, y, n)),
+                           "Legacy addIceCandidate fails on close"))
+      .catch(reason => ok(false, "unexpected failure: " + reason))
+      .then(finish);
+
       // Other methods are unaffected.
 
       SimpleTest.doesThrow(function() {
         pc.updateIce("Invalid RTC Configuration")},
         "updateIce() on closed PC raised expected exception");
 
       SimpleTest.doesThrow(function() {
         pc.addStream("Invalid Media Stream")},
@@ -75,19 +106,17 @@
 
       SimpleTest.doesThrow(function() {
         pc.createDataChannel({})},
         "createDataChannel() on closed PC raised expected exception");
 
       SimpleTest.doesThrow(function() {
         pc.setIdentityProvider("Invalid Provider")},
         "setIdentityProvider() on closed PC raised expected exception");
-
-      finish();
-    }
+    };
 
     // This prevents a mochitest timeout in case the event does not fire
     eTimeout = setTimeout(function() {
       ok(false, "Failed to receive expected onsignalingstatechange event in 60s");
       finish();
     }, 60000);
 
     try {