ExtensionProtocolHandler::SubstituteChannel, which is called from SubstitutingProtocolHandler::NewChannel2, actuall calls AsyncOpen2 on the passed-in channel before returning. This is quite weird. Creating a channel does NOT mean it's OK to open it (or whatever its underlying transport is). At this point security checks haven't been done properly yet, the loadinfo may not be set up properly yet, etc. It _might_ be kind of safe in this case because this is always doing a file:// load (is it?) and the result can't ever be observed if the outer channel is never opened (is that true?), but it's still really odd and expectation-violating behavior.... Yes, I know this was even worse before bug 1348442 was fixed; I appreciate that that was done!

Basically because I didn't have time for a better option at the time. My preference would have been to create a new channel type that opened tye underlying channel and wrapped it in a converter when it was opened. That's still my plan, and I'll do it when I have the time, and someone has time to review it. But there's never any actual network activity. The wrapped stream is always either a local file: channel or a local jar: channel. I still don't like opening it as soon as the channel is created, but in the end I decided it was a good compromise (especially since we do similar things elsewhere in the tree).

> especially since we do similar things elsewhere in the tree We do? Where? :(

I don't know if there's anywhere else we open an underlying channel, but there are a bunch of other places where we open a stream for an underlying local resource, e.g.: http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/components/places/nsAnnoProtocolHandler.cpp#330-367 If you'd be willing to review it, I'll work on a channel-based solution to this over the next week or two. I could probably just create a callback-based version of nsIStreamChannel that calls a function to get the input stream when Open2/AsyncOpen2 is called. Or something along those lines. It would be great if we didn't need to use the pipe for the async version...

I'm happy to review, yes. Least I can do in this case! ;)

Boris, can you please review these changes? Let me know if you want me to find a necko and places peer to review those changes, but they should be pretty straightforward. The NS_TRY and TRY_VAR macros are a bit weird, so I'd be happy to remove them. I'm still trying to figure out how to handle code that mixes Result and nsresult cleanly. There are still a bunch of other places that we should do something similar to this, including: http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/netwerk/protocol/about/nsAboutCacheEntry.cpp#132 http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/netwerk/protocol/about/nsAboutCache.cpp#81 http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/image/decoders/icon/gtk/nsIconChannel.cpp#39 http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/image/decoders/icon/android/nsIconChannel.cpp#64 But I don't have the time to fix all of them right now.

I'd really prefer that a necko peer reviewed the necko bits. For the places bits, I'm happy to do it if the places peers want to delegate, but figure they should at least be in the loop.

Marco, are you happy for bz to review these moz-anno: protocol handler changes?

Comment on attachment 8853572 [details] Bug 1351163: Part 1 - Support direct stream listener output in nsBaseChannel. Honza: I'm sorry to pile stuff on you but I can't think of a better reviewer. Feel free to delegate if you can.

(In reply to Kris Maglione [:kmag] from comment #11) > Marco, are you happy for bz to review these moz-anno: protocol handler > changes? Sure, I could never say no to bz :) Actually, I'll have to port these changes to my patch in bug 977177, so I hope this lands soon. Thank you for the heads up!

Sorry for the question, is there a solution also for js implementations, or are we supposed to only use cpp for these more complex cases?

(In reply to Marco Bonardo [::mak] from comment #14) > Sorry for the question, is there a solution also for js implementations, or > are we supposed to only use cpp for these more complex cases? I had considered that, but I didn't want to implement it as part of this bug. It would be easy enough to support JS implementations with a [function] interface for the callbacks. I can implement that after this lands if you have a use for it.

Places has another protocol (page-icon) that is quite similar to moz-anno, but implemented in js, and its code looks nicer to maintain, so I was evaluating also converting the moz-anno handler to js. But I could even have both in cpp, it's not that complex anyway, I just need to figure out the right path forward, and that's why I was asking.

As long as the channels don't require any non-standard URL parsing for newURI, I think moving them to JS should be fine. I don't really have a strong opinion either way. So if you need this functionality in JS, just let me know and I'll implement it.

(In reply to Kris Maglione [:kmag] from comment #17) > So if you need this functionality in JS, just let > me know and I'll implement it. It would be great, the only other alternative I'd have to port this bugfix to the other protocol would be to rewrite the other protocol in cpp. Not a big deal but I'd avoid it if possible.

Comment on attachment 8853574 [details] Bug 1351163: Part 3 - Don't open moz-extension: base channel on NewChannel2(). https://reviewboard.mozilla.org/r/125634/#review129780 ::: netwerk/protocol/res/ExtensionProtocolHandler.cpp:147 (Diff revision 1) > > // Filter CSS files to replace locale message tokens with localized strings. > > + nsCOMPtr<nsIChannel> channel = NS_NewSimpleChannel( > + aURI, aLoadInfo, *result, > + [=] (nsIStreamListener* listener, nsIChannel* channel, nsIChannel* origChannel) -> RequestOrReason { Do we need to capture anything other than "channel"? That said, if the channel holds on to the lambda and the lambda keeps the channel alive, how does this not leak?

Comment on attachment 8853573 [details] Bug 1351163: Part 2 - Add a nsSimpleChannel helper class for creating simple wrapper channels. https://reviewboard.mozilla.org/r/125632/#review129782 ::: netwerk/base/nsSimpleChannel.cpp:57 (Diff revision 1) > + nsCOMPtr<nsIInputStream> stream; > + TRY_VAR(stream, mCallbacks->OpenContentStream(async, this)); > + MOZ_ASSERT(stream); > + > + *channel = nullptr; > + *streamOut = stream.forget().take(); stream.forget(streamout); ::: netwerk/base/nsSimpleChannel.cpp:67 (Diff revision 1) > +nsSimpleChannel::BeginAsyncRead(nsIStreamListener* listener, nsIRequest** request) > +{ > + nsCOMPtr<nsIRequest> req; > + TRY_VAR(req, mCallbacks->StartAsyncRead(listener, this)); > + > + *request = req.forget().take(); req.forget(request);

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review129784 r=me with the nits below fixed. ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:132 (Diff revision 1) > + > + rv = pump->AsyncRead(mListener, nullptr); > NS_ENSURE_SUCCESS(rv, rv); > > // At this point, we should have written out all of our data to our stream. > // HandleCompletion will close the output stream, so we are done here. What output stream? ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:165 (Diff revision 1) > protected: > virtual ~faviconAsyncLoader() {} > > private: > nsCOMPtr<nsIChannel> mChannel; > - nsCOMPtr<nsIOutputStream> mOutputStream; > + nsCOMPtr<nsIStreamListener> mListener; Should we null this ref out at some point, to prevent cycles leaking through it? ::: toolkit/components/places/nsAnnoProtocolHandler.cpp:303 (Diff revision 1) > - > // Create our channel. We'll call SetContentType with the right type when > // we know what it actually is. > - nsCOMPtr<nsIChannel> channel; > - rv = NS_NewInputStreamChannelInternal(getter_AddRefs(channel), > - aURI, > + nsCOMPtr<nsIChannel> channel = NS_NewSimpleChannel( > + aURI, aLoadInfo, aAnnotationURI, > + [=] (nsIStreamListener* listener, nsIChannel* channel, nsIURI* annotationURI) { Does this need to capture anything at all? ::: toolkit/components/places/tests/favicons/test_moz-anno_favicon_mime_type.js:42 (Diff revision 1) > do_check_true(this._checked); > do_test_finished(); > }, > onDataAvailable(aRequest, aContext, aInputStream, aOffset, aCount) { > aRequest.cancel(Cr.NS_ERROR_ABORT); > + throw Cr.NS_ERROR_ABORT; This might lead to browser console spew. Did you check for that?

Comment on attachment 8853573 [details] Bug 1351163: Part 2 - Add a nsSimpleChannel helper class for creating simple wrapper channels. https://reviewboard.mozilla.org/r/125632/#review129794

Comment on attachment 8853574 [details] Bug 1351163: Part 3 - Don't open moz-extension: base channel on NewChannel2(). https://reviewboard.mozilla.org/r/125634/#review129780 > Do we need to capture anything other than "channel"? > > That said, if the channel holds on to the lambda and the lambda keeps the channel alive, how does this not leak? The lambda doesn't capture the channel. It actually doesn't capture anything, so that should probably be made explicit.

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review129784 > Should we null this ref out at some point, to prevent cycles leaking through it? Probably. It couldn't hurt, anyway. > Does this need to capture anything at all? Nope. > This might lead to browser console spew. Did you check for that? I didn't notice any when I ran the test, so if it was there, it probably wasn't noisy enough to be a problem.

> It actually doesn't capture anything Oh, "channel" in the lambda is an arg, not the thing it could be capturing. That's definitely very confusing. Explicitly capturing nothing would be _much_ clearer!

Comment on attachment 8853574 [details] Bug 1351163: Part 3 - Don't open moz-extension: base channel on NewChannel2(). https://reviewboard.mozilla.org/r/125634/#review129802 r=me with the lambda changed to eplicitly capture nothing.

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review129784 > I didn't notice any when I ran the test, so if it was there, it probably wasn't noisy enough to be a problem. No, I meant if you exercise this code, does a message show up there? If so, we can change this so the message won't show up. But I want to know whether it's a problem before adding complications. Fwiw, any noise at all when there shouldn't be any is a problem. It's not a matter of volume, but of debuggability.

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review129784 > No, I meant if you exercise this code, does a message show up there? > > If so, we can change this so the message won't show up. But I want to know whether it's a problem before adding complications. > > Fwiw, any noise at all when there shouldn't be any is a problem. It's not a matter of volume, but of debuggability. I think a message shows up any time a JS request listener method throws an exception. I'm not sure exactly what the output is here, but I can rebuild this branch tomorrow and check. Without throwing here, the stream pump code complains that the listener didn't read any data. But this is only really an issue with the test code, which wants to check the mime type of the request and then cancel it. In normal use, this should behave the same as any other request listener.

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review129784 > I think a message shows up any time a JS request listener method throws an exception. I'm not sure exactly what the output is here, but I can rebuild this branch tomorrow and check. Without throwing here, the stream pump code complains that the listener didn't read any data. > > But this is only really an issue with the test code, which wants to check the mime type of the request and then cancel it. In normal use, this should behave the same as any other request listener. Yeah, the error crossing the XPConnect boundary gets reported (which I don't think we have a solution to), but the failure value in native code doesn't: 0:13.80 PROCESS_OUTPUT: Thread-1 (pid:7834) "JavaScript error: /home/kris/code/mozilla-central/testing/xpcshell/head.js, line 211: uncaught exception: 2147500036"

Comment on attachment 8853574 [details] Bug 1351163: Part 3 - Don't open moz-extension: base channel on NewChannel2(). https://reviewboard.mozilla.org/r/125634/#review129780 > The lambda doesn't capture the channel. It actually doesn't capture anything, so that should probably be made explicit. Oh, it does capture `aLoadInfo`, and uses it to decide whether to call `AsyncOpen` or `AsyncOpen2`. But it only captures the raw pointer, so it doesn't create a reference cycle.

I assume that's safe because the lambda only lives as long as the channel does, and the channel keeps the loadinfo alive? Worth a comment....

Well, in this case it's safe either way, because we're only checking whether the pointer is null in the lambda. In the moz-anno: case we're also capturing aLoadInfo, and using it if we fallback to the default icon. In that case, it's safe because the channel is holding it alive. I'll add a comment about that.

Comment on attachment 8853575 [details] Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. https://reviewboard.mozilla.org/r/125636/#review130018 r=me

Comment on attachment 8853572 [details] Bug 1351163: Part 1 - Support direct stream listener output in nsBaseChannel. https://reviewboard.mozilla.org/r/125630/#review130432 r+ with the comments addressed ::: netwerk/base/nsBaseChannel.cpp:235 (Diff revision 1) > } > > nsresult > nsBaseChannel::BeginPumpingData() > { > + nsresult rv = BeginAsyncRead(this, getter_AddRefs(mRequest)); nit: please as following: nsrsult rv; rv = BeginAsync... ::: netwerk/base/nsBaseChannel.cpp:238 (Diff revision 1) > nsBaseChannel::BeginPumpingData() > { > + nsresult rv = BeginAsyncRead(this, getter_AddRefs(mRequest)); > + if (NS_SUCCEEDED(rv)) { > + mPumpingData = true; > + return rv; return NS_OK; ::: netwerk/base/nsBaseChannel.cpp:241 (Diff revision 1) > + if (NS_SUCCEEDED(rv)) { > + mPumpingData = true; > + return rv; > + } > + if (rv != NS_ERROR_NOT_IMPLEMENTED) > + return rv; if () { } ::: netwerk/base/nsInputStreamPump.cpp:243 (Diff revision 1) > > LOG(("nsInputStreamPump::Resume [this=%p]\n", this)); > NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED); > NS_ENSURE_TRUE(mState != STATE_IDLE, NS_ERROR_UNEXPECTED); > > - if (--mSuspendCount == 0) > + if (--mSuspendCount == 0 && mAsyncStream) this needs some explanation (a comment maybe). and I'm not entirely sure how is this change related to this patch.

Comment on attachment 8853573 [details] Bug 1351163: Part 2 - Add a nsSimpleChannel helper class for creating simple wrapper channels. https://reviewboard.mozilla.org/r/125632/#review130444 yum, like this. ::: netwerk/base/moz.build:156 (Diff revision 2) > 'nsInputStreamPump.h', > 'nsMIMEInputStream.h', > 'nsNetUtil.h', > 'nsReadLine.h', > 'nsSerializationHelper.h', > + 'nsSimpleChannel.h', should be SimpleChannel (no ns prefix), but up to you to change it or leave it. same applies to the class name (should be SimpleChannel) ::: netwerk/base/nsSimpleChannel.cpp:40 (Diff revision 2) > + > + virtual nsresult BeginAsyncRead(nsIStreamListener* listener, > + nsIRequest** request) override; > + > +private: > + UniquePtr<nsSimpleChannelCallbacks> mCallbacks;; should be 2 space indent only two ;; the context is held during the whole channel's (callback's) lifetime. should we throw it away after it's no longer used? Although, not sure if we can easily put the obligation of self-reference on the consumer... this way I'm afraid of leaks (cycle of channel <-> context), but it's probably better telling people to either not hold a ref to the channel or destroy it when they are done with it than risk hard to predict crashes with use after free. ::: netwerk/base/nsSyncStreamListener.cpp:11 (Diff revision 2) > #include "nsSyncStreamListener.h" > #include "nsIPipe.h" > #include "nsThreadUtils.h" > #include <algorithm> > > +using namespace mozilla::net; why?

Comment on attachment 8853573 [details] Bug 1351163: Part 2 - Add a nsSimpleChannel helper class for creating simple wrapper channels. https://reviewboard.mozilla.org/r/125632/#review130444 > should be 2 space indent only > > two ;; > > the context is held during the whole channel's (callback's) lifetime. should we throw it away after it's no longer used? Although, not sure if we can easily put the obligation of self-reference on the consumer... > > this way I'm afraid of leaks (cycle of channel <-> context), but it's probably better telling people to either not hold a ref to the channel or destroy it when they are done with it than risk hard to predict crashes with use after free. Yeah, I was a bit worried about that, too. We should probably null the reference after the first successfull callback, and make sure that's documented. If it needs to be kept alive after the callback is called, then the reference should be kept by whatever's still holding it. > why? When I added a new file to the unified sources, the chunking changed, and this wound up failing because it was depending on this having happened in an earlier source.

Comment on attachment 8853572 [details] Bug 1351163: Part 1 - Support direct stream listener output in nsBaseChannel. https://reviewboard.mozilla.org/r/125630/#review130432 > this needs some explanation (a comment maybe). and I'm not entirely sure how is this change related to this patch. Before this change, we only ever suspended/resumed the input stream pump that we created directly. After it, we sometimes call it on an underlying channel, which may not always be in the state we expect. And there happens to be a weird intermediate state in the pump's OnStateStop callback, where it sets mAsyncStream to null, dispatches OnStopRequest, and *then* sets the state to STATE_IDLE. Which means if we call Resume() at that point, we get crashes. This winds up happening when the underlying channels OnStopRequest listener calls nsBaseChannel's OnStartRequest listener.

(In reply to Kris Maglione [:kmag] from comment #32) > In the moz-anno: case we're also capturing aLoadInfo, and using it if we > fallback to the default icon. In that case, it's safe because the channel is > holding it alive. I'll add a comment about that. It turns out that the clang plugin wasn't willing to accept my word on this, so I used the reference from the channel rather than a lambda capture.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ebba58bac18e05edc26c318c08a5ec5f4252ab3 Bug 1351163: Part 1 - Support direct stream listener output in nsBaseChannel. r=honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/48d6ab7dafea23eeb87eeddf428a63b9a43db6fc Bug 1351163: Part 2 - Add a nsSimpleChannel helper class for creating simple wrapper channels. r=bz,honzab https://hg.mozilla.org/integration/mozilla-inbound/rev/148913e0461823a3adcb152be5148920e14e56bc Bug 1351163: Part 3 - Don't open moz-extension: base channel on NewChannel2(). r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/6be92f49166ce550e6bbe6ce931fb9367703b1ee Bug 1351163: Part 4 - Don't start moz-anno: requests until the channel is opened. r=bz

https://hg.mozilla.org/mozilla-central/rev/9ebba58bac18 https://hg.mozilla.org/mozilla-central/rev/48d6ab7dafea https://hg.mozilla.org/mozilla-central/rev/148913e04618 https://hg.mozilla.org/mozilla-central/rev/6be92f49166c