Closed Bug 1087442 Opened 6 years ago Closed 5 years ago

Attach LoadInfo in newChannel2 inside each individual ProtocolHandler

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: ckerschb, Assigned: tanvi)

References

(Blocks 2 open bugs)

Details

Attachments

(14 files, 49 obsolete files)

1.91 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.30 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.50 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.06 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.06 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
22.88 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.93 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.23 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.85 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.47 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.56 KB, patch
sicking
: review+
Details | Diff | Splinter Review
3.42 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
40.90 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
14.63 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Currently we attach the loadInfo in nsNetutil.h whenever a channel is created through NS_NewChannel() [1]. Since we are extending the API for nsIProtocolHandler, nsIAboutModule and nsIIOService we should attach the LoadInfo to the channel in the different protoclHandlers instead of nsNetutil.h

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#237
Assignee: nobody → tanvi
Steps for this bug:

Update netwerk/base/src/nsIOService.cpp to create the loadInfo object and call the protocol handlers newchannel2() with the loadInfo.  If no newchannel2 exists, call new channel.

Update the protocol handlers NewChannel2() to set the loadInfo that's passed into it.

Update NS_NewChannel and friends in nsNetUtil.h to call ioservice->NewChannelFromURI2, passing in the load info.  Assert afterwards that a load info is set.  Stop setting the loadInfo in nsNetUtil.h.
(In reply to Tanvi Vyas [:tanvi] from comment #2)
> Note we will also need to update C++ and js callers of NewChannelFromURI()
> to pass in the loading info.
> 
> js -
> http://mxr.mozilla.org/mozilla-central/
> search?string=newChannelFromURI&case=on&find=&findi=&filter=^%5B^\0%5D*%24&hi
> tlimit=&tree=mozilla-central

All the *JS* callers of the ioservice including newChannel(), newChannelFromURI(), etc. will be updated in bug 1087720.

> cpp -
> http://mxr.mozilla.org/mozilla-central/
> search?string=NewChannelFromURI&case=on&find=&findi=&filter=^%5B^\0%5D*%24&hi
> tlimit=&tree=mozilla-central

What we need to do here is to update callers of ioService in *C++*, in particular:
* ioservice->NewChannel, and
* ioservice->NewChannelFromURI
Summary: Attach LoadInfo to the channel in NewChannel2 instead of nsNetutil.h → Attach LoadInfo in newChannel2 inside each individual ProtocolHandler
Blocks: 1099296
Attaching work in progress patches.  I am going to be out for a week, so Christoph is going to pick up where I have left off.  Here is the current status.

Done:
Update netwerk/base/src/nsIOService.cpp to create the load info and call the protocol handlers newchannel2.  If no newchannel2 exists, call newchannel

Update NS_NewChannel and friends in nsNetUtil.h to call ioservice->NewChannelFromURI2, passing in the load info.

Update the protocol handlers NewChannel2() and NewProxiedChannel2() to set the load info that's passed into it.

Add NS_NewInputStreamChannelInternal that takes a loadinfo (so that we don't have to break up loadinfo that is passed into NewChannel2 and can just pass the whole object).

In the code but commented out right now:
Assert afterwards that a load info is set. Stop setting the loadInfo in nsNetUtil.h.  We need to uncomment and check for browser crashes.  It's quite possible we've missed a callsite or protocol handler.

Not done:
Action item from our call with Jonas this week ->
In ioservice, document that you only have to pass a node or a principal.  If pass both, the principals have to match.
So if both are passed, add an assert for that.  i.e. In ioservice:NewChannelFromURIWithProxyFlags2, if(loadingNode && loadingPrincipal) MOZ_ASSERT(loadingPrincipal is equal to loadingNode->NodePrincipal()).


Also, these two should be calling NewProxiedChannel2 with a  loadinfo.  Finding a the right load info of course will be tricky.
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1969
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/ftp/nsFtpConnectionThread.cpp#2380
Attached patch protocolhandler_setloadinfo_dom (obsolete) — Splinter Review
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Attachment #8523275 - Attachment is obsolete: true
Attachment #8523277 - Attachment is obsolete: true
Attachment #8523278 - Attachment is obsolete: true
Attachment #8523279 - Attachment is obsolete: true
Attachment #8523280 - Attachment is obsolete: true
Attachment #8523281 - Attachment is obsolete: true
Attachment #8523282 - Attachment is obsolete: true
Attachment #8523284 - Attachment is obsolete: true
Attachment #8523285 - Attachment is obsolete: true
Attachment #8523286 - Attachment is obsolete: true
Attachment #8523287 - Attachment is obsolete: true
Attachment #8523289 - Attachment is obsolete: true
Attachment #8523652 - Flags: review?(jonas)
Attachment #8523652 - Flags: review?(jduell.mcbugs)
Attached patch prot_2_ioservice.patch (obsolete) — Splinter Review
Attachment #8523653 - Flags: review?(jonas)
Attachment #8523653 - Flags: review?(jduell.mcbugs)
Attached patch prot_3_chrome.patch (obsolete) — Splinter Review
Attachment #8523654 - Flags: review?(benjamin)
Attached patch prot_4_dom.patch (obsolete) — Splinter Review
Attachment #8523655 - Flags: review?(jonas)
Attached patch prot_5_extensions.patch (obsolete) — Splinter Review
Additional question: Is nsGnomeVFSProtocolHandler still used?
This callsite never got updated:
http://mxr.mozilla.org/mozilla-central/source/extensions/gnomevfs/nsGnomeVFSProtocolHandler.cpp#923
but apparently the compiler is not complaining? Can we remove that file if it's not used anymore?
Attachment #8523656 - Flags: review?(ehsan.akhgari)
Attached patch prot_6_image.patch (obsolete) — Splinter Review
Attachment #8523657 - Flags: review?(seth)
Attached patch prot_7_modules.patch (obsolete) — Splinter Review
Attachment #8523658 - Flags: review?(mwu)
Attached patch prot_8_netwerk.patch (obsolete) — Splinter Review
Attachment #8523659 - Flags: review?(jduell.mcbugs)
Attached patch prot_9_toolkit.patch (obsolete) — Splinter Review
Attachment #8523660 - Flags: review?(gpascutto)
Attached patch prot_10_uriloader.patch (obsolete) — Splinter Review
Boris is too busy at the moment. Would you mind reviewing that?
Attachment #8523661 - Flags: review?(jst)
Attached patch prot_11_widget.patch (obsolete) — Splinter Review
Attachment #8523662 - Flags: review?(blassey.bugs)
Attached patch prot_12_browser.patch (obsolete) — Splinter Review
Attachment #8523663 - Flags: review?(benjamin)
Attached patch prot_13_docshell.patch (obsolete) — Splinter Review
Attachment #8523664 - Flags: review?(jst)
Comment on attachment 8523660 [details] [diff] [review]
prot_9_toolkit.patch

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

::: toolkit/components/places/nsAnnoProtocolHandler.cpp
@@ +345,5 @@
>    nsCOMPtr<nsIChannel> channel;
> +  // Bug 1087720 (and Bug 1099296):
> +  // Once all callsites have been updated to call NewChannel2() instead of NewChannel()
> +  // we should have a non-null loadInfo consistently. Until then we have to brach on the
> +  // loadInfo and provide default arguments to create a NewInputStreamChannel.

Can you explain what exactly this if gains you? Either all callers are setting this loadinfo, in which case you can use the new code, or they don't, in which case you might as well use the old code and add a warning or trace that someone still needs to be updated?
Comment on attachment 8523652 [details] [diff] [review]
prot_1_netutil.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +235,5 @@
> +    // a loadInfo within it's implementation of ::newChannel2()
> +    nsCOMPtr<nsILoadInfo> loadInfo;
> +    rv = channel->GetLoadInfo(getter_AddRefs(loadInfo));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    MOZ_ASSERT(loadInfo);

This assertion seems better to do in the ioservice. Since there are many more places that call the ioservice than this one. And especially since you in the comment above are referring to implementation details of the IO service.

@@ +717,2 @@
>  {
> +  NS_ASSERTION(aLoadInfo, "can not create channel without a loadinfo");

MOZ_ASSERT
Attachment #8523652 - Flags: review?(jonas) → review+
Comment on attachment 8523653 [details] [diff] [review]
prot_2_ioservice.patch

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

r=me with those things fixed.

::: netwerk/base/src/nsIOService.cpp
@@ +651,5 @@
> +    //
> +    // BUG 1087720: Once that bug lands, we should have a loadInfo for all callers of
> +    // NewChannelFromURIWithProxyFlags2() and should remove the *if* before creating a
> +    // a new loadinfo.
> +    if (aLoadingPrincipal) {

This doesn't seem right. Didn't we agree that there are callsites that will only pass a loadingNode?

Or do we *everywhere* get the principal from node before getting in here?

Wouldn't it be easier to instead make the loadinfo ctor take care of extracing the loadingprincipal from the node as appropriate. And make all other newChannel* functions just forward the arguments unchanged?

I.e. change the above to if (aLoadingPrincipal || aLoadingNode)

@@ +659,5 @@
> +      {
> +        // if there is a requestingNode, then that node's principal
> +        // must match the loadingPrincipal.
> +        if (requestingNode) {
> +          MOZ_ASSERT(requestingNode->NodePrincipal() == aLoadingPrincipal);

MOZ_ASSERT(!requestingNode ||...

Or really

MOZ_ASSERT(!requestingNode || !aLoadingNode ||...

Or, even better, move the assertion into the LoadInfo ctor.

@@ +670,5 @@
> +                                       requestingNode,
> +                                       aSecurityFlags,
> +                                       aContentPolicyType);
> +      if (!loadInfo) {
> +        return NS_ERROR_UNEXPECTED;

No need to do this. new will crash if we run out of memory.

@@ +698,2 @@
>          return rv;
> +    }

NS_ENSURE_SUCCESS(rv, rv);
Attachment #8523653 - Flags: review?(jonas) → review+
Comment on attachment 8523655 [details] [diff] [review]
prot_4_dom.patch

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

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ +484,5 @@
>  }
>  
>  NS_IMETHODIMP
>  nsHostObjectProtocolHandler::NewChannel2(nsIURI* uri,
> +                                         nsILoadInfo *aLoadInfo,

Be consistent with surrounding code. Put the * next to the type.
Attachment #8523655 - Flags: review?(jonas) → review+
Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

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

I'd prefer if Olli looks at this. Though happy to have jst look as well if he wants to.
Attachment #8523664 - Flags: review?(bugs)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #30)
> Comment on attachment 8523660 [details] [diff] [review]
> prot_9_toolkit.patch
> 
> Review of attachment 8523660 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/places/nsAnnoProtocolHandler.cpp
> @@ +345,5 @@
> >    nsCOMPtr<nsIChannel> channel;
> > +  // Bug 1087720 (and Bug 1099296):
> > +  // Once all callsites have been updated to call NewChannel2() instead of NewChannel()
> > +  // we should have a non-null loadInfo consistently. Until then we have to brach on the
> > +  // loadInfo and provide default arguments to create a NewInputStreamChannel.
> 
> Can you explain what exactly this if gains you? Either all callers are
> setting this loadinfo, in which case you can use the new code, or they
> don't, in which case you might as well use the old code and add a warning or
> trace that someone still needs to be updated?

Ideally all channels are created by calling NewChannel2() handing the accurate loadInfo as an argument. At the moment we are in a kind of a limbo-state, where some callers call newChannel2() and some still call NewChannel() in which case we want to use default arguments. We are going to update JS callsites in Bug 1087720. Once that has landed, we will remove the if-clause here and always call NewStreamChannelWithLoadInfo() instead of using default arguments.

To answer your question, if there is a loadInfo handed to NewChannel2() we would like to use that (since it is the most accurate loadInfo). If not, we are using default arguments since we are trying to avoid having channels without a loadInfo. Ideally, we are getting out of this limbo-state super soonish. We have code to update all callsites for Bug 1087720 (roughtly 350 callsites), but we rather land this bug first, so we can convert JS callsites iteratively, which is going to make debugging a whole lot easier than converting all the callsites first and then start using the attached loadInfo once we flip the switch in this bug.
(In reply to Jonas Sicking (:sicking) from comment #31)
> Comment on attachment 8523652 [details] [diff] [review]
> prot_1_netutil.patch
> 
> Review of attachment 8523652 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/public/nsNetUtil.h
> @@ +235,5 @@
> > +    // a loadInfo within it's implementation of ::newChannel2()
> > +    nsCOMPtr<nsILoadInfo> loadInfo;
> > +    rv = channel->GetLoadInfo(getter_AddRefs(loadInfo));
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    MOZ_ASSERT(loadInfo);
> 
> This assertion seems better to do in the ioservice. Since there are many
> more places that call the ioservice than this one. And especially since you
> in the comment above are referring to implementation details of the IO
> service.

Yeah, agreed.

> @@ +717,2 @@
> >  {
> > +  NS_ASSERTION(aLoadInfo, "can not create channel without a loadinfo");
> 
> MOZ_ASSERT

Will also use MOZ_ASSERT of course like in the other cases. Dunno why I haven't used that in the first place.
(In reply to Jonas Sicking (:sicking) from comment #32)
> Or, even better, move the assertion into the LoadInfo ctor.

That makes the most sense to me. Then we don't have to deal with it here. Good idea!
Attached patch prot_0_loadinfo.patch (obsolete) — Splinter Review
Indeed it's better to query the principal from the node if one is passed in and then adding an assertion that the principal and the node's principal must match.
Jonas, I think this is what you wanted here, right?
Attachment #8523989 - Flags: review?(jonas)
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Moving the assetion from nsnetutil.h into the ioservice. Carrying over r+ from Jonas.
Attachment #8523652 - Attachment is obsolete: true
Attachment #8523652 - Flags: review?(jduell.mcbugs)
Attachment #8523990 - Flags: review+
Attachment #8523990 - Flags: review?(jduell.mcbugs)
Attached patch prot_2_ioservice.patch (obsolete) — Splinter Review
Moving assertions around:
1) assert that we have a loadInfo attached to the newly created channel in ioservice rather than in netutil.
2) Moving assertion that node's principal and loadingPrincipal match (in case there is a node) to prot_0_loadinfo.patch.
Attachment #8523653 - Attachment is obsolete: true
Attachment #8523653 - Flags: review?(jduell.mcbugs)
Attachment #8523993 - Flags: review+
Comment on attachment 8523993 [details] [diff] [review]
prot_2_ioservice.patch

Carried over r+ from sicking, but still need jduell to look at this too.
Attachment #8523993 - Flags: review?(jduell.mcbugs)
Attached patch prot_4_dom.patch (obsolete) — Splinter Review
Just fixed a nit. Carrying over r+ from Jonas.
Attachment #8523655 - Attachment is obsolete: true
Attachment #8523998 - Flags: review+
Attachment #8523662 - Flags: review?(blassey.bugs) → review+
Attachment #8523660 - Flags: review?(gpascutto) → review+
Comment on attachment 8523989 [details] [diff] [review]
prot_0_loadinfo.patch

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

Shouldn't you also remove equivalent code elsewhere? r=me on this change either way.

::: docshell/base/LoadInfo.cpp
@@ +36,5 @@
> +  {
> +    // if consumers pass both, aLoadingContext and aLoadingPrincipal
> +    // then the loadingPrincipal must be the same as the node's principal
> +    if (aLoadingContext && aLoadingPrincipal) {
> +      MOZ_ASSERT(aLoadingContext->NodePrincipal() == aLoadingPrincipal);

Nit: I'd write this as

MOZ_ASSERT(!aLoadingContext || !aLoadingPrincipal ||
           aLoadingContext->NodePrincipal() == aLoadingPrincipal);

That way you can avoid the extra curlies and #ifdef.
Attachment #8523989 - Flags: review?(jonas) → review+
Comment on attachment 8523656 [details] [diff] [review]
prot_5_extensions.patch

Karl, since Ehsan is on PTO and you already reviewed a similar change for Bug 1063197, do you mind reviewing that patch as well?
Attachment #8523656 - Flags: review?(ehsan.akhgari) → review?(karlt)
Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

It is still not clear to me how NewChannel2 works and what kind of params should
be passed to it, but do we need to handle
URI_SAFE_FOR_UNTRUSTED_CONTENT vs non-URI_SAFE_FOR_UNTRUSTED_CONTENT differently here?
(In reply to Olli Pettay [:smaug] from comment #45)
> Comment on attachment 8523664 [details] [diff] [review]
> prot_13_docshell.patch
> 
> It is still not clear to me how NewChannel2 works and what kind of params
> should
> be passed to it, but do we need to handle
> URI_SAFE_FOR_UNTRUSTED_CONTENT vs non-URI_SAFE_FOR_UNTRUSTED_CONTENT
> differently here?

Let me try to clarify. The LoadInfo (see [1] for description of all the necessary args) is supposed to be created and attached to a new instantiated channel at construction time. As a first step we attached the loadInfo whenever calling NS_NewChannel in netUtil.h (See Bug 1038756).

Now that we have that, we not longer want to attach the loadInfo in nsNetutil.h but rather in every channel at construction time, which is whenever a protoclHandler calls NewChannel2().

As of now there are not that many consumers of the LoadInfo. If there is a channel owner, than we still use the channel owner as of now, but we try to get away from that eventually.

Callers can still call NewChannel(), but then it's forwared to NewChannel2 with a null loadInfo. Soon we should call NewChannel2 throughout Gecko and we should have a loadInfo attached to all channels (see also Bug 1087720).

To sum it up, all what we are doing in this patch is, we push the setting of the loadInfo into each individual protocolhandler.

 


[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadInfo.idl
Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

NewChannel2 really needs some documentation.
And it is mystery to me why there isn't a version of NewChannel2 which just takes
nsILoadInfo.
Attachment #8523664 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #47)
> Comment on attachment 8523664 [details] [diff] [review]
> prot_13_docshell.patch
> 
> NewChannel2 really needs some documentation.
Agreed, working on that.

> And it is mystery to me why there isn't a version of NewChannel2 which just
> takes nsILoadInfo.

I assume you are referring to the ioservice, because each protocolHandler's newChannel2() takes a loadInfo.
In that case, I would like Jonas to answer that question.
Flags: needinfo?(jonas)
Comment on attachment 8523656 [details] [diff] [review]
prot_5_extensions.patch

>+                                          EmptyCString(), // aContentType

>     // start out assuming an unknown content-type.  we'll set the content-type
>     // to something better once we open the URI.

>                                   NS_LITERAL_CSTRING(UNKNOWN_CONTENT_TYPE));

Any reason for the change from UNKNOWN_CONTENT_TYPE to EmptyCString()?
Attached patch prot_5_extensions.patch (obsolete) — Splinter Review
(In reply to Karl Tomlinson (:karlt) from comment #49)
> Any reason for the change from UNKNOWN_CONTENT_TYPE to EmptyCString()?

Of course no reason to do that. Copy-paste error. Thanks - here is a new patch!
Attachment #8523656 - Attachment is obsolete: true
Attachment #8523656 - Flags: review?(karlt)
Attachment #8525603 - Flags: review?(karlt)
Attachment #8525603 - Flags: review?(karlt) → review+
Attachment #8523658 - Flags: review?(mwu) → review+
Comment on attachment 8523657 [details] [diff] [review]
prot_6_image.patch

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

Looks good to me!
Attachment #8523657 - Flags: review?(seth) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #48)
> > And it is mystery to me why there isn't a version of NewChannel2 which just
> > takes nsILoadInfo.
> 
> I assume you are referring to the ioservice, because each protocolHandler's
> newChannel2() takes a loadInfo.
> In that case, I would like Jonas to answer that question.

My concern is that it's almost never the case that if you want to instantiate a channel, that it's beneficial to first instantiate an nsILoadInfo and then pass that to a channel factory function. It's essentially always simpler and more clear if you pass the relevant arguments directly to the channel factory function.

The main exception to this is for code that is essentially internal to network code. Where one channel is redirecting or deferring to another channel. It's somewhat silly here that we need to go through the scriptable nsIIOService interface.

A better solution would have been to have an internal channel factory function which ioservice is just a wrapper for.

All that said, I thought we had added a channel factory function to NetUtils.h which takes an nsILoadInfo, specifically to cover channels instantiating other channels. Why aren't we calling that here?
Flags: needinfo?(jonas)
Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

It seems that jst might be busy due to the recent structure changes, can you take a look at that as well?
Attachment #8523664 - Flags: review?(jst) → review?(bugs)
Attachment #8523661 - Flags: review?(jst) → review?(bugs)
Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

Not sure me reviewing the same patch twice is needed ;)
Attachment #8523664 - Flags: review?(bugs)
Attachment #8523661 - Flags: review?(bugs) → review+
Attached patch prot_0_loadinfo.patch (obsolete) — Splinter Review
Updated assertion based on Comment 43 - carrying over r+ from Jonas.
Attachment #8523989 - Attachment is obsolete: true
Attachment #8527914 - Flags: review+
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
This patch is till calling |setLoadinfo()| in nsNetutil.h. That way we still have the same loadinfo instance attached to the channel.

Please note, that ioservice has an assertion that we actually set the loadInfo on the channel even before we overwrite it nsNetutil.h.
Also updated documentation.

Carrying over r+ from Jonas, but jduell still needs to review this.
Attachment #8523990 - Attachment is obsolete: true
Attachment #8523990 - Flags: review?(jduell.mcbugs)
Attachment #8527915 - Flags: review+
Attachment #8527915 - Flags: review?(jduell.mcbugs)
Attached patch prot_2_ioservice.patch (obsolete) — Splinter Review
Updated documentation, carrying over r+ from Jonas.
Attachment #8523993 - Attachment is obsolete: true
Attachment #8523993 - Flags: review?(jduell.mcbugs)
Attachment #8527916 - Flags: review+
Attached patch prot_3_chrome.patch (obsolete) — Splinter Review
Switching reviewer from bsmedberg to Jonas. (bsmedberg is ok with that).
Attachment #8523654 - Attachment is obsolete: true
Attachment #8523654 - Flags: review?(benjamin)
Attachment #8527917 - Flags: review?(jonas)
Attached patch prot_5_extensions.patch (obsolete) — Splinter Review
Just updated reviewer in commit message. Carrying over r+ from karlt.
Attachment #8525603 - Attachment is obsolete: true
Attachment #8527918 - Flags: review+
Attached patch prot_8_netwerk.patch (obsolete) — Splinter Review
Converted a caller to use NS_NewChannel() instead of ioserive->NewChannel2().
Attachment #8523659 - Attachment is obsolete: true
Attachment #8523659 - Flags: review?(jduell.mcbugs)
Attachment #8527919 - Flags: review?(jduell.mcbugs)
Attached patch prot_10_uriloader.patch (obsolete) — Splinter Review
Updated reviewer from bz to smaug in commit message. Carrying over r+ from smaug.
Attachment #8523661 - Attachment is obsolete: true
Attachment #8527920 - Flags: review+
Attached patch prot_12_browser.patch (obsolete) — Splinter Review
Updating reviewer from bsmedberg to Jonas as well.
Attachment #8523663 - Attachment is obsolete: true
Attachment #8523663 - Flags: review?(benjamin)
Attachment #8527921 - Flags: review?(jonas)
Attached patch prot_13_docshell.patch (obsolete) — Splinter Review
Converted a caller to use NS_NewChannel() instead of ioserive->NewChannel2().
Carrying over r+ from smaug.
Attachment #8523664 - Attachment is obsolete: true
Attachment #8527922 - Flags: review+
Comment on attachment 8527916 [details] [diff] [review]
prot_2_ioservice.patch

Jason needs to review those bits as well.
Attachment #8527916 - Flags: review?(jduell.mcbugs)
Comment on attachment 8527915 [details] [diff] [review]
prot_1_netutil.patch

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

::: netwerk/base/public/nsNetUtil.h
@@ +197,5 @@
> + * @param aRequestingNode
> + *        The loadingDocument of the channel.
> + *        The loadingDocument of a channel is the document that requested
> + *        the load of the resource. It is *not* the resource itself, it's
> + *        the resource's caller or requester in which the load is happening.

This sounds too much like the triggering principal.

How about:

The element or document where the result of this request will be used. This is the document/element that will get access to the result of this request. For example for an image load, it's the document in which the image will be loaded. And for a CSS stylesheet it's the document whose rendering will be affected by 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.

For loads that are not related to any document, such as loads coming from addons or internal browser features, use null here.

@@ +205,5 @@
> + *        load. It is *NOT* the principal tied to the resource/URI that this
> + *        channel is loading, it's the principal of the resource's caller or
> + *        requester. For example, if this channel is loading an image from
> + *        http://b.com that is embedded in a document who's origin is
> + *        http://a.com, the loadingPrincipal is http://a.com.

Again, "responsible for load" sounds a little too similar to triggering principal.

How about:

The element or document where the result of this request will be used. This is generally the principal of the aRequestingNode. However for loads where aRequestingNode is null this argument still needs to 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 aRequestingNode is null. The only exception to this is for loads from WebWorkers since they don't have any nodes to be passed as aRequestingNode.

aRequestingPrincipal is *not* the principal of the resource being loaded. But rather the principal of the context where the resource will be used.

@@ +208,5 @@
> + *        http://b.com that is embedded in a document who's origin is
> + *        http://a.com, the loadingPrincipal is http://a.com.
> + * @param aTriggeringPrincipal
> + *        The triggeringPrincipal of the load.
> + *        The triggeringPrincipal is the principal that triggerd the load.

"The triggeringPrincipal is the principal of the resource that caused this particular URL to be loaded."

The rest of this one sounds great.
Attachment #8527915 - Flags: feedback+
Comment on attachment 8527921 [details] [diff] [review]
prot_12_browser.patch

+      if (aLoadInfo) {
+        rv = NS_NewURI(getter_AddRefs(tempURI),
+                       nsDependentCString(kRedirMap[i].url));
+        NS_ENSURE_SUCCESS(rv, rv);
+        rv = NS_NewChannelInternal(getter_AddRefs(tempChannel),
+                                   tempURI,
+                                   aLoadInfo);
+      }
+      else {
+        rv = ioService->NewChannelFromURI(tempURI, getter_AddRefs(tempChannel));
+      }

tempURI is not initialized in the else case.
Comment on attachment 8527922 [details] [diff] [review]
prot_13_docshell.patch

Same here, tempURI not initialized for the else case.  Put this before the if(aLoadInfo) instead of inside it:
+              rv = NS_NewURI(getter_AddRefs(tempURI), kRedirMap[i].url);
+              NS_ENSURE_SUCCESS(rv, rv);
Comment on attachment 8527915 [details] [diff] [review]
prot_1_netutil.patch

Steve volunteered to take over here. Chattet with Jason about the change in reviewers. He is fine with that.
Attachment #8527915 - Flags: review?(jduell.mcbugs) → review?(sworkman)
Attachment #8527916 - Flags: review?(jduell.mcbugs) → review?(sworkman)
Attachment #8527919 - Flags: review?(jduell.mcbugs) → review?(sworkman)
Comment on attachment 8527915 [details] [diff] [review]
prot_1_netutil.patch

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

r=me with some checks on comments.

::: netwerk/base/public/nsNetUtil.h
@@ -191,5 @@
> - * How to create a new Channel using NS_NewChannel:
> - *  1) Please try to call NS_NewChannel providing a requesting *nsINode*
> - *  2) If no requesting nsINode is available,
> - *     call NS_NewChannel providing a requesting *nsIPrincipal*.
> - *  3) Call NS_NewChannelInternal *only* if requesting Principal and

There's no mention of NS_NewChannelInternal in the new comment - is this on purpose?

@@ +735,1 @@
>  NS_NewInputStreamChannelInternal(nsIChannel**        outChannel,

This doesn't match the comment.
Attachment #8527915 - Flags: review?(sworkman) → review+
Comment on attachment 8527916 [details] [diff] [review]
prot_2_ioservice.patch

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

r=me with the comments updated, and check if you want to assert on nsIHttpHandler.

::: netwerk/base/public/nsIIOService.idl
@@ +96,5 @@
> +     *        The securityFlags of the channel.
> +     *        Any of the securityflags defined in nsILoadInfo.idl
> +     * @param aContentPolicyType
> +     *        The contentPolicyType of the channel.
> +     *        Any of the content types defined in nsIContentPolicy.idl

Looks like Jonas' comments on nsNetUtil.h could apply here too.

Same for nsIIOService2.idl.

::: netwerk/base/src/nsIOService.cpp
@@ +666,5 @@
> +        rv = pph->NewProxiedChannel2(aURI, nullptr, aProxyFlags, aProxyURI,
> +                                     loadInfo, result);
> +        // if calling NewProxiedChannel2() fails we try to fall back to
> +        // creating a new proxied channel by calling NewProxiedChannel().
> +        if (NS_FAILED(rv)) {

This should always work for nsIHttpHandler, right? Would it be worth it to have a DEBUG block here to assert that pph is NOT an nsIHttpHandler if NS_FAILED(rv)?

I know that we own other protocol handlers, e.g. FTP, that don't have an interface, so this wouldn't be a catch all. Up to you.

@@ +683,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +#ifdef DEBUG
> +  {
> +    // Make sure that all the individual protocolhandlers attach

nit: I think you can remove the outer braces here; the 'if' braces should be enough.
Attachment #8527916 - Flags: review?(sworkman) → review+
Comment on attachment 8527919 [details] [diff] [review]
prot_8_netwerk.patch

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

::: netwerk/protocol/about/nsAboutBlank.cpp
@@ +25,5 @@
>      nsCOMPtr<nsIChannel> channel;
> +    // Bug 1087720 (and Bug 1099296):
> +    // Once all callsites have been updated to call NewChannel2()
> +    // instead of NewChannel() we should have a non-null loadInfo
> +    // consistently. Until then we have to brach on the loadInfo.

nit: s/brach/branch/

Check other files too, please.
Attachment #8527919 - Flags: review?(sworkman) → review+
I incorporated the suggested changes from Jonas, Tanvi and Steve and pushed the new patches to try:
  https://tbpl.mozilla.org/?tree=Try&rev=d371511d4e3c

We are failing tests whenever we try to create a channel using a scheme of "moz-page-thumb".
One of the failing tests is e.g. newtab/browser_newtab_background_captures.js. In that testcase we have a non-null 'aLoadingNode', hence we create a loadInfo but then we can't call handler->NewChannel*2*, but rather fall back to NewChannel, which then of course does *not* attach a loadInfo. We then assert that the newly created channel has a loadInfo attached, which fails in that case because we call newChannel() instead of NewChannel*2*. See also:
  https://bugzilla.mozilla.org/attachment.cgi?id=8527916&action=diff
(Please note that I haven't uploaded the latest patches yet, but the assertion code is unchanged).

Jonas, can we loosen the assertion a little and only assert if we where actually able to call NewChannel*2* on the handler within the ioservice? I think we should have a boolean flag that tells us whether we where able to call NewChannel2 or had to fall back on NewChannel.

Alternatively we can also update the code for "moz-page-thumb" right now and land with this patch (instead of updating it within the patches in Bug 1087720). What would you prefer? I rather lean towards loosing the assertion, because this problem might not only be in code we control, right?
Flags: needinfo?(jonas)
I thought we had decided not to check that assertion in, but rather just run it through mochitest. Since any addon that provides a protocol handler will assert for the same reason that you're seeing. So yes, I think we should just assert that there's a loadinfo if NewChannel2 succeeded.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #73)
> I thought we had decided not to check that assertion in, but rather just run
> it through mochitest. Since any addon that provides a protocol handler will
> assert for the same reason that you're seeing. So yes, I think we should
> just assert that there's a loadinfo if NewChannel2 succeeded.

Thanks for the speedy response. Initially I had an assertion in NewChannelInternal() in NetUtil.h. Since you requested to move it to the ioservice (see https://bugzilla.mozilla.org/show_bug.cgi?id=1087442#c31) I thought you want me to keep it.

Going to use a boolean-flag and keep the assertion, because it makes me feel way better to know that there is an assertion if NewChannel2 succeeds.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #74)
> (In reply to Jonas Sicking (:sicking) from comment #73)
> > I thought we had decided not to check that assertion in, but rather just run
> > it through mochitest. Since any addon that provides a protocol handler will
> > assert for the same reason that you're seeing. So yes, I think we should
> > just assert that there's a loadinfo if NewChannel2 succeeded.

At the moment we can not keep the assertion in ioservice even if we can ensure that we call NewChannel2 with a loadInfo.

Reason is:
We haven't added a NewChannel2 to nsIAboutModule, but rather extended NewChannel() with a LoadInfo argument, see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsIAboutModule.idl#22

Now, here is what happens:
We call NewChannel2() on the AboutProtocolHandler which internally calls NewChannel(), passing the loadInfo along to some class that implements nsIAboutModule. Now, in some cases the implementation of nsIAboutModule is in JS, and not in C++, see for example:
http://mxr.mozilla.org/mozilla-central/search?string=newChannel%3A%2Bfunction%28aURI%29
(Note that this was a trivial search, so there might be some more JS objects/classes that implement nsIAboutModule).

In such a case the call to newChannel() succeeds, because the additional loadInfo argument is just ignored by JS but then the assertion in the ioservice gets triggered because there is no loadInfo on the final channel.

Jonas, I would like your input again, would you rather:
a) eliminate the assertion for now and wait till we have updated JS callers/objects, or do you want to
b) extend the JS objects/classes that implement nsIAboutModule by the additional loadInfo argument in newChannel?
Flags: needinfo?(jonas)
This is why I was arguing that we should move setting the loadinfo from the individual nsIAboutModules to the about protocol handler. That seems like a simpler solution. Otherwise we'll have to hunt down each individual about module as well as create wrappers for addon-provided nsIAboutModules. That seems like a lot of busy work for very little tangible benefit.
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #76)
> This is why I was arguing that we should move setting the loadinfo from the
> individual nsIAboutModules to the about protocol handler. That seems like a
> simpler solution. Otherwise we'll have to hunt down each individual about
> module as well as create wrappers for addon-provided nsIAboutModules. That
> seems like a lot of busy work for very little tangible benefit.

Jonas, you convinced me. This patch will make some of the other patches in this bug obsolete. If you are fine with the changes in this patch, then I will qfold the changes into the other patches in this bug (making some of them disappear).

Using this patch we set the loadInfo inside the AboutProtocolHandler instead of each individual aboutModule.
Attachment #8530758 - Flags: review?(jonas)
Comment on attachment 8530758 [details] [diff] [review]
prot_14_remove_loadInfo_from_aboutModule.patch

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

You could also keep the aLoadInfo argument in order to allow aboutmodules to use it if they want/need to. Then let nsAboutProtocolHandler only add the loadinfo if it's missing. This seems useful in the two AboutRedirector classes.

But this is fine too.
Attachment #8530758 - Flags: review?(jonas) → review+
Blocks: 1099585
Uploading a new set of patches, what has changed:
* nsAboutProtocolHandler set a LoadInfo in case an individual nsIAboutModule doesn't set it
* NS_NewChannel(LoadInfo) calls NS_NewChannel(individualLoadInfoArgs) but then overwrites the loadInfo on that channel, so we have them very same instance on the channel. Further, we don't have to propagate the baseURI all the way to the ioservice because we overwrite the loadInfo already on the channel with the loadInfo that has the baseURI in it. More precise only nsDocShell call NS_newChannel(LoadInfo) where a baseURI is ever set. (At least at the moment).
* We are updating the securityManager to not create a nullPrincipal in case the principal was system (see patch prot_14_securitymanager).

That's it!
Carrying over r+ from already reviewed patches!
Attachment #8523657 - Attachment is obsolete: true
Attachment #8523658 - Attachment is obsolete: true
Attachment #8523660 - Attachment is obsolete: true
Attachment #8523662 - Attachment is obsolete: true
Attachment #8523998 - Attachment is obsolete: true
Attachment #8527914 - Attachment is obsolete: true
Attachment #8527915 - Attachment is obsolete: true
Attachment #8527916 - Attachment is obsolete: true
Attachment #8527917 - Attachment is obsolete: true
Attachment #8527918 - Attachment is obsolete: true
Attachment #8527919 - Attachment is obsolete: true
Attachment #8527920 - Attachment is obsolete: true
Attachment #8527921 - Attachment is obsolete: true
Attachment #8527922 - Attachment is obsolete: true
Attachment #8530758 - Attachment is obsolete: true
Attachment #8534688 - Flags: review+
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Attachment #8534689 - Flags: review+
Attached patch prot_2_ioservice.patch (obsolete) — Splinter Review
Attachment #8534691 - Flags: review+
Attached patch prot_4_dom.patch (obsolete) — Splinter Review
Attachment #8534696 - Flags: review+
Attached patch prot_10_uriloader.patch (obsolete) — Splinter Review
Attachment #8534702 - Flags: review+
Attached patch prot_14_securitymanager.patch (obsolete) — Splinter Review
I already ran tests locally. The most important two are:
  browser_bug1104623.js (the blob bug)
and also
  browser_bug464222.js (the baseUri bug)

Both of them are working correctly. Will push to try later tonight and hopefully land this tomorrow!
Attachment #8534710 - Flags: review?(jonas)
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Jonas, it doesn't really make sense to have a separate patch on top just replacing requestingPrincipal with loadingPrincipal and requestingNode with loadingNode.

Since it's just text replacement I incorporated the change within this patch and also in the following nsIoservice patch. Should be consistent now.
Attachment #8534689 - Attachment is obsolete: true
Attachment #8535264 - Flags: review+
Attached patch prot_2_ioservice.patch (obsolete) — Splinter Review
Attachment #8534691 - Attachment is obsolete: true
Attachment #8535265 - Flags: review+
Had this on try, looks good to me:
  https://tbpl.mozilla.org/?tree=Try&rev=4ab13da0e513

Finally, we are all set and ready to go with this one!
Smaug already r+ the intial patch, but we forgot to actually implement the ::SetLoadInfo for nsExtProcolHandler. Jonas, that should be ok, right?
Attachment #8534702 - Attachment is obsolete: true
Attachment #8535374 - Flags: review?(jonas)
So we're still always attaching a loadinfo in NS_NewChannel. Per comment 0 that was the main thing this bug was supposed to change, no? :)

https://hg.mozilla.org/integration/mozilla-inbound/file/3f4166fb5e37/netwerk/base/public/nsNetUtil.h#l306
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Thanks Jonas for catching that - we actually have to revert the changes from Bug 1104623, which was just a regression fix.

Since we missed to revert that change here I would also appreciate if you could take another look at prot_2_ioservice to make sure those are the changes we indeed want to land. Thanks!
Attachment #8535264 - Attachment is obsolete: true
Attachment #8535397 - Flags: review?(jonas)
Comment on attachment 8534710 [details] [diff] [review]
prot_14_securitymanager.patch

So, the landing plan slightly changed. Marking this patch as obsolete because we are going to revert/update the hotfix from Bug 1104623 in Bug 1110615.

In other words, updating the SecurityManager is the right thing to do to make blobs work correctly, hence we will do that in a separate Bug - Bug 1110615.

Once bug 1110615 landed, we will land the patches in this bug.
Attachment #8534710 - Attachment is obsolete: true
Attached patch prot_1_netutil.patch (obsolete) — Splinter Review
Rebased the patch on top of Bug 1110615 and pushed everything to try:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=909e14fc6010

(Carrying over r+ from Jonas)
Attachment #8535397 - Attachment is obsolete: true
Attachment #8535417 - Flags: review+
Attached patch prot_4_dom.patchSplinter Review
Just rebased on top of Bug 1110615. Carrying over r+.
Attachment #8534696 - Attachment is obsolete: true
Attachment #8535425 - Flags: review+
Comment on attachment 8535417 [details] [diff] [review]
prot_1_netutil.patch

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

Sorry, some more documentation nits:

::: netwerk/base/public/nsNetUtil.h
@@ +210,5 @@
> +*        For loads that are not related to any document, such as loads coming
> +*        from addons or internal browser features, use null here.
> +* @param aLoadingPrincipal
> +*        The loadingPrincipal of the channel.
> +*        The principal of the document where the result of this request will be used.

Over 80 chars.

@@ +219,5 @@
> +*        features, pass the system principal.
> +*        This principal should almost always be the system principal if
> +*        aLoadingNode is null. The only exception to this is for loads
> +*        from WebWorkers since they don't have any nodes to be passed as
> +*        aLoadingNode. Please note, aLoadingPrincipal is *not* the

Add a newline before "Please"

@@ +242,5 @@
> +*        Any of the content types defined in nsIContentPolicy.idl
> +*
> +* Please note, if you provide a loadingNode, then the loadingPrincipal
> +* must match the loadingNode->NodePrincipal(). Alternatively you can
> +* discard the loadingPrincipal if you provide a loadingNode.

"If you provide both a loadingNode and a loadingPrincipal, then loadingPrincipal must be equal to loadingNode->NodePrincipal(). But less error prone is to just supply a loadingNode."

@@ +244,5 @@
> +* Please note, if you provide a loadingNode, then the loadingPrincipal
> +* must match the loadingNode->NodePrincipal(). Alternatively you can
> +* discard the loadingPrincipal if you provide a loadingNode.
> +*
> +* What specific API function to use:

Move this section to the top of the comment. Otherwise many will likely not reading before getting here and instead use the NewChannelInternal function.

@@ +245,5 @@
> +* must match the loadingNode->NodePrincipal(). Alternatively you can
> +* discard the loadingPrincipal if you provide a loadingNode.
> +*
> +* What specific API function to use:
> +* * If possible try to call NS_NewChannel() providing a loading *nsINode*

"If possible, use NS_NewChannel()..."

@@ +249,5 @@
> +* * If possible try to call NS_NewChannel() providing a loading *nsINode*
> +* * If no loading *nsINode* is avaialable, call NS_NewChannel() providing
> +*   a loading *nsIPrincipal*.
> +* * Call NS_NewChannelWithTriggeringPrincipal() if the loading principal
> +*   and the Node's principal have to be different (see above).

"Call NS_NewChannelWithTriggeringPrincipal if the triggeringPrincipal is different from the loadingPrincipal.

@@ +251,5 @@
> +*   a loading *nsIPrincipal*.
> +* * Call NS_NewChannelWithTriggeringPrincipal() if the loading principal
> +*   and the Node's principal have to be different (see above).
> +* * Call NS_NewChannelInternal() providing aLoadInfo object in cases where
> +*   you already have loadInfo object, e.g in case of a channel redirect.

Please add:
* The NS_NewChannelInternal functions should almost never be directly
  called outside of necko code.

Maybe even as the first bullet.
Comment changes only as requested by Jonas!
Attachment #8535417 - Attachment is obsolete: true
Attachment #8535675 - Flags: review+
Comment changes only as requested by Jonas!
Attachment #8535265 - Attachment is obsolete: true
Attachment #8535676 - Flags: review+
Depends on: 1111017
No longer blocks: 1111304
Depends on: 1111304
Apparently the first patch did not get pushed:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3fbb01003d
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/bc3fbb01003d
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Depends on: 1113323
Depends on: 1180389
You need to log in before you can comment on or make changes to this bug.