521258 - Catch exceptions in template ChannelListener class to avoid hangs.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jason Duell]

Attachment: Catch exceptions in template ChannelListener class to avoid hangs.

If onStartRequest, onDataAvailable, or onStopRequest throw an exception from
javascript, xpcshell httpd tests often appear to hang, or at best continue on
with no error message of any kind.   Catch exceptions in our base
ChannelListener class, so we get a proper error msg.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 405282 [details] [diff] [review]
Catch exceptions in template ChannelListener class to avoid hangs.

>From: Jason Duell <[e-mail]>
>
>Catch exceptions in template ChannelListener class to avoid hangs.
>
>
>diff --git a/netwerk/test/unit/head_channels.js b/netwerk/test/unit/head_channels.js
>--- a/netwerk/test/unit/head_channels.js
>+++ b/netwerk/test/unit/head_channels.js
>@@ -55,54 +55,69 @@ ChannelListener.prototype = {
>     if (iid.equals(Components.interfaces.nsIStreamListener) ||
>         iid.equals(Components.interfaces.nsIRequestObserver) ||
>         iid.equals(Components.interfaces.nsISupports))
>       return this;
>     throw Components.results.NS_ERROR_NO_INTERFACE;
>   },
> 
>   onStartRequest: function(request, context) {
>-    if (this._got_onstartrequest)
>-      do_throw("Got second onStartRequest event!");
>-    this._got_onstartrequest = true;
>+    try {
>+      if (this._got_onstartrequest)
>+        do_throw("Got second onStartRequest event!");
>+      this._got_onstartrequest = true;
> 
>-    request.QueryInterface(Components.interfaces.nsIChannel);
>-    this._contentLen = request.contentLength;
>-    if (this._contentLen == -1)
>-      do_throw("Content length is unknown in onStartRequest!");
>+      request.QueryInterface(Components.interfaces.nsIChannel);
>+      this._contentLen = request.contentLength;
>+      if (this._contentLen == -1)
>+        do_throw("Content length is unknown in onStartRequest!");
>+    } catch (ex) {
>+      do_throw("Error in onStartRequest: " + ex);
>+    }
>   },
> 
>   onDataAvailable: function(request, context, stream, offset, count) {
>-    if (!this._got_onstartrequest)
>-      do_throw("onDataAvailable without onStartRequest event!");
>-    if (this._got_onstoprequest)
>-      do_throw("onDataAvailable after onStopRequest event!");
>-    if (!request.isPending())
>-      do_throw("request reports itself as not pending from onDataAvailable!");
>-    if (this._flags & CL_EXPECT_FAILURE)
>-      do_throw("Got data despite expecting a failure");
>+    try {
>+      if (!this._got_onstartrequest)
>+        do_throw("onDataAvailable without onStartRequest event!");
>+      if (this._got_onstoprequest)
>+        do_throw("onDataAvailable after onStopRequest event!");
>+      if (!request.isPending())
>+        do_throw("request reports itself as not pending from onDataAvailable!");
>+      if (this._flags & CL_EXPECT_FAILURE)
>+        do_throw("Got data despite expecting a failure");
> 
>-    this._buffer = this._buffer.concat(read_stream(stream, count));
>+      this._buffer = this._buffer.concat(read_stream(stream, count));
>+    } catch (ex) {
>+      do_throw("Error in onDataAvailable: " + ex);
>+    }
>   },
> 
>   onStopRequest: function(request, context, status) {
>-    if (!this._got_onstartrequest)
>-      do_throw("onStopRequest without onStartRequest event!");
>-    if (this._got_onstoprequest)
>-      do_throw("Got second onStopRequest event!");
>-    this._got_onstoprequest = true;
>-    var success = Components.isSuccessCode(status);
>-    if ((this._flags & CL_EXPECT_FAILURE) && success)
>-      do_throw("Should have failed to load URL (status is " + status.toString(16) + ")");
>-    else if (!(this._flags & CL_EXPECT_FAILURE) && !success)
>-      do_throw("Failed to load URL: " + status.toString(16));
>-    if (status != request.status)
>-      do_throw("request.status does not match status arg to onStopRequest!");
>-    if (request.isPending())
>-      do_throw("request reports itself as pending from onStopRequest!");
>-    if (!(this._flags & CL_EXPECT_FAILURE) &&
>-	!(this._flags & CL_EXPECT_GZIP) &&
>-        this._contentLen != -1)
>-	do_check_eq(this._buffer.length, this._contentLen)
>-
>-    this._closure(request, this._buffer, this._closurectx);
>+    try {
>+      if (!this._got_onstartrequest)
>+        do_throw("onStopRequest without onStartRequest event!");
>+      if (this._got_onstoprequest)
>+        do_throw("Got second onStopRequest event!");
>+      this._got_onstoprequest = true;
>+      var success = Components.isSuccessCode(status);
>+      if ((this._flags & CL_EXPECT_FAILURE) && success)
>+        do_throw("Should have failed to load URL (status is " + status.toString(16) + ")");
>+      else if (!(this._flags & CL_EXPECT_FAILURE) && !success)
>+        do_throw("Failed to load URL: " + status.toString(16));
>+      if (status != request.status)
>+        do_throw("request.status does not match status arg to onStopRequest!");
>+      if (request.isPending())
>+        do_throw("request reports itself as pending from onStopRequest!");
>+      if (!(this._flags & CL_EXPECT_FAILURE) &&
>+          !(this._flags & CL_EXPECT_GZIP) &&
>+          this._contentLen != -1)
>+          do_check_eq(this._buffer.length, this._contentLen)
>+    } catch (ex) {
>+      do_throw("Error in onStopRequest: " + ex);
>+    }
>+    try {
>+      this._closure(request, this._buffer, this._closurectx);
>+    } catch (ex) {
>+      do_throw("Error in closure function: " + ex);
>+    }
>   }
> };

Comment 2

[Jason Duell]

Note that we should probably NOT check this patch in yet, as it turns out to expose some bugs in some of the existing necko tests (bug 525588).

Do we run the xpcshell tests as part of any of our nightly/tinderbox runs?  If not, I suppose we /could/ check this in before we actually fix the test bugs.

Comment 3

[Jason Duell]

This can get checked in.  Make sure the patch for bug 525588 lands first or at the same time.

Comment 4

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/b04f5b4ad14b