Closed Bug 1076896 Opened 10 years ago Closed 10 years ago

Add newChannel2 to nsIoService that allows loadinfo propagation

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Similar to Bug 1067471, and Bug 1067468, we not only have to extend the nsIProtcolHandler and nsIAboutModule by a loadInfo argument for NewChannel, but also nsIOService. In addition, newChannelFromURI2 should also take a loadInfo.
Blocks: 1006868
Depends on: 1067471, 1067468
Summary: Add newChannel2 to nsIService that allows loadinfo propagation → Add newChannel2 to nsIoService that allows loadinfo propagation
Blocks: 1087442
Attached patch shim_2_ioservice.patch (obsolete) — Splinter Review
I think patches for (nsIProtocolHandler::newChannel2, nsIAboutModule::NewChannel including a loadinfo argument) and this one should land at the same time.

Once all of those landed, we can start with the shim in bug 1087442.
Attachment #8509696 - Flags: review?(jonas)
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment on attachment 8509696 [details] [diff] [review]
shim_2_ioservice.patch

As discussed in the meeting, we should pass individual arguments to the ioService and not the loadinfo. No need to review that patch, I will put up a new one incorporating that change.
Attachment #8509696 - Flags: review?(jonas)
Attached patch shim_2_ioservice.patch (obsolete) — Splinter Review
This is what we want, I think also defaulting to the NullPrincipal is what we want, right?
Attachment #8509696 - Attachment is obsolete: true
Attachment #8509854 - Flags: review?(jonas)
Blocks: 1087720
Comment on attachment 8509854 [details] [diff] [review]
shim_2_ioservice.patch

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

r=me with that fixed.

::: netwerk/base/src/nsIOService.cpp
@@ +598,5 @@
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> +    do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

No, loading with a nullprincipal is useless once we move security checks into AsyncOpen.

So just leave this as null.

@@ +679,5 @@
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> +    do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

Same here.

@@ +723,5 @@
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> +    do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

And here
Attachment #8509854 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #4)
> ::: netwerk/base/src/nsIOService.cpp
> @@ +598,5 @@
> > +{
> > +  nsresult rv;
> > +  nsCOMPtr<nsIPrincipal> nullPrincipal =
> > +    do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> No, loading with a nullprincipal is useless once we move security checks
> into AsyncOpen.
> 
> So just leave this as null.

I know it's not great, but better than using systemPrincipal, right? Unfortunately we can't leave it a nullptr, because LoadInfo requires a principal in it's constructor [1].

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/LoadInfo.cpp#26
Flags: needinfo?(jonas)
leave it null, but then make the IO service not create a loadinfo if both loadingnode and loadingprincipal is null
Flags: needinfo?(jonas)
Attached patch shim_2_ioservice.patch (obsolete) — Splinter Review
Incorporated Jonas suggestions. Carrying over r+.
Attachment #8509854 - Attachment is obsolete: true
Comment on attachment 8509956 [details] [diff] [review]
shim_2_ioservice.patch

Now for real :-)
Attachment #8509956 - Flags: review+
Carrying over r+. Just forgot to rev the uuid in the idl.
Attachment #8509956 - Attachment is obsolete: true
Attachment #8509971 - Flags: review+
Note that this doesn't make the new newChannel*2 functions actually set the loadinfo on the created channel. That needs to be done before we start migrating callers from newChannel* to newChannel*2.
(In reply to Jonas Sicking (:sicking) from comment #11)
> Note that this doesn't make the new newChannel*2 functions actually set the
> loadinfo on the created channel. That needs to be done before we start
> migrating callers from newChannel* to newChannel*2.

Intentionally this bug just changes the interface. We are going to attach the loadInfo to the newly created channel in Bug 1087442, which blocks the bugs which are going to change callsistes of newChannel in JS.
https://hg.mozilla.org/mozilla-central/rev/400577bd3bb0
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.