1087442 - Attach LoadInfo in newChannel2 inside each individual ProtocolHandler

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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

Comment 1

[Tanvi Vyas[: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.

Comment 2

[Tanvi Vyas[:tanvi]]

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&hitlimit=&tree=mozilla-central

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

Comment 3

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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

Comment 4

[Tanvi Vyas[:tanvi]]

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

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: ioservice_create_loadinfo-11-14-14.patch

Comment 6

[Tanvi Vyas[:tanvi]]

Attachment: newinputstreamchannel-withloadinfo-11-14-14.patch

Comment 7

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_modules_libjar

Comment 8

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_image

Comment 9

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_dom

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_chrome

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_toolkit

Comment 12

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_uriloader

Comment 13

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_widget

Comment 14

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_netwerk

Comment 15

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_aboutmodule

Comment 16

[Tanvi Vyas[:tanvi]]

Attachment: protocolhandler_setloadinfo_extensions

Comment 17

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

Comment 18

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

Comment 19

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_3_chrome.patch

Comment 20

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_4_dom.patch

Comment 21

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_5_extensions.patch

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?

Comment 22

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_6_image.patch

Comment 23

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_7_modules.patch

Comment 24

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_8_netwerk.patch

Comment 25

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_9_toolkit.patch

Comment 26

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_10_uriloader.patch

Boris is too busy at the moment. Would you mind reviewing that?

Comment 27

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_11_widget.patch

Comment 28

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_12_browser.patch

Comment 29

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_13_docshell.patch

Comment 30

[Gian-Carlo Pascutto [:gcp]]

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 31

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

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

Comment 32

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

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);

Comment 33

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

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.

Comment 34

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

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.

Comment 35

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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.

Comment 36

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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.

Comment 37

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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!

Comment 38

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_0_loadinfo.patch

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?

Comment 39

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

Moving the assetion from nsnetutil.h into the ioservice. Carrying over r+ from Jonas.

Comment 40

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

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.

Comment 41

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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.

Comment 42

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_4_dom.patch

Just fixed a nit. Carrying over r+ from Jonas.

Comment 43

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

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.

Comment 44

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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?

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 46

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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 47

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 48

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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.

Comment 49

[Karl Tomlinson (:karlt)]

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()?

Comment 50

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_5_extensions.patch

(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!

Comment 51

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8523657 [details] [diff] [review]
prot_6_image.patch

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

Looks good to me!

Comment 52

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

(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?

Comment 53

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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?

Comment 54

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8523664 [details] [diff] [review]
prot_13_docshell.patch

Not sure me reviewing the same patch twice is needed ;)

Comment 55

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_0_loadinfo.patch

Updated assertion based on Comment 43 - carrying over r+ from Jonas.

Comment 56

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

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.

Comment 57

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

Updated documentation, carrying over r+ from Jonas.

Comment 58

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_3_chrome.patch

Switching reviewer from bsmedberg to Jonas. (bsmedberg is ok with that).

Comment 59

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_5_extensions.patch

Just updated reviewer in commit message. Carrying over r+ from karlt.

Comment 60

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_8_netwerk.patch

Converted a caller to use NS_NewChannel() instead of ioserive->NewChannel2().

Comment 61

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_10_uriloader.patch

Updated reviewer from bz to smaug in commit message. Carrying over r+ from smaug.

Comment 62

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_12_browser.patch

Updating reviewer from bsmedberg to Jonas as well.

Comment 63

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_13_docshell.patch

Converted a caller to use NS_NewChannel() instead of ioserive->NewChannel2().
Carrying over r+ from smaug.

Comment 64

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Comment on attachment 8527916 [details] [diff] [review]
prot_2_ioservice.patch

Jason needs to review those bits as well.

Comment 65

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

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.

Comment 66

[Tanvi Vyas[:tanvi]]

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 67

[Tanvi Vyas[:tanvi]]

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 68

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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.

Comment 69

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 70

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 71

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 72

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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?

Comment 73

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

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.

Comment 74

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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.

Comment 75

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

(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?

Comment 76

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

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.

Comment 77

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_14_remove_loadInfo_from_aboutModule.patch

(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.

Comment 78

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

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.

Comment 79

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_0_loadinfo.patch

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!

Comment 80

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

Comment 81

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

Comment 82

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_3_chrome.patch

Comment 83

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_4_dom.patch

Comment 84

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_5_extensions.patch

Comment 85

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_6_image.patch

Comment 86

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_7_modules.patch

Comment 87

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_8_netwerk.patch

Comment 88

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_9_toolkit.patch

Comment 89

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_10_uriloader.patch

Comment 90

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_11_widget.patch

Comment 91

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_12_browser.patch

Comment 92

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_13_docshell.patch

Comment 93

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_14_securitymanager.patch

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!

Comment 94

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

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.

Comment 95

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

Comment 96

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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!

Comment 97

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_10_uriloader.patch

Smaug already r+ the intial patch, but we forgot to actually implement the ::SetLoadInfo for nsExtProcolHandler. Jonas, that should be ok, right?

Comment 98

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Let's roll:
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=3f4166fb5e37

Comment 99

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

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

Comment 100

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7e85f193b8 at ckerschb's request.

Comment 101

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

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!

Comment 102

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

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.

Comment 103

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

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)

Comment 104

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_4_dom.patch

Just rebased on top of Bug 1110615. Carrying over r+.

Comment 105

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

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 106

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_1_netutil.patch

Comment changes only as requested by Jonas!

Comment 107

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Attachment: prot_2_ioservice.patch

Comment changes only as requested by Jonas!

Comment 108

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

alright, that should work now:
  https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2e0a0c0d7685

Comment 109

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6d556f4cd5a7
https://hg.mozilla.org/mozilla-central/rev/9f78c3062ad9
https://hg.mozilla.org/mozilla-central/rev/9cc6292a427c
https://hg.mozilla.org/mozilla-central/rev/c436f9b93cc9
https://hg.mozilla.org/mozilla-central/rev/9827f9f190fd
https://hg.mozilla.org/mozilla-central/rev/637d148890cc
https://hg.mozilla.org/mozilla-central/rev/675d4570a254
https://hg.mozilla.org/mozilla-central/rev/f3e8f9d63c96
https://hg.mozilla.org/mozilla-central/rev/b68eb56da34d
https://hg.mozilla.org/mozilla-central/rev/3ea94aed024a
https://hg.mozilla.org/mozilla-central/rev/c4449b63bf0d
https://hg.mozilla.org/mozilla-central/rev/34a7d8441386
https://hg.mozilla.org/mozilla-central/rev/2e0a0c0d7685

Comment 110

[Christoph Kerschbaumer [:ckerschb, back on May 21st]]

Apparently the first patch did not get pushed:
   https://hg.mozilla.org/integration/mozilla-inbound/rev/bc3fbb01003d

Comment 111

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bc3fbb01003d