372486 - Document and enforce that channels can't be reopened

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Sylvain Pasche]

See bug 369787, commment 2 and bug 369787, commment 3.

Comment 1

[Sylvain Pasche]

oops, I meant:

See bug 369787, comment 2 and bug 369787, comment 3.

Comment 2

[Sylvain Pasche]

Attachment: version 1

Flag based approach.

I'm not setting mWasOpened in nsHttpChannel.cpp in all situations, like redirects or errors. Is reopening bad in these cases?

While duplicating some code in the unit test, I was wondering if a netwerk/test/unit/head_utils.js for common functions would be better.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 257414 [details] [diff] [review]
version 1

why don't you just set mWasOpened in AsyncOpen? I think you should be consistent and set it in all cases.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 257414 [details] [diff] [review]
version 1

>+     * @throw NS_ERROR_ALREADY_OPENED if channel is reopened.

>+     * @throw NS_ERROR_ALREADY_OPENED if channel is reopened.

throws

>Index: base/src/nsBaseChannel.cpp

>+  if (!NS_FAILED(rv))
>+    mWasOpened = PR_TRUE;

mWasOpened = NS_SUCCEEDED(rv);


>Index: test/unit/test_reopen.js

>+function makeBIS(stream) {
>+  var bis = Cc["@mozilla.org/binaryinputstream;1"]
>+              .createInstance(Ci.nsIBinaryInputStream);
>+  bis.setInputStream(stream);
>+  return bis;
>+}

var BinaryInputStream = Components.Constructor("@mozilla.org/binaryinputstream;1",
                                               "nsIBinaryInputStream",
                                               "init");

is more efficient and lets you do |new BinaryInputStream(stream)|.  (The server doesn't do that yet because I haven't finished the patch to make it so yet.)


>+function test_file_channel() {
>+  var file = getFile("XpcomLib");
>+  test_channel(function() {
>+    return new_file_channel(file);
>+  });
>+}

I'd just use |do_get_file("netwerk/test/Makefile.in")| and not bother with XPCOM stuff.

Comment 5

[Sylvain Pasche]

