1099296 - Attach LoadInfo to remaining callers of ioService and ProtocolHandlers

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In Bug 1087442 we are going to attach the loadInfo in each ProtocolHandler. We are pusing the attachment of the loadInfo from nsNetutil.h (through the ioService) into each ProtocolHandler.

We still have to attach the loadInfo to channels that are *not* created through nsNetutil.h.

In particular we have to update the remaining callsites of:
* ioService->NewChannel
* ioservice->NewChannelFromURI
* ioService->NewChannelFromURIWithProxyFlags
* handler->NewChannel

Once Bug 1087442 has landed, we should follow up on this.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: tmp.patch

I started doing this, it's still very WIP, but it's a first start.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_1_docshell.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_2_dom.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_3_embedding.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_4_extensions.patch

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_5_js.patch

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_6_netwerk.patch

This Bug depends on Bug 1087442, but since Jason knows what we are doing he volunteered to already take a look at this patch.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_7_security.patch

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_8_toolkit.patch

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, once you are back can you please also take a look and make a sanity check for all of the remaining callsites?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Now that I think about it, we can actually ask for reviews on those patches as well and just don't land them before Bug 1087442 has landed.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8525681 [details] [diff] [review]
cpp_4_extensions.patch

I'm not the right reviewer here.
I'm guessing there are no intended behavior changes and so someone who knows NewChannelFromURI2 can review this.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8525679 [details] [diff] [review]
cpp_2_dom.patch

