After bug 1087720 lands, we should have a loadinfo on all gecko owned channels (created within C++ or JS). Before we can move security checks into AsyncOpen however, we have to implement a shim for addons. Addons can implement their own ProtocolHandlers and probably have not implemented newChannel2, or also AsyncOpen2(). In order to gracefully handle addons we need to implement a shim which forwards call to AsyncOpen in case AsyncOpen2() is not implemented.

Hey Jonas, when working on the shim I realized that we also might want to implement 'nsIWyciwygChannel'. Please note, that without inheriting 'nsIWyciwygChannel' everything compiles fine. When adding 'nsIWyciwygChannel' I get: > nsIOService.cpp:711:15: error: ambiguous conversion from derived class 'nsSecCheckWrapChannel' to base class 'nsIChannel': > class nsSecCheckWrapChannel -> class nsIWyciwygChannel -> class nsIChannel > class nsSecCheckWrapChannel -> class nsIHttpChannel -> class nsIChannel > channel = new nsSecCheckWrapChannel(channel, aLoadInfo); Interesting is that resolving the ambiguity works just fine for NS_ISUPPORTS as well as NS_IREQUEST. I am really don't know why > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWyciwygChannel) doesn't do the trick here. See the full mapping: > NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannel) > NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIWyciwygChannel, mWyciwygChannel) > NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel) > NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIWyciwygChannel) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIChannel, nsIWyciwygChannel) > NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIWyciwygChannel) > NS_INTERFACE_MAP_END * Any idea what might be wrong here? * Also, let me know if something else feels like it should updated, simplified, or extended!

Comment on attachment 8574950 [details] [diff] [review] basic_shim.patch Review of attachment 8574950 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +26,5 @@ > +NS_IMPL_ADDREF(nsSecCheckWrapChannel) > +NS_IMPL_RELEASE(nsSecCheckWrapChannel) > + > +NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannel) > + NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIWyciwygChannel, mWyciwygChannel) I doubt any addons implement nsIWyciwygChannel, so I don't think we should have that here. ::: netwerk/base/nsSecCheckWrapChannel.h @@ +40,5 @@ > +/* > + * Using the MACRO from dist/include/nsIChannel.h to forward calls to the underlaying channel. > + * The only methods we need to customize are the Get/SetLoadInfo bits. > + */ > +#define NS_FORWARD_SAFE_SECCHECKWRAP_NSICHANNEL(_to) \ Another solution is to insert an extra class. I.e. rename your main class to nsSecCheckWrapChannel_base and have that class forward all interfaces the same way you do now. Including forward all of nsIChannel. Then create a subclass called nsSecCheckWrapChannel which overrides Get/SetLoadInfo and has a ctor which just forwards the ctor arguments to nsSecCheckWrapChannel_base. Up to you. @@ +70,5 @@ > + , public nsIHttpChannelInternal > +{ > +public: > + NS_FORWARD_SAFE_NSIWYCIWYGCHANNEL(mWyciwygChannel) > + NS_FORWARD_SAFE_NSIHTTPCHANNEL(mHttpChannel) You shouldn't need to use the 'SAFE' versions of the macros. If mHttpChannel is null then these functions should never be called. @@ +72,5 @@ > +public: > + NS_FORWARD_SAFE_NSIWYCIWYGCHANNEL(mWyciwygChannel) > + NS_FORWARD_SAFE_NSIHTTPCHANNEL(mHttpChannel) > + NS_FORWARD_SAFE_NSIHTTPCHANNELINTERNAL(mHttpChannelInternal) > + NS_FORWARD_SAFE_SECCHECKWRAP_NSICHANNEL(mChannel) Likewise here. mChannel should never be null.

The problem isn't the QI macros. Those compile file. The problem is this line: channel = new nsSecCheckWrapChannel(channel, aLoadInfo); The compiler doesn't know how to cast an nsSecCheckWrapChannel to an nsIChannel since the compiler won't call QI. So you'll have to do something like: channel = static_cast<nsIHttpChannel>(new nsSecCheckWrapChannel(channel, aLoadInfo)); But really, remove the wyciwyg channel stuff. It shouldn't be there in the final code anyway.