(In reply to comment #3)
> (From update of attachment 257414 [details] [diff] [review])
> why don't you just set mWasOpened in AsyncOpen? I think you should be
> consistent and set it in all cases.

Indeed, that would make things more simple and consistent. I was first thinking of using a state which needed to be changed only when mIsPending went from true to false, but this can be done more simply with the flag, thanks for the comment.

Jeff: thanks for your comments, too. I'll write a new version later.

Comment 6

[Sylvain Pasche]

Attachment: version 2

Version 2, addressing comments

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 257570 [details] [diff] [review]
version 2

>Index: test/unit/test_reopen.js

>+function makeBIS(stream) {
>+  var BinaryInputStream =
>+            Components.Constructor("@mozilla.org/binaryinputstream;1",
>+                                   "nsIBinaryInputStream", "setInputStream");
>+  return new BinaryInputStream(stream);
>+}

>+  onDataAvailable: function test_ODA(request, cx, inputStream,
>+                                     offset, count) {
>+    makeBIS(inputStream).readByteArray(count); // required by API
>+    check_async_open_throws(NS_ERROR_IN_PROGRESS);
>+  },

So what I *meant* here was to eliminate |makeBIS| entirely and just change the line in |onDataAvailable| to:

new BinaryInputStream(inputStream).readByteArray(count);

Half the benefit of using |Components.Constructor| is that the arguments are cached and calling the returned constructor is faster (because you don't have to cross the XPConnect boundary three times), but if you have to re-run it for every |makeBIS| you lose that advantage.

Comment 8

[Sylvain Pasche]

Attachment: version 3

Thanks for the explanations, that new version should make more sense.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 257584 [details] [diff] [review]
version 3

test/unit/test_reopen.js
+  return chan = ios.newChannel(url, null, null)
+                .QueryInterface(Ci.nsIChannel);

please align the two dots here

it'd be nice to also test that asyncOpen after open throws, and open after asyncOpen

Comment 10

[Darin Fisher]

Comment on attachment 257584 [details] [diff] [review]
version 3

looks fine, but i noticed that sometimes you set mWasOpened to true immediately even if the function could still fail, but in other cases you only set it once you know that the function is going to succeed.  might be better to be consistent?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

ah, yeah... please fix that :)

Comment 12

[Sylvain Pasche]

Attachment: version 4

This should be more consistent, in not setting the flag in case of failure.

Oddly, the ftp channel test is now failing with "ASSERTION: This XPCOM object fails in QueryInterface to nsISupports". Where the object is a nsFtpState:

#0  0x9003d1dc in kill ()
#1  0x9010f2af in raise ()
#2  0x9010de02 in abort ()
#3  0x004e83bc in PR_Abort () at /Users/sypasche/moz/trunk/mozilla/nsprpub/pr/src/io/prlog.c:513
#4  0x010765ca in Abort (aMsg=0xbfffc5bc "###!!! ASSERTION: This XPCOM object fails in QueryInterface to nsISupports!: 'Error', file /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 314") at /Users/sypasche/moz/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:363
#5  0x010768dd in NS_DebugBreak_P (aSeverity=1, aStr=0x286658c "This XPCOM object fails in QueryInterface to nsISupports!", aExpr=0x285ffbc "Error", aFile=0x28655e8 "/Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp", aLine=314) at /Users/sypasche/moz/trunk/mozilla/xpcom/base/nsDebugImpl.cpp:346
#6  0x0284952e in XPCWrappedNative::GetNewOrUsed (ccx=@0xbfffcd18, Object=0x688e40, Scope=0x61fda0, Interface=0x640420, isGlobal=0, resultWrapper=0xbfffcb54) at /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:314
#7  0x028232ba in XPCConvert::NativeInterface2JSObject (ccx=@0xbfffcd18, dest=0xbfffcc2c, src=0x688e40, iid=0xbfffceac, scope=0x207c3e0, allowNativeWrapper=1, isGlobal=0, pErr=0x0) at /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcconvert.cpp:1098
#8  0x028277aa in XPCConvert::NativeData2JS (ccx=@0xbfffcd18, d=0xbfffce58, s=0xbfffd084, type=@0xbfffcedd, iid=0xbfffceac, scope=0x207c3e0, pErr=0x0) at /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcconvert.cpp:472
#9  0x0284161b in nsXPCWrappedJSClass::CallMethod (this=0x689a90, wrapper=0x689ac0, methodIndex=5, info=0x20514e0, nativeParams=0xbfffd074) at /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1303
#10 0x0283a7bd in nsXPCWrappedJS::CallMethod (this=0x689ac0, methodIndex=5, info=0x20514e0, params=0xbfffd074) at /Users/sypasche/moz/trunk/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:571
#11 0x0107e0ea in PrepareAndDispatch (self=0x68d270, methodIndex=5, args=0xbfffd194) at /Users/sypasche/moz/trunk/mozilla/xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
#12 0x0107e1ec in nsXPTCStubBase::Stub5 (this=0x68d270) at ../../../../../../dist/include/xpcom/xptcstubsdef.inc:7
#13 0x03345eba in nsBaseChannel::OnDataAvailable (this=0x6899c0, request=0x689b20, ctxt=0x0, stream=0x688e40, offset=0, count=1144) at /Users/sypasche/moz/trunk/mozilla/netwerk/base/src/nsBaseChannel.cpp:647

Comment 13

[Sylvain Pasche]

Attachment: version 5

Added a NS_INTERFACE_MAP_ENTRY_AMBIGUOUS entry in nsBaseContentStream, as proposed by biesi to fix the ftp issue.

Depending on whether the implementation uses NS_ImplementChannelOpen or not has effect on the error returned by open() in case the channel is already opened (NS_ERROR_ALREADY_OPENED or NS_ERROR_IN_PROGRESS). I don't think this is a big issue, isn't it? I changed the test accordingly.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 258233 [details] [diff] [review]
version 5

+     * @throws NS_ERROR_ALREADY_OPENED if channel is reopened.
      */
     nsIInputStream open();

so, if you document this as the error code, you should actually use it always. perhaps it would be better to use IN_PROGRESS for open always.

but anyway, since the interface is frozen, it's not so nice to impose new restrictions on existing implementors. Perhaps this should instead say something like:

"Implementations should throw NS_ERROR_IN_PROGRESS if they are reopened."

similar for asyncOpen (can stay with ALREADY_OPENED there)

     return NS_ImplementChannelOpen(this, result);
+    
+  mWasOpened = NS_SUCCEEDED(rv);

please don't add trailing whitespace (second line here)

Comment 15

[Sylvain Pasche]

Attachment: version 6

Using NS_ERROR_ALREADY_OPENED only for asyncOpen, updated doc/test

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 258245 [details] [diff] [review]
version 6

+     * NOTE: Implementations should throw NS_ERROR_IN_PROGRESS if channel is
+     * reopened.

I think that should be "the channel" instead of "channel", in both comments

looks good otherwise, thanks!

Comment 17

[Sylvain Pasche]

Attachment: version 7

Here it is. Thanks for your reviews.

Comment 18

[Sylvain Pasche]

So is this latest version ok for commit?

If so, could you land it for me please? Thanks.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

RCS file: /cvsroot/mozilla/netwerk/test/unit/test_reopen.js,v
done
Checking in test/unit/test_reopen.js;
/cvsroot/mozilla/netwerk/test/unit/test_reopen.js,v  <--  test_reopen.js
initial revision: 1.1
done
Checking in base/public/nsIChannel.idl;
/cvsroot/mozilla/netwerk/base/public/nsIChannel.idl,v  <--  nsIChannel.idl
new revision: 1.68; previous revision: 1.67
done
Checking in base/public/nsNetError.h;
/cvsroot/mozilla/netwerk/base/public/nsNetError.h,v  <--  nsNetError.h
new revision: 1.7; previous revision: 1.6
done
Checking in base/src/nsBaseChannel.cpp;
/cvsroot/mozilla/netwerk/base/src/nsBaseChannel.cpp,v  <--  nsBaseChannel.cpp
new revision: 1.13; previous revision: 1.12
done
Checking in base/src/nsBaseChannel.h;
/cvsroot/mozilla/netwerk/base/src/nsBaseChannel.h,v  <--  nsBaseChannel.h
new revision: 1.8; previous revision: 1.7
done
Checking in base/src/nsBaseContentStream.cpp;
/cvsroot/mozilla/netwerk/base/src/nsBaseContentStream.cpp,v  <--  nsBaseContentStream.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.307; previous revision: 1.306
done
Checking in protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.89; previous revision: 1.88
done

Comment 20

[Jeff Walden [:Waldo]]

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/netwerk/test/unit/test_reopen.js&root=/cvsroot