Review of attachment 8525679 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xul/templates/nsXULTemplateQueryProcessorStorage.cpp
@@ +215,5 @@
>          nsCOMPtr<nsIIOService> ioservice =
>              do_GetService("@mozilla.org/network/io-service;1", &rv);
>          NS_ENSURE_SUCCESS(rv, rv);
>  
> +        rv = ioservice->NewChannelFromURI2(uri,

Change this to use NS_NewChannel instead. That way you don't have to pass all of the arguments that are deduced from the node.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8525681 [details] [diff] [review]
cpp_4_extensions.patch

Review of attachment 8525681 [details] [diff] [review]:
-----------------------------------------------------------------

Christoph: we really need to land documentation for NewChannel2 before asking for further reviews. It's critical that people that understand the code can review the patches meaningfully and know what the different arguments mean.

Comment 15

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8525685 [details] [diff] [review]
cpp_7_security.patch

Clearing review for now based on comment 14.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8525680 [details] [diff] [review]
cpp_3_embedding.patch

We are going to improve documentation within bug 1087442 and will reflag for review once Bug 1087442 has landed. Clearing all reviews in this bug for now.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Clearing needinfo for now, because those patches are going to change. We should call NS_newChannel wherever possible and not make use of the ioservice directly.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

I am going to fix those patches up - they should land soonish so we have loadInfo on all channels created in the C++ land.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_1_docshell.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_2_dom.patch

Calling NS_NewChannel rather than calling the ioservice directly. Other than that the patch is unchanged. Carrying over r+ from Jonas.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_3_embedding.patch

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_4_extensions.patch

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_5_js.patch

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_6_netwerk.patch

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_7_security.patch

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_8_toolkit.patch

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8535147 [details] [diff] [review]
cpp_6_netwerk.patch

Steve, you are already familiar with loadInfo and especially what the loadingPrincipal for a channel should be. This is the biggest patch in this bug, hence I am already flagging you for review.
Particularly please also check when it's worth keeping the ioservice and when we could potentially delete it from the callsite. Sometimes it seems useful to pass a chached ioservice to NS_NewChannel but sometimes we just create an ioservice right before we create the channel, in such cases I don't think it's useful to keep it on the callsite.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Hey Tanvi, the more eyes the better. Can you also take a look at all the arguments we pass to NewChannel in all of the patches? Thanks!

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_5_js.patch

We should keep the cached ioservice for js/ (I think).

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

To all reviewers:

In Bug 1038756 we started attaching a loadInfo to every channel whenever the channel getscreated through NS_NewChannel in nsNetUtil.h. Some Gecko callers however rather create a channel by calling the ioservice directly.

In this bug we try to convert those callers to rather create a channel by calling NS_NewChannel whenever possible and whenever this conversion makes sense. Please make sure that we pass the most appropriate arguments whenever we create a channel that end up getting stored in the LoadInfo. Please find a detailed description of all the arguments here: [1] or alternatively here [2].

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#202
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 8535172 [details] [diff] [review]
cpp_5_js.patch

Review of attachment 8535172 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/loader/mozJSComponentLoader.cpp
@@ +247,5 @@
>          BEGIN_ENSURE(ScriptChannel, IOService, URI);
> +        return NS_NewChannel(getter_AddRefs(mScriptChannel),
> +                             mURI,
> +                             //ckerschb: I doubt systemPrincipal to be correct here!!!
> +                             nsContentUtils::GetSystemPrincipal(),

If this is supposed to be the principal of the code doing the load (or the content being loaded), system principal is correct. In that case, please remove the comment, and r=me.

Comment 32

[Benjamin Smedberg]

Comment on attachment 8535144 [details] [diff] [review]
cpp_3_embedding.patch

I believe we use WBP for 'Save As' and I suspect we want better principal information than system, but I don't know that for certain. Somebody with expertise needs to understand whether this is ok. I'm going to tag sstamm since he just touched this with bug 704320.

Comment 33

[Benjamin Smedberg]

Comment on attachment 8535150 [details] [diff] [review]
cpp_8_toolkit.patch

ApplicationReputation should be reviewed by mmc.

nsFaviconService should probably be reviewed by mak: I think we've had issues with favicon security in the past, and so it might be that we really shouldn't be using the system principal here.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8535150 [details] [diff] [review]
cpp_8_toolkit.patch

mmc, mak, can you two review this one? Please also see Comment 33 and Comment 30.

Comment 35

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8535150 [details] [diff] [review]
cpp_8_toolkit.patch

Review of attachment 8535150 [details] [diff] [review]:
-----------------------------------------------------------------

LG for application reputation part.

Comment 36

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8535144 [details] [diff] [review]
cpp_3_embedding.patch

Review of attachment 8535144 [details] [diff] [review]:
-----------------------------------------------------------------

It appears to me like the only thing that uses CreateChannelFromURI here is nsWebBrowserPersist::StartUpload().  If I'm correct here (though I'm not 100% confident) then the system principal is probably right for the loadingPrincipal, since it is the system (nsWebBrowserPersist) uploading the content.

I'm confused about the content type; why have you selected nsIContentPolicy::TYPE_OTHER?

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #36)
> Comment on attachment 8535144 [details] [diff] [review]
> cpp_3_embedding.patch
> 
> Review of attachment 8535144 [details] [diff] [review]:
> -----------------------------------------------------------------
> I'm confused about the content type; why have you selected
> nsIContentPolicy::TYPE_OTHER?

Usually we try to select the most descriptive content type from
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicy.idl#55
but default to TYPE_OTHER if we can't find a suitable content type.
I am happy to update if you let me know what content type would be most suitable for that channel.

Comment 38

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8535148 [details] [diff] [review]
cpp_7_security.patch

Review of attachment 8535148 [details] [diff] [review]:
-----------------------------------------------------------------

AFAICT from the documentation, this looks correct.

Comment 39

[Marco Bonardo [:mak]]

Comment on attachment 8535150 [details] [diff] [review]
cpp_8_toolkit.patch

Review of attachment 8535150 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsFaviconService.cpp
@@ +333,5 @@
> +    new mozilla::LoadInfo(nullptr, // aLoadingNode
> +                          nsContentUtils::GetSystemPrincipal(),
> +                          nullptr, // aTriggeringPrincipal
> +                          nsILoadInfo::SEC_NORMAL,
> +                          nsIContentPolicy::TYPE_OTHER);

this channel only serves favicons, can we use TYPE_IMAGE?

If so, likely we could do the same in nsAnnoProtocolHandler.cpp where TYPE_OTHER was used.

Indeed, while here, there is a comment in nsAnnoProtocolHandler pointing to this bug, I guess that code can now be fixed properly with this change:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsAnnoProtocolHandler.cpp#346

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_8_toolkit.patch

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #39)
> this channel only serves favicons, can we use TYPE_IMAGE?

Yes, that is correct, thanks.

> If so, likely we could do the same in nsAnnoProtocolHandler.cpp where
> TYPE_OTHER was used.

Yes, I updated nsAnnoProtocolHandler to use TYPE_IMAGE instead of TYPE_OTHER. Thanks for pointing that out.

> Indeed, while here, there is a comment in nsAnnoProtocolHandler pointing to
> this bug, I guess that code can now be fixed properly with this change:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> nsAnnoProtocolHandler.cpp#346

Not quite, because we still have that forwarding call where the LoadInfo is a nullptr:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsAnnoProtocolHandler.cpp#288
which might potentially be called from an Addon, right?

I agree, the comment is misleading, I will update all those comments once Bug 1087720 landed.

Comment 41

[Marco Bonardo [:mak]]

Comment on attachment 8538591 [details] [diff] [review]
cpp_8_toolkit.patch

Review of attachment 8538591 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for the clarification.

Comment 42

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8535144 [details] [diff] [review]
cpp_3_embedding.patch

Review of attachment 8535144 [details] [diff] [review]:
-----------------------------------------------------------------

I think TYPE_OTHER makes as much sense as any for this use.  We could get more precise if we thread the content type through from the download channels via the callers to StartUpload and into the creation of this channel, but I think since this just creates upload channels (for storage) it doesn't matter a whole lot what the nsIContentPolicy type is.

Comment 43

[Tanvi Vyas[:tanvi]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #36)
> Comment on attachment 8535144 [details] [diff] [review]
> cpp_3_embedding.patch
> 
> Review of attachment 8535144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It appears to me like the only thing that uses CreateChannelFromURI here is
> nsWebBrowserPersist::StartUpload().  If I'm correct here (though I'm not
> 100% confident) then the system principal is probably right for the
> loadingPrincipal, since it is the system (nsWebBrowserPersist) uploading the
> content.
> 
What is nsWebBrowserPersist for?  What content is being uploaded and from where?

Comment 44

[Tanvi Vyas[:tanvi]]

Comment on attachment 8535144 [details] [diff] [review]
cpp_3_embedding.patch

(In reply to Tanvi Vyas [:tanvi] from comment #43)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #36)
> > Comment on attachment 8535144 [details] [diff] [review]
> > cpp_3_embedding.patch
> > 
> > Review of attachment 8535144 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > It appears to me like the only thing that uses CreateChannelFromURI here is
> > nsWebBrowserPersist::StartUpload().  If I'm correct here (though I'm not
> > 100% confident) then the system principal is probably right for the
> > loadingPrincipal, since it is the system (nsWebBrowserPersist) uploading the
> > content.
> > 
> What is nsWebBrowserPersist for?  What content is being uploaded and from
> where?

Talked to Sid and got some more info on this -   nsWebBrowserPersist is used to save html pages to disk.  The channel in question is uploading content to a file.  So TYPE_OTHER and systemPrincipal both seem fine for this.

Comment 45

[Tanvi Vyas[:tanvi]]

Comment on attachment 8535145 [details] [diff] [review]
cpp_4_extensions.patch

I'm not too familiar with this code, but from what I can tell system looks okay.  Plus it looks like ehsan, who is familiar with this code, looked at this and r+'ed it.

Comment 46

[Tanvi Vyas[:tanvi]]

Comment on attachment 8535148 [details] [diff] [review]
cpp_7_security.patch

This code is for OCSP request, so system makes sense here.

Comment 47

[Tanvi Vyas[:tanvi]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #39)
> Comment on attachment 8535150 [details] [diff] [review]
> cpp_8_toolkit.patch
> 
> ::: toolkit/components/places/nsFaviconService.cpp
> @@ +333,5 @@
> > +    new mozilla::LoadInfo(nullptr, // aLoadingNode
> > +                          nsContentUtils::GetSystemPrincipal(),
> > +                          nullptr, // aTriggeringPrincipal
> > +                          nsILoadInfo::SEC_NORMAL,
> > +                          nsIContentPolicy::TYPE_OTHER);
> 

Hi Marco,

Instead of using systemPrincipal here, can we get the top level document that is initiating this favicon request?  For example, when you visit https://bugzilla.mozilla.org in your browser, the favicon shows up in the tab above the address bar.  The tab is chrome UI, but the actor initiating the load was the document with principal https://bugzilla.mozilla.org.

What does ReplaceFaviconDataFromDataURL() do?  It extracts the image from a data: uri?

Thanks!

Comment 48

[Tanvi Vyas[:tanvi]]

Comment on attachment 8535147 [details] [diff] [review]
cpp_6_netwerk.patch

Waiting for Steve's review, and making some notes here for him to followup on.

diff --git a/netwerk/base/src/nsPACMan.cpp b/netwerk/base/src/nsPACMan.cpp
We believe this file is used when users set up a proxy for their browser.  Is the channel that is created the channel for that proxy?  If so, systemPrincipal and TYPE_OTHER looks fine.

diff --git a/netwerk/base/src/nsURIChecker.cpp b/netwerk/base/src/nsURIChecker.cpp
The channel in this file looks like it's associated with a HEAD request, in which case there could be a document associated with it.  System might not be accurate here.

diff --git a/netwerk/protocol/ftp/nsFtpConnectionThread.cpp b/netwerk/protocol/ftp/nsFtpConnectionThread.cpp
Also unclear why we should use system here.

diff --git a/netwerk/protocol/http/HttpChannelChild.cpp b/netwerk/protocol/http/HttpChannelChild.cpp
Are we using TYPE_OTHER in Redirect1Begin() because we don't know which content type is being redirected?

diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp
Same question about TYPE_OTHER here in nsHttpChannel::StartRedirectChannelToURI

In nsHttpChannel::ProcessFallback(), can we use the loadInfo on the original channel for the fallback channel?  That seems better than using system here.  Would mLoadInfo work in this case, or do we have to pass LoadInfo into ProcessFallback()?

Even for the cases where we are using a nullPrincipal (like CreateProcessRedirectionAfterFallback()), we might be able to get the loadInfo off the original channel.

diff --git a/netwerk/protocol/viewsource/nsViewSourceChannel.cpp b/netwerk/protocol/viewsource/nsViewSourceChannel.cpp
Can we use the loadInfo from the channel that loaded the document that's being view-sourced?  If we can get a reference to that channel (which Jonas' says we can), we can just pass through the loadInfo.

I didn't review the tests.  If I recall correctly, a couple of them are outdated and never run.

Comment 49

[Marco Bonardo [:mak]]

(In reply to Tanvi Vyas [:tanvi] from comment #47)
> Instead of using systemPrincipal here, can we get the top level document
> that is initiating this favicon request?  For example, when you visit
> https://bugzilla.mozilla.org in your browser, the favicon shows up in the
> tab above the address bar.  The tab is chrome UI, but the actor initiating
> the load was the document with principal https://bugzilla.mozilla.org.
> 
> What does ReplaceFaviconDataFromDataURL() do?  It extracts the image from a
> data: uri?

The API doesn't have knowledge of documents or such, it just receives a dataurl and replaces the data that is stored in the db, in most cases this data comes from a bookmarks.html or json file, so it couldn't even get the original principal.

Comment 50

[Tanvi Vyas[:tanvi]]

Comment on attachment 8535172 [details] [diff] [review]
cpp_5_js.patch

Got some context on what this file is doing from bholley ->

The Component Loader Info is just a helper to collate all the various pieces of information used by the component loader.

The component loader is the thing in charge of loading JSMs and JS-Implemented XPCOM components. Both can only be loaded from chrome/file URIs, and everything that is loaded runs with system principal (with a BackstagePass global).

Comment 51

[Tanvi Vyas[:tanvi]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #49)
> (In reply to Tanvi Vyas [:tanvi] from comment #47)
> > Instead of using systemPrincipal here, can we get the top level document
> > that is initiating this favicon request?  For example, when you visit
> > https://bugzilla.mozilla.org in your browser, the favicon shows up in the
> > tab above the address bar.  The tab is chrome UI, but the actor initiating
> > the load was the document with principal https://bugzilla.mozilla.org.
> > 
> > What does ReplaceFaviconDataFromDataURL() do?  It extracts the image from a
> > data: uri?
> 
> The API doesn't have knowledge of documents or such, it just receives a
> dataurl and replaces the data that is stored in the db, in most cases this
> data comes from a bookmarks.html or json file, so it couldn't even get the
> original principal.

Are there cases where this data comes fromt he webpage itself?  Where does the dataurl come from?  Looks like /toolkit/components/places/BookmarkHTMLUtils.jsm and /toolkit/components/places/BookmarkJSONUtils.jsm .  If it is only coming from bookmarks, then system is fine.  But if the favicon also comes from a webpage, we should try and include that as the loadingPrincipal (by passing that information through to the API).

Comment 52

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I think we should think of favicons as no different from <img>. I.e. the loadingPrincipal and the triggeringPrincipal should both be the principal of the webpage. The fact that the image is rendered outside of the square in which we render the webpage, shouldn't really affect anything from a security point of view.

Though I guess once we've downloaded and cached an favicon, and we're then loading *from that cache* in order to render for example bookmarks UI, then it seems like we could safely use a loadingPrincipal and triggeringPrincipal of 'system'.

Comment 53

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8535147 [details] [diff] [review]
cpp_6_netwerk.patch

Review of attachment 8535147 [details] [diff] [review]:
-----------------------------------------------------------------

OMG - so many NewChannel APIs!! ;) I'm sure this has been a nightmare to patch.

I didn't do the test channels yet either. I think there are enough comments here to do another revision. No big issues, just some conventions that have to be decided re default values.

::: netwerk/base/src/nsPACMan.cpp
@@ +440,5 @@
> +                      nsIContentPolicy::TYPE_OTHER,
> +                      nullptr, // aLoadGroup
> +                      nullptr, // aCallbacks
> +                      nsIRequest::LOAD_NORMAL,
> +                      ios);

Tanvi asked: "We believe this file is used when users set up a proxy for their browser.  Is the channel that is created the channel for that proxy?"

This is the channel created to get the PAC file which then sets up proxy config in the browser.

::: netwerk/base/src/nsURIChecker.cpp
@@ +139,5 @@
> +    nsresult rv = NS_NewChannel(getter_AddRefs(mChannel),
> +                                aURI,
> +                                nsContentUtils::GetSystemPrincipal(),
> +                                nsILoadInfo::SEC_NORMAL,
> +                                nsIContentPolicy::TYPE_OTHER);

If the URI is http: schemed, then the channel is an nsHttpChannel and it uses a HEAD request. Only OnStart and OnStop are used as callbacks - the channel has an NS_NOTREACHED for OnDataAvailable, so only the response header will be available for processing.

I don't see this used anywhere in Gecko. I see old refs to it (e.g. from 2008, http://archives.seul.org/or/cvs/Jun-2008/msg00526.html), where it looks like it's used to test if a file exists. So, it might be used in addons somewhere.

TYPE_OTHER seems fine. And I think system is fine if it's called from an addon, right?

::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
@@ +2385,5 @@
> +                            nsILoadInfo::SEC_NORMAL,
> +                            nsIContentPolicy::TYPE_OTHER);
> +
> +    return pph->NewProxiedChannel2(uri, pi, 0, nullptr,
> +                                   loadInfo, newChannel);

So, this is a little complicated :) but basically this is the callback for proxy resolution.

