Document and enforce that channels can't be reopened

RESOLVED FIXED

Status

()

Core
Networking
--
enhancement
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Sylvain Pasche, Assigned: Sylvain Pasche)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

11 years ago
See bug 369787, commment 2 and bug 369787, commment 3.
(Assignee)

Comment 1

11 years ago
oops, I meant:

See bug 369787, comment 2 and bug 369787, comment 3.
(Assignee)

Comment 2

11 years ago
Created attachment 257414 [details] [diff] [review]
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.
Attachment #257414 - Flags: review?(cbiesinger)
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

11 years ago
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.
(Assignee)

Comment 5

11 years ago
(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.
(Assignee)

Comment 6

11 years ago
Created attachment 257570 [details] [diff] [review]
version 2

Version 2, addressing comments
Attachment #257414 - Attachment is obsolete: true
Attachment #257570 - Flags: review?(cbiesinger)
Attachment #257414 - Flags: review?(cbiesinger)

Comment 7

11 years ago
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.
(Assignee)

Comment 8

11 years ago
Created attachment 257584 [details] [diff] [review]
version 3

Thanks for the explanations, that new version should make more sense.
Attachment #257570 - Attachment is obsolete: true
Attachment #257584 - Flags: review?(cbiesinger)
Attachment #257570 - Flags: review?(cbiesinger)
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
Attachment #257584 - Flags: superreview?(darin.moz)
Attachment #257584 - Flags: review?(cbiesinger)
Attachment #257584 - Flags: review+

Comment 10

11 years ago
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?
Attachment #257584 - Flags: superreview?(darin.moz) → superreview+
ah, yeah... please fix that :)
(Assignee)

Comment 12

11 years ago
Created attachment 258215 [details] [diff] [review]
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
Attachment #257584 - Attachment is obsolete: true
(Assignee)

Comment 13

11 years ago
Created attachment 258233 [details] [diff] [review]
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.
Attachment #258215 - Attachment is obsolete: true
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)
(Assignee)

Comment 15

11 years ago
Created attachment 258245 [details] [diff] [review]
version 6

Using NS_ERROR_ALREADY_OPENED only for asyncOpen, updated doc/test
Attachment #258233 - Attachment is obsolete: true
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!
(Assignee)

Comment 17

11 years ago
Created attachment 258251 [details] [diff] [review]
version 7

Here it is. Thanks for your reviews.
Attachment #258245 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
So is this latest version ok for commit?

If so, could you land it for me please? Thanks.
Assignee: nobody → sylvain.pasche
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
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.