Closed Bug 521258 Opened 11 years ago Closed 11 years ago

Catch exceptions in template ChannelListener class to avoid hangs.

Categories

(Core :: Networking, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

References

Details

Attachments

(1 file)

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.
Attachment #405282 - Flags: review?(jwalden+bmo)
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 405282 [details] [diff] [review]
Catch exceptions in template ChannelListener class to avoid hangs.

>From: Jason Duell <jduell.mcbugs@gmail.com>
>
>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);
>+    }
>   }
> };
Attachment #405282 - Flags: review?(jwalden+bmo) → review+
Depends on: 525588
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.
This can get checked in.  Make sure the patch for bug 525588 lands first or at the same time.
Keywords: checkin-needed
Assignee: nobody → jduell.mcbugs
http://hg.mozilla.org/mozilla-central/rev/b04f5b4ad14b
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
You need to log in before you can comment on or make changes to this bug.