nsFtpChannel inherits from nsBaseChannel, so it uses nsBaseChannel's AsyncOpen.

When nsBaseChannel::AsyncOpen is called, it calls BeginPumpingData, which calls nsFtpChannel::OpenContentStream (OpenContentStream is a pure function that must be implemented by nsBaseChannel's derived classes).
OpenContentStream creates and inits nsFtpState, which then initiates the proxy resolution - nsFtpConnectionThread.cpp:1872.

nsFtpState::OnProxyAvailable is the callback for that, and CreateHTTPProxiedChannel is called if an HTTP proxy is supposed to be used.

(Yay for networking!)

If you look in OnProxyAvailable, CreateHTTPProxiedChannel is followed by a call to mChannel->Redirect - (nsFtpChannel::Redirect which is unimplemented and falls back to nsBaseChannel::Redirect). In that function, loadGroup, notification callbacks and loadflags are set.

I could be wrong, but I don't see anything here or in async redirect functions that copies over the LoadInfo from nsFtpChannel (or nsBaseChannel) to the new channel. I *think* we could (probably should?) do that in nsBaseChannel::Redirect along with loadGroup, notf. callbacks and loadFlags.


So, here that means some default value for principal until Redirect is called. I see you erring on the side of caution here in HttpChannelChild for redirects/replacement channels, at least until the real LoadInfo is passed on. Probably you should do the same here.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +917,5 @@
> +  rv = NS_NewChannel(getter_AddRefs(newChannel),
> +                     uri,
> +                     nullPrincipal,
> +                     nsILoadInfo::SEC_NORMAL,
> +                     nsIContentPolicy::TYPE_OTHER,

Tanvi asked: "Are we using TYPE_OTHER in Redirect1Begin() because we don't know which content type is being redirected?"

My assumption reading this is that TYPE_OTHER is used as a default initialized until SetupReplacementChannel copies over the real LoadInfo. Christoph told me that OTHER has lowest privileges, so maybe that's fine. If you're concerned that some security check could be done between here and SetupReplacementChannel, then maybe SetLoadInfo should be called here - but I think that the sec checks should be done after the channel is setup, right?

Assuming that's the case, please add a comment to make it clear.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1876,5 @@
> +    rv = NS_NewChannel(getter_AddRefs(newChannel),
> +                       upgradedURI,
> +                       nullPrincipal,
> +                       nsILoadInfo::SEC_NORMAL,
> +                       nsIContentPolicy::TYPE_OTHER,

see my comment for HttpChannelChild.

@@ +2634,5 @@
>      // Create a new channel to load the fallback entry.
>      nsRefPtr<nsIChannel> newChannel;
> +    rv = gHttpHandler->NewChannel2(mURI,
> +                                   loadInfo,
> +                                   getter_AddRefs(newChannel));

I echo Tanvi's comment. Why can't we use mLoadInfo here instead of allocating a new temp one?

@@ +4514,2 @@
>      nsCOMPtr<nsIChannel> newChannel;
> +    rv = NS_NewChannel(getter_AddRefs(newChannel),

You already have ioService - why not call NS_NewChannelInternal and use mLoadInfo, like you do later in the file?

::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
@@ +59,5 @@
> +                               nullptr, // aOriginCharset
> +                               nullptr, // aCharSet
> +                               nullptr, // aLoadingNode
> +                               nsContentUtils::GetSystemPrincipal(),
> +                               nullptr, // aTriggeringPrincipal

Looks like this is called from nsViewSourceHandler::NewChannel2, which sets the LoadInfo following this call anyway - aLoadInfo from the arg list. So, in this case, this is a default loadinfo.

Related: It looks like DocShell already checks for "view-source:" schemed urls before creating the channel and calls nsViewSrcHandler::NewSrcDocChannel instead. That calls nsViewSourceChannel::InitSrcDoc, and that function uses system principal and TYPE_OTHER. So, I'd say it's fine.

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2911,5 @@
> +                mLoadInfo->LoadingNode()->AsDOMNode() : nullptr,
> +              mLoadInfo->LoadingPrincipal(),
> +              mLoadInfo->TriggeringPrincipal(),
> +              mLoadInfo->GetSecurityFlags(),
> +              mLoadInfo->GetContentPolicyType(),

mLoadInfo or default values. This is a similar case to the HTTP redirects. SetLoadInfo is called after the replacement channel is created - and I think you should do that to make sure everything is copied over, right?

Here you copy from mLoadInfo; in those other cases you use default values. If there's a specific reason, document it. Otherwise, decide on a convention and stick to it in all these cases.

Comment 54

[Marco Bonardo [:mak]]

(In reply to Tanvi Vyas [:tanvi] from comment #51)
> Are there cases where this data comes fromt he webpage itself?  Where does
> the dataurl come from?  Looks like
> /toolkit/components/places/BookmarkHTMLUtils.jsm and
> /toolkit/components/places/BookmarkJSONUtils.jsm .  If it is only coming
> from bookmarks, then system is fine.  But if the favicon also comes from a
> webpage, we should try and include that as the loadingPrincipal (by passing
> that information through to the API).

this is where we set the favicon from the browser code
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#869
the API takes the favicon uri, then the favicon is fetched by the uri here
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp#545
We never get the favicon data directy from the browser.

Comment 55

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_6_netwerk.patch

Thanks Steve for the speedy review. Some of the code was slightly outdated because it was written quite some time ago. In the meanwhile a lot of API functions have changed and also a lot of channels, previously not having a Loadinfo, finally have one. Anyway, I incorporated your suggestions and answered all of your questions below. Time for round two and the tests.

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #53)
> Comment on attachment 8535147 [details] [diff] [review]
> cpp_6_netwerk.patch
> ::: netwerk/base/src/nsPACMan.cpp
> @@ +440,5 @@
> > +                      nsIContentPolicy::TYPE_OTHER,
> > +                      nullptr, // aLoadGroup
> > +                      nullptr, // aCallbacks
> > +                      nsIRequest::LOAD_NORMAL,
> > +                      ios);
> 
> Tanvi asked: "We believe this file is used when users set up a proxy for
> their browser.  Is the channel that is created the channel for that proxy?"
> 
> This is the channel created to get the PAC file which then sets up proxy
> config in the browser.

Then what we have sounds right to me.

> ::: netwerk/base/src/nsURIChecker.cpp
> @@ +139,5 @@
> > +    nsresult rv = NS_NewChannel(getter_AddRefs(mChannel),
> > +                                aURI,
> > +                                nsContentUtils::GetSystemPrincipal(),
> > +                                nsILoadInfo::SEC_NORMAL,
> > +                                nsIContentPolicy::TYPE_OTHER);
> 
> If the URI is http: schemed, then the channel is an nsHttpChannel and it
> uses a HEAD request. Only OnStart and OnStop are used as callbacks - the
> channel has an NS_NOTREACHED for OnDataAvailable, so only the response
> header will be available for processing.
> 
> I don't see this used anywhere in Gecko. I see old refs to it (e.g. from
> 2008, http://archives.seul.org/or/cvs/Jun-2008/msg00526.html), where it
> looks like it's used to test if a file exists. So, it might be used in
> addons somewhere.
> 
> TYPE_OTHER seems fine. And I think system is fine if it's called from an
> addon, right?

Well addon code is special. But if you think it's not used, we could use a nullPrincipal to be overly conservative. So in case it's still in use we would get complaints once we move seucirty checks into asyncOpen we would block the request. I am not sure what the right thing to do is in that case.

> ::: netwerk/protocol/ftp/nsFtpConnectionThread.cpp
> @@ +2385,5 @@
> > +                            nsILoadInfo::SEC_NORMAL,
> > +                            nsIContentPolicy::TYPE_OTHER);
> > +
> > +    return pph->NewProxiedChannel2(uri, pi, 0, nullptr,
> > +                                   loadInfo, newChannel);
> 

> I could be wrong, but I don't see anything here or in async redirect
> functions that copies over the LoadInfo from nsFtpChannel (or nsBaseChannel)
> to the new channel. I *think* we could (probably should?) do that in
> nsBaseChannel::Redirect along with loadGroup, notf. callbacks and loadFlags.

I realized a while ago that baseChannel->Redirect() does not forward the loadInfo but until now I didn't see any use-case why we would need it. Thanks for the clarificaiton. To sum it up, you are totally right, we should use restrictive dummy values until we hit the redirect and set the right loadInfo on the channel.

I added the propagtion of the loadInfo in the baseChannel redirect and added a comment in nsFtpConnectionThread.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +917,5 @@
> > +  rv = NS_NewChannel(getter_AddRefs(newChannel),
> > +                     uri,
> > +                     nullPrincipal,
> > +                     nsILoadInfo::SEC_NORMAL,
> > +                     nsIContentPolicy::TYPE_OTHER,

Agreed, we could use mLoadInfo. Updated that part here and elsewhere.

> ::: netwerk/protocol/viewsource/nsViewSourceChannel.cpp
> @@ +59,5 @@
> > +                               nullptr, // aOriginCharset
> > +                               nullptr, // aCharSet
> > +                               nullptr, // aLoadingNode
> > +                               nsContentUtils::GetSystemPrincipal(),
> > +                               nullptr, // aTriggeringPrincipal
> 
> Looks like this is called from nsViewSourceHandler::NewChannel2, which sets
> the LoadInfo following this call anyway - aLoadInfo from the arg list. So,
> in this case, this is a default loadinfo.

Since we use nullPrincipal for similar cases where we have to set a LoadInfo which then gets updated to the correct LoadInfo, I think we should do the same thing here. I added a comment in the code to make that clear.

> ::: netwerk/protocol/websocket/WebSocketChannel.cpp
> @@ +2911,5 @@
> > +                mLoadInfo->LoadingNode()->AsDOMNode() : nullptr,
> > +              mLoadInfo->LoadingPrincipal(),
> > +              mLoadInfo->TriggeringPrincipal(),
> > +              mLoadInfo->GetSecurityFlags(),
> > +              mLoadInfo->GetContentPolicyType(),
> 
> mLoadInfo or default values. This is a similar case to the HTTP redirects.
> SetLoadInfo is called after the replacement channel is created - and I think
> you should do that to make sure everything is copied over, right?
> 
> Here you copy from mLoadInfo; in those other cases you use default values.
> If there's a specific reason, document it. Otherwise, decide on a convention
> and stick to it in all these cases.

This one is tricky, I added a comment to make the intention clear. We want to create
a new channel with ProxyFlags, but there is no *.idl function that allows us to pass
the loadInfo to the ioservice. What we do in nsNetutil.h, we take all the arguments of
channels loadInfo, create the new channel and then call setLoadInfo on the channel.
We should do the same thing here, because we want the same instance on the newly created
channel.

Comment 56

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8540296 [details] [diff] [review]
cpp_6_netwerk.patch

Review of attachment 8540296 [details] [diff] [review]:
-----------------------------------------------------------------

One minor nit, one question about API choices in test files. Almost there. Leaving review open until test file question is answered.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1951,5 @@
>      nsresult rv;
>  
>      nsCOMPtr<nsIChannel> newChannel;
> +    rv = gHttpHandler->NewProxiedChannel2(mURI, pi, mProxyResolveFlags,
> +                                          mProxyURI, mLoadInfo, getter_AddRefs(newChannel));

nit: 80 chars max, please.

::: netwerk/test/TestUpload.cpp
@@ +141,2 @@
>          nsCOMPtr<nsIChannel> channel;
> +        rv = NS_NewChannel(getter_AddRefs(channel),

Here you have switched from IOService calls to nsNetUtil.h, but in TestRes.cpp you kept it, and in PropertiesTest.cpp you added it. Any reason why you don't just use NS_NewChannel in all four test files?

Comment 57

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #56)
> ::: netwerk/test/TestUpload.cpp
> @@ +141,2 @@
> >          nsCOMPtr<nsIChannel> channel;
> > +        rv = NS_NewChannel(getter_AddRefs(channel),
> 
> Here you have switched from IOService calls to nsNetUtil.h, but in
> TestRes.cpp you kept it, and in PropertiesTest.cpp you added it.Any reason
> why you don't just use NS_NewChannel in all four test files?

Ideally we should use NS_NewChannel() everywhere. Minor note, I didn't add the ioservice in PropertiesTest, it was already there - just looks awkward in the diff. One possible scenario is, that some of these tests are dead code and some not, so I might have gone back to the ioservice when there were compile issues. For some reason compiled code tests don't really come along with nsNetutil.h. That has to do with some exports - other compiled code tests have the same problem. Anyway, I will double check - ideally we should call NS_NewChannel() everywhere, but as I said, potentially leaving compiled code tests to call the ioservice directly.

Comment 58

[Tanvi Vyas[:tanvi]]

Comment on attachment 8538591 [details] [diff] [review]
cpp_8_toolkit.patch

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #54)
> (In reply to Tanvi Vyas [:tanvi] from comment #51)
> > Are there cases where this data comes fromt he webpage itself?  Where does
> > the dataurl come from?  Looks like
> > /toolkit/components/places/BookmarkHTMLUtils.jsm and
> > /toolkit/components/places/BookmarkJSONUtils.jsm .  If it is only coming
> > from bookmarks, then system is fine.  But if the favicon also comes from a
> > webpage, we should try and include that as the loadingPrincipal (by passing
> > that information through to the API).
> 
> this is where we set the favicon from the browser code
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#869
> the API takes the favicon uri, then the favicon is fetched by the uri here
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/
> AsyncFaviconHelpers.cpp#545
> We never get the favicon data directy from the browser.

Since the favicon uris are coming from the webpage, we really should try and get the loadingPrincipal of that webpage instead of using system.  When we move security checks to AsyncOpen(), favicons will bypass both mixed content and csp checks (for example).  We won't catch mixed content favicons and image-src csp directives will not be enforced for favicons.

Looks like we have the same issue here - http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/AsyncFaviconHelpers.cpp#545, which was added as part of bug 1038756.

Comment 59

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #55)
> > ::: netwerk/base/src/nsURIChecker.cpp
> > @@ +139,5 @@
> > > +    nsresult rv = NS_NewChannel(getter_AddRefs(mChannel),
> > > +                                aURI,
> > > +                                nsContentUtils::GetSystemPrincipal(),
> > > +                                nsILoadInfo::SEC_NORMAL,
> > > +                                nsIContentPolicy::TYPE_OTHER);
> > 
> > If the URI is http: schemed, then the channel is an nsHttpChannel and it
> > uses a HEAD request. Only OnStart and OnStop are used as callbacks - the
> > channel has an NS_NOTREACHED for OnDataAvailable, so only the response
> > header will be available for processing.
> > 
> > I don't see this used anywhere in Gecko. I see old refs to it (e.g. from
> > 2008, http://archives.seul.org/or/cvs/Jun-2008/msg00526.html), where it
> > looks like it's used to test if a file exists. So, it might be used in
> > addons somewhere.
> > 
> > TYPE_OTHER seems fine. And I think system is fine if it's called from an
> > addon, right?
> 
> Well addon code is special. But if you think it's not used, we could use a
> nullPrincipal to be overly conservative. So in case it's still in use we
> would get complaints once we move seucirty checks into asyncOpen we would
> block the request. I am not sure what the right thing to do is in that case.
> 
Let's use the nullPrincipal.

Comment 60

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_6_netwerk.patch

Steve - thanks for the review - here is an updated version of the patch and here is what I've addressed:

* Lines no longer than 80 chars.
* calling NS_NewChannel wherever possible
* nsViewSourceChannel still uses the pService, would be too much of a change otherwise - but that is fine to leave that as is.

FWIF, I figured why I called ioservice->NewChannel2() initially on some of those tests instead of NS_NewChannel. Because NS_NewChannel only takes an uri, whereas the ioservice provides an alternative API that also accepts strings. Anyway, creating a nsIURI now and then call NS_NewChannel. Thanks for pointing that out.

Comment 61

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Tanvi Vyas [:tanvi] from comment #59)
> Let's use the nullPrincipal.

I think we should keep the systemPrincipal for favIcons for now. I agree it's not great, but we need to dig a little deeper and fix security checks for favicons in general. Apparently that has been a problem for quite some time now, see Bug 437014, filed in 2008.

I hope you agree and we can clear the r- on the toolkit.patch!

Comment 62

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8545553 [details] [diff] [review]
cpp_6_netwerk.patch

Review of attachment 8545553 [details] [diff] [review]:
-----------------------------------------------------------------

r=me but change that system principal to null principal per your suggestion in comment #55 and Tanvi's comment #59.

::: netwerk/base/src/nsURIChecker.cpp
@@ +137,5 @@
>  nsURIChecker::Init(nsIURI *aURI)
>  {
> +    nsresult rv = NS_NewChannel(getter_AddRefs(mChannel),
> +                                aURI,
> +                                nsContentUtils::GetSystemPrincipal(),

nullPrincipal?

Comment 63

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_6_netwerk.patch

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #62)
> Comment on attachment 8545553 [details] [diff] [review]
> ::: netwerk/base/src/nsURIChecker.cpp
> > +                                nsContentUtils::GetSystemPrincipal(),
> nullPrincipal?

Indeed, we should used the nullPrincipal to be conservative where we are not sure if that code is still in use.
Thanks - carrying over r+ from sworkman.

Comment 64

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, as discussed I filed Bug 1119386 to investigate security checks for favicons. I think we can go with what we have here. Please clear your r- on toolkit and we can investigate in Bug 1119386. Using systemPrincipal is not any worse than what we have right now.

Comment 65

[Tanvi Vyas[:tanvi]]

Comment on attachment 8538591 [details] [diff] [review]
cpp_8_toolkit.patch

Tracking the long standing issues with favicons in bugs 437014 and 1119386 and using systemPrincipal for now.

Comment 66

[Christoph Kerschbaumer [:ckerschb]]

Marking this bug to be dependent on Bug 1087720. Assertions get triggered otherwise.

Comment 67

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_websocket.patch

Jason, as discussed on IRC the other day, we have to make sure we have a LoadInfo on every WebSocketChannel, even if created within JS, hence I added a initLoadInfo to nsIWebSocketChannel.idl so that JS can init the loadInfo for the websocket channel after creating one using doCreateInstance.

The JS callsites in this patch are tests only, so I think using systemPrincipal is totally fine. If you do think there are more appropriate arguments to use, please let me know.

Further, we also have to make sure that WebSocketChannels are properly initalized in e10s as well. I added those changes as well.

It would be great if you can have a look at this patch. Thanks!

Comment 68

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_a_browser_websocket.patch

Gavin, we also have to make sure we have a loadInfo attached for websocket channels. Since you are already familier with what we are trying to achive, I was wondering if you can review (or help find me reviewers) for this patch as well.

Comment 69

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_b_dom_push_websocket.patch

Nikhil, in Bug 1038756 we started attaching a loadInfo to every channel whenever the channel gets created through NS_NewChannel in nsNetUtil.h. Now we are expanding the loadInfo attachment to also include channels that get created within JS to ultimately end up having a loadInfo on every channel. WebSocketChannels however are slighlty different in a way, that nsIWebSocketChannel does not inherit from nsIChannel and hence is not created by calling ioservice::newChannel2(). Hence we added a initLoadInfo to the websocketchannel so we can pass loadinfo arguments from JS to the C++ where we then actually create the nsIChannel for websockets.

Please find a description of all the required arguments to create a channel here [1] or alternatively here [2].

In the attached patch we tried to pass the right arguments to initLoadInfo() to the best of our knowledge. It's quite complicated to provide the most accurate arguments (node, principal, contentType, etc.) because we are not experts within dom/push/ code. Probably we have to change some more function signatures to pass the correct principal/node around so we finally end up having the right arguments in the function that finally creates the channel.

Please take a look at the patch. Since this is security critical code please look closely and let us know if we need additional reviewers/help for certain sub components/files.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#202
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIIOService.idl#76

Comment 70

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_c_mobile_websocket.patch

Wesely, since you already reviewed loadInfo arguments in Bug 1087737, I was wondering if you can review this patch as well. Thanks!

Comment 71

[Jason Duell]

Comment on attachment 8547704 [details] [diff] [review]
cpp_9_websocket.patch

Review of attachment 8547704 [details] [diff] [review]:
-----------------------------------------------------------------

+r (no need for re-review) if you fix the goto nit and the comments cleanup.

::: netwerk/protocol/websocket/BaseWebSocketChannel.cpp
@@ +189,5 @@
> +                                   uint32_t aContentPolicyType)
> +{
> +  nsCOMPtr<nsINode> node = do_QueryInterface(aLoadingNode);
> +  mLoadInfo = new mozilla::LoadInfo(aLoadingPrincipal, aTriggeringPrincipal,
> +                                    node, aSecurityFlags, aContentPolicyType);

very mild pref to "use namespace mozilla" so we don't need to refer to mozilla::LoadInfo.  Seems likely that eventually all code will want to use the mozilla namespace in the long run...

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +2925,5 @@
>        *aSecurityInfo = nullptr;
>    }
>    return NS_OK;
>  }
> + 

If you're gonna reduce whitespace (sure, why not), then keep blank lines empty.

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ -79,5 @@
>    if (appId != NECKO_UNKNOWN_APP_ID &&
>        appId != NECKO_NO_APP_ID) {
>      gIOService->IsAppOffline(appId, &appOffline);
>      if (appOffline) {
> -      goto fail;

So you've converted from the "goto" error handling here, to one where you repeat all the failure code (mChannel = null) at each failure site.  I think that's actually worse.  I'd prefer to either revert to goto again, or (better) you could create some AutoFail(nsresult &rv) class whose destructor automatically sets mChannel = null and calls SendOnStop(rv), unless its .Succeeded() method was called (which it would be at the end of the function if everything goes ok).

::: netwerk/protocol/websocket/nsIWebSocketChannel.idl
@@ +81,5 @@
> +     *        the stylesheet.
> +     *        If possible, pass in the element which is performing the load. But
> +     *        if the load is coming from a JS API (such as XMLHttpRequest) or if
> +     *        the load might be coalesced across multiple elements (such as
> +     *        for <img>) then pass in the Document node instead.

Websockets can't be created by XHR, so this comment doesn't really fit in websockets.idl.  (talk of a "load" in general doesn't fit, since websockets aren't just a single one way load).  I suspect just leaving the 1st paragraph here (ending in 'the stylesheet') is enough here? Maybe leave in the last paragraph (about null loads from addons etc) in case some code ever does that with websockets.

@@ +99,5 @@
> +     *        from WebWorkers since they don't have any nodes to be passed as
> +     *        aLoadingNode.
> +     *        Please note, aLoadingPrincipal is *not* the principal of the
> +     *        resource being loaded. But rather the principal of the context
> +     *        where the resource will be used.

ok, all the loadingPrincipal language here seems fine, if a little wordy.  That's fine.

@@ +110,5 @@
> +     *        In some cases the loadingPrincipal and the triggeringPrincipal are
> +     *        different however, e.g. a stylesheet may import a subresource. In
> +     *        that case the principal of the stylesheet which contains the
> +     *        import command is the triggeringPrincipal, and the principal of
> +     *        the document whose rendering is affected is the loadingPrincipal.

Do you have any idea if the loadingPrincipal/TriggeringPrincipal can be different for a websocket?  I suspect not.  Do we want to clean up the API to not pass aTriggeringPrincipal if that's the case? (if you want to keep the params to initLoadInfo to be the same as for other channels, I guess that OK)

Comment 72

[Tanvi Vyas[:tanvi]]

(In reply to Jason Duell [:jduell] (needinfo? me for lower latency) from comment #71)
> Comment on attachment 8547704 [details] [diff] [review]
> cpp_9_websocket.patch
>
> Do you have any idea if the loadingPrincipal/TriggeringPrincipal can be
> different for a websocket?  I suspect not.  Do we want to clean up the API
> to not pass aTriggeringPrincipal if that's the case? (if you want to keep
> the params to initLoadInfo to be the same as for other channels, I guess
> that OK)

See https://bugzilla.mozilla.org/show_bug.cgi?id=1119989.  Per Jonas, they can't be different.  But perhaps we should keep initLoadInfo the way it is so that it matches with our other APIs.

Comment 73

[Tanvi Vyas[:tanvi]]

(In reply to Tanvi Vyas [:tanvi] from comment #72)
> See https://bugzilla.mozilla.org/show_bug.cgi?id=1119989.  Per Jonas, they
> can't be different.  But perhaps we should keep initLoadInfo the way it is
> so that it matches with our other APIs.

Whatever we decide, we should also update the triggeringPrincipal comment to indicate its the same as loadingPrincipal for websockets.

Comment 74

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_websockets.patch

Addressed all the concerns from jduell. Before landing we should yet again go over our argument description in nsIWebSocketChannel.idl.

Just chatted with Tanvi on IRC, she has some more suggestions.

Code is fine however, carrying over r+ from jduell.

Comment 75

[Tanvi Vyas[:tanvi]]

Comment on attachment 8564265 [details] [diff] [review]
cpp_9_websockets.patch

>diff --git a/netwerk/protocol/websocket/nsIWebSocketChannel.idl b/netwerk/protocol/websocket/nsIWebSocketChannel.idl

>+     * @param aLoadingPrincipal
>+     *        The loadingPrincipal of the channel.
>+     *        The principal of the document where the result of this request will
>+     *        be used.
>+     *        This defaults to the principal of aLoadingNode, so when aLoadingNode
>+     *        is passed then aLoadingPrincipal can be left as null. However, for
>+     *        loads where aLoadingNode is null this argument must be passed.
>+     *        For example for loads from a WebWorker, pass the principal
>+     *        of that worker. For loads from an addon or from internal browser
>+     *        features, pass the system principal.
>+     *        This principal should almost always be the system principal if
>+     *        aLoadingNode is null. One exception to this is for loads
Switch this sentence around to:
If aLoadingNode is null and the URI being loaded isn't coming from a webpage, the principal should almost always be the systemPrincipal.

>+     *        from WebWorkers since they don't have any nodes to be passed as
>+     *        aLoadingNode.
>+     *        Please note, aLoadingPrincipal is *not* the principal of the
>+     *        resource being loaded. But rather the principal of the context
>+     *        where the resource will be used.


>+     *        Keep in mind that URIs coming from a webpage should *never* use the
>+     *        systemPrincipal as the loadingPrincipal.
Put this back at the bottom of the entire comment like you had before.

Comment 76

[Christoph Kerschbaumer [:ckerschb]]

Attachment: cpp_9_websockets.patch

Ok, revised the docs. Carrying over r+.

Comment 77

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=78282059eb4a

Comment 78

[Daniel Holbert [:dholbert]]

Comment on attachment 8546018 [details] [diff] [review]
cpp_6_netwerk.patch

>+++ b/netwerk/test/PropertiesTest.cpp
> #define TEST_URL "resource:/res/test.properties"
> static NS_DEFINE_CID(kPersistentPropertiesCID, NS_IPERSISTENTPROPERTIES_CID);
> 
> static NS_DEFINE_CID(kIOServiceCID, NS_IOSERVICE_CID);
> 
> /***************************************************************************/
> 
>@@ -34,21 +36,33 @@ main(int argc, char* argv[])
> 
>   nsresult ret;
> 
>   nsCOMPtr<nsIServiceManager> servMan;
>   NS_InitXPCOM2(getter_AddRefs(servMan), nullptr, nullptr);
> 
>   nsIInputStream* in = nullptr;
> 
>-  nsCOMPtr<nsIIOService> service(do_GetService(kIOServiceCID, &ret));
>+  nsCOMPtr<nsIScriptSecurityManager> secman =
>+    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &ret);

This commit caused local build bustage for me (using clang 3.7 & enable-warnings-as-errors) because it removed the last usage of the local variable "kIOServiceCID" in PropertiesTest.cpp. The error is:
{
 netwerk/test/PropertiesTest.cpp:27:22: error: unused variable 'kIOServiceCID' [-Werror,-Wunused-const-variable]
}

Any objections to me landing a followup to drop "static NS_DEFINE_CID(kIOServiceCID, NS_IOSERVICE_CID);" from this PropertiesTest.cpp file?

Comment 79

[Daniel Holbert [:dholbert]]

Landed that removal w/ rs=ckerschb granted over IRC:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/060d29a8e8bc

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c36a47a1d27
https://hg.mozilla.org/mozilla-central/rev/235767967861
https://hg.mozilla.org/mozilla-central/rev/490fbc9b7f9b
https://hg.mozilla.org/mozilla-central/rev/b92d3b73d5f6
https://hg.mozilla.org/mozilla-central/rev/918a7b7f8275
https://hg.mozilla.org/mozilla-central/rev/b75f375f7cb5
https://hg.mozilla.org/mozilla-central/rev/a64004249507
https://hg.mozilla.org/mozilla-central/rev/f01170060577
https://hg.mozilla.org/mozilla-central/rev/e369969c0bdd
https://hg.mozilla.org/mozilla-central/rev/b24695fc8d79
https://hg.mozilla.org/mozilla-central/rev/6676dab23ede
https://hg.mozilla.org/mozilla-central/rev/78282059eb4a
https://hg.mozilla.org/mozilla-central/rev/060d29a8e8bc