Thanks Jonas for all your answers. Having a subclass sounds like the cleaner solution, especially if someone extends nsIChannel.idl in the future, then we don't have to worry about any manual updates to our self-defined MACRO. I will remove the wyciwyg channel again - but we should do a whole lot of testing before landing that patch - I am slightly worried at this time. Let's talk in our meeting on Wednesday.

Ok, only wrapping if newChannel2 is not available on the ProtocolHandler in the ioService. It seems to work fine after some initial testing, which included: * browsing the web * installing adBlock plus and browsing the web * running some mochitests Definitely needs more testing though!!! The only time I see some channels getting wrapped look like this one: nsSecCheckWrapChannelBase(file:///home/ckerschb/.cache/mozilla/firefox/...png) So, potentially we still forgot to update some code somewhere, I think we shouldn't wrap file channels - we have to investigate, because nsFileProtocolHandler seems correct to me.

I think this should be pretty much it - created and *.idl to give addons the ability to query the innerChannel from within our wrapper. This is not going to land for 39 but really early in the cycle for 40 so we have a full cycle available to shake out eventual bugs. Pushing to TRY as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=166fe8adf015

Comment on attachment 8576436 [details] [diff] [review] shim.patch Review of attachment 8576436 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +45,5 @@ > + > + { > + nsCOMPtr<nsIURI> uri; > + aChannel->GetURI(getter_AddRefs(uri)); > + nsAutoCString spec; This should be |#ifdef PR_LOGGING| @@ +80,5 @@ > +{ > + { > + nsCOMPtr<nsIURI> uri; > + mChannel->GetURI(getter_AddRefs(uri)); > + nsAutoCString spec; Same here. Or just remove this since the base class does it.

(In reply to Jonas Sicking (:sicking) from comment #10) > Comment on attachment 8576436 [details] [diff] [review] > shim.patch > > Review of attachment 8576436 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: netwerk/base/nsSecCheckWrapChannel.cpp > @@ +45,5 @@ > > + > > + { > > + nsCOMPtr<nsIURI> uri; > > + aChannel->GetURI(getter_AddRefs(uri)); > > + nsAutoCString spec; > > This should be |#ifdef PR_LOGGING| > > @@ +80,5 @@ > > +{ > > + { > > + nsCOMPtr<nsIURI> uri; > > + mChannel->GetURI(getter_AddRefs(uri)); > > + nsAutoCString spec; > > Same here. Or just remove this since the base class does it. Thanks Jonas, but I guess we are going to delete the PR_LOGGING anyway - it's just for testing the shim before landing.

Putting a MOZ_ASSERT(false) in the constructor of the channel wrapper. We need to make sure it's not called on TBPL (our own code) - otherwise we have to fix it before we land the shim: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38c638be43c8

We have to shake out the remaining problems here where we are still calling newChannel or do not propagate the same laodInfo instance on channels. Pushing this bug together with the patches of bug 1147562 to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17485a876539

Comment on attachment 8589790 [details] [diff] [review] bug_1120487_shim_wrapper.patch Review of attachment 8589790 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +26,5 @@ > + > +NS_INTERFACE_MAP_BEGIN(nsSecCheckWrapChannelBase) > + NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel) > + NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIHttpChannel) This one probably doesn't need to use the _AMBIGUOUS macro @@ +28,5 @@ > + NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannel, mHttpChannel) > + NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIHttpChannelInternal, mHttpChannelInternal) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsIRequest, nsIHttpChannel) > + NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIHttpChannel) > + NS_INTERFACE_MAP_ENTRY(nsISecCheckWrapChannel) You need an entry for nsIChannel as well

Comment on attachment 8589790 [details] [diff] [review] bug_1120487_shim_wrapper.patch Review of attachment 8589790 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Some nits just. r=me. ::: netwerk/base/nsISecCheckWrapChannel.idl @@ +18,5 @@ > +{ > + /** > + * Returns the wrapped channel inside this class. > + */ > + readonly attribute nsIChannel innerChannel; I know you and Jonas discussed this; I know there was some discussion about including this API. Since it will be a little unsafe for the addon devs to use this, do you want to call it unsafeInnerChannel? ::: netwerk/base/nsSecCheckWrapChannel.cpp @@ +41,5 @@ > + , mHttpChannel(do_QueryInterface(aChannel)) > + , mHttpChannelInternal(do_QueryInterface(aChannel)) > + , mRequest(do_QueryInterface(aChannel)) > +{ > + NS_ASSERTION(mChannel, "can not create a channel wrapper without a channel"); MOZ_ASSERT? @@ +55,5 @@ > + > +NS_IMETHODIMP > +nsSecCheckWrapChannelBase::GetInnerChannel(nsIChannel **aInnerChannel) > +{ > + NS_ADDREF(*aInnerChannel = mChannel); nit: I think the preferred way is to use 'forget' now. See https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp?from=HttpBaseChannel.cpp#1269 for an example. Or NS_IF_ADDREF :) @@ +92,5 @@ > + > +NS_IMETHODIMP > +nsSecCheckWrapChannel::GetLoadInfo(nsILoadInfo** aLoadInfo) > +{ > + CHANNELWRAPPERLOG(("nsSecCheckWrapChannel::GetLoadInfo()")); You talked offline about maybe deleting these. If you keep them, print the pointer or the uri. Make sure it links back to the constructors log too. ::: netwerk/base/nsSecCheckWrapChannel.h @@ +24,5 @@ > + * provides information about the content-type of the channel, > + * who initiated the load, etc. > + * > + * Addon created channels might *not* provide that loadInfo object for > + * some transition time before we mark the API as deprecated. Which API? The old newChannel? @@ +32,5 @@ > + * Please note that the wrapper only forwards calls for > + * * nsIRequest > + * * nsIChannel > + * * nsIHttpChannel > + * * nsIHttpChannelInternal Please add a small comment about the Get/SetInnerChannel, and say something about it being readonly. @@ +46,5 @@ > + NS_FORWARD_NSIHTTPCHANNELINTERNAL(mHttpChannelInternal->) > + NS_FORWARD_NSICHANNEL(mChannel->) > + NS_FORWARD_NSIREQUEST(mRequest->) > + NS_DECL_NSISECCHECKWRAPCHANNEL > + NS_DECL_ISUPPORTS I don't think these need to be threadsafe, so this is good right? @@ +54,5 @@ > +protected: > + virtual ~nsSecCheckWrapChannelBase(); > + > + nsCOMPtr<nsIChannel> mChannel; > + // we do a QI in the constructor "We do a QI in the constructor to set the following pointers." @@ +61,5 @@ > + nsCOMPtr<nsIRequest> mRequest; > +}; > + > + > +class nsSecCheckWrapChannel : public nsSecCheckWrapChannelBase Can you add a comment something like the following? "We define a separate class here to make it clear that we're overriding Get/SetLoadInfo, rather that using the forwarded implementations provided by NS_FORWARD_NSICHANNEL" My summary of what you told me offline :) Please amend as necessary.

> do you want to call it unsafeInnerChannel? What unsafetyness in particular are you thinking about?

(In reply to Jonas Sicking (:sicking) from comment #18) > > do you want to call it unsafeInnerChannel? > > What unsafetyness in particular are you thinking about? The addon calling wrapper.innerChannel.asyncOpen directly. No?

In the scenarios that we're worried about the addon is implementing the channel, but gecko is the one calling asyncOpen. So I don't think that's a concern. If the addon is the one calling asyncOpen, then hopefully it's also doing security checks. So then things should be quite fine.

Comment on attachment 8589789 [details] [diff] [review] bug_1120487_shim_ioservice.patch Review of attachment 8589789 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=me. ::: netwerk/base/nsIOService.cpp @@ +685,3 @@ > > + // Make sure that all the individual protocolhandlers attach a loadInfo. > + nsCOMPtr<nsILoadInfo> loadInfo; Can this be moved inside the if branch?

(In reply to Jonas Sicking (:sicking) from comment #20) > In the scenarios that we're worried about the addon is implementing the > channel, but gecko is the one calling asyncOpen. So I don't think that's a > concern. > > If the addon is the one calling asyncOpen, then hopefully it's also doing > security checks. So then things should be quite fine. OK - just wanted to flag it.

Thanks Steve, addressed all of your concerns, but still calling the innerChannel just 'innerChannel'. It think addon developers will understand that innerChannel represents the wrapped channel and if they mess with that channel than they might get unexpected behavior. Carrying over r+ from Jonas and Steve.

Carrying over r+ from Jonas and Steve.

If we decide to accept the TBPL assertions in Bug 1162657, then we should probably add a similar assertion to the constructor of a wrapped channel - because we shouldn't need to wrap any channels from within our own codebase.

Comment on attachment 8604266 [details] [diff] [review] bug_1120487_shim_ioservice.patch When adding the new patch I forgot to carry over the r+ from Steve and Jonas. Let's make sure those patches work on all machines before landing (has been some time since the last try run): https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf8997470811

Ups, seems like we have to mark the constructor as 'explicit' to make sure it compiles everywhere. Here we go. Otherwise try looks good so far. Carrying over r+ from Steve and Jonas.

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c588a14445 https://hg.mozilla.org/integration/mozilla-inbound/rev/357fcbe1de45

Backed out for card_manager_test.js Gaia unit test failures. https://treeherder.mozilla.org/logviewer.html#?job_id=10011964&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2fbba7e47c

Here is another TRY run where I put a MOZ_ASSERT and also a printf statement so we can somehow figure out what's going on here: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b52e3e99cbd James, Gaia unit tests are failing on > B2G Desktop Linux x64 opt it seems I can't query any log for 'Gu' tests. Do you know how I can access those logs? For the sake of completeness: on TBPL we shouldn't wrap any channels. Only for addon created channels where the protocolhandler of the addon does not provide a newChannel2 implementation. Potentially the problem is indeed covered by Bug 1166948. But again, I am not sure at the moment and would like to verify by inspecting the logs.

James, sorry, I forgot to set the needinfo flag, please see comment 31.

So I see your output in the logs (example: https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/Iaqp2yD9TU6Dum9epkU_OA/2/public/logs/live_backing.log) I am looking at "Gu" and clicking on the log link button in treeherder (the bars |||)

(In reply to James Lal [:lightsofapollo] Offline until May 26th from comment #33) > So I see your output in the logs (example: > https://s3-us-west-2.amazonaws.com/taskcluster-public-artifacts/ > Iaqp2yD9TU6Dum9epkU_OA/2/public/logs/live_backing.log) > > I am looking at "Gu" and clicking on the log link button in treeherder (the > bars |||) Thanks James.

So, the reason this patch got backed out was fixed in Bug 1166948. Within Gaia we were still using newChannel instead of newChannel2. Here is a TRY push that verifies it's fixed now. > https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8b8158e6f24 Going to reland whenever inbound opens up again!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b06b05456f75 https://hg.mozilla.org/integration/mozilla-inbound/rev/4819768c871f

https://hg.mozilla.org/mozilla-central/rev/b06b05456f75 https://hg.mozilla.org/mozilla-central/rev/4819768c871f

We're in the final build process for 41 at this point. I'm letting Ritu know!

carsten it looks like you landed this on m-c but marked it fixed for status-firefox41. That may mess up your queries... I'm setting it back to "affected" for 41.

Don't see a pressing reason to track this for 41, it is a wontfix.

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #39) > carsten it looks like you landed this on m-c but marked it fixed for > status-firefox41. That may mess up your queries... I'm setting it back to > "affected" for 41. Hey Liz, so this landed back in June (June 1st) and the merge tool at that time was setting automatically the fixed status since this was the target milestone at that date. Even hg says for https://hg.mozilla.org/mozilla-central/rev/b06b05456f75 milestone: 41.0a1 so what the tool did with fixed for 41 should be technically ok or ?

Oh it was June. Whoops! Sorry tomcat. Somehow I saw a "9" not a 6 and thought the work had just landed. My mistake!

A patch for bug 1205410 landed on mozilla-release with this bug's number in the commit message, so you might see some bugspam for that bug in here.

I think that 42 is also fixed.