1110469 - Consider removing NS_OpenURI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

While we are still working on the overall 'necko security hooks revamp project', we should consider removing NS_OpenURI completely which would simplify nsNetutil.h a lot.

Comment 1

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

I could only find 10 callsites of NS_OpenURI* that wasn't in tests or in nsNetUtil.h

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: removeNSOpenURI.patch

Jonas, you are right, we should totally "nuke" NS_OpenURI :-)


Steve, how do you feel about reviewing this patch? If you are too busy, I can ask someone else, just let me know!


Interesting is that there are some callers appearing to be in dead code.
In particular:
./editor/libeditor/nsHTMLURIRefObject.cpp:  rv = NS_OpenURI(getter_AddRefs(theChannel), theURI, nullptr, theLoadGroup);
./xpcom/tests/CvtURL.cpp:  nsresult ec = NS_OpenURI(&in,
./netwerk/test/TestPerf.cpp:    rv = NS_OpenURI(getter_AddRefs(stream),
./netwerk/test/TestPerf.cpp:    nsresult rv = NS_OpenURI(listener,
./netwerk/test/TestPerf.cpp:        printf(">> NS_OpenURI failed [rv=%x]\n", rv);
./netwerk/test/TestThreadedIO.cpp:        printf( "Calling NS_OpenURI...\n" );
./netwerk/test/TestThreadedIO.cpp:        nsresult rv = NS_OpenURI(getter_AddRefs(result),
./netwerk/test/TestThreadedIO.cpp:            printf( "...NS_OpenURI completed OK\n" );
./netwerk/test/TestThreadedIO.cpp:            printf( "%s %d: NS_OpenURI failed, rv=0x%08X\n",
./netwerk/test/TestHttp.cpp:        RETURN_IF_FAILED(rv, "NS_OpenURI");
./netwerk/test/TestPageLoad.cpp:        RETURN_IF_FAILED(rv, -1, "NS_OpenURI");
./netwerk/test/TestProtocols.cpp:            LOG(("ERROR: NS_OpenURI failed for %s [rv=%x]\n", aUrlString, rv));
./netwerk/test/TestSyncHTTP.cpp:        rv = NS_OpenURI(getter_AddRefs(c[i].channel,
./netwerk/test/TestSyncHTTP.cpp:        RETURN_IF_FAILED(rv, "NS_OpenURI");

What should we do about them? Just ignore them? Update them so they would "somehow" compile in case we are going to use such code again?

Comment 3

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

Comment on attachment 8536894 [details] [diff] [review]
removeNSOpenURI.patch

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

::: modules/libjar/nsJARChannel.cpp
@@ +882,5 @@
>              else {
> +              rv = NS_NewChannel(getter_AddRefs(channel),
> +                                 mJarBaseURI,
> +                                 nsContentUtils::GetSystemPrincipal(),
> +                                 nsILoadInfo::SEC_NORMAL,

This is a problem in the existing code, but shouldn't this pass a null loadingPrincipal/triggeringPrincipal? It seems like guessing on a system principal is very risky. If the outer channel doesn't have a loadinfo, then it seems better if the inner one also doesn't have a loadinfo.


Now that (soon) NS_NewChannelInternal(loadinfo) passes the passed in loadinfo all the way through to the protocol handler, maybe we can allow NS_NewChannelInternal(loadinfo) to accept a null loadinfo, which would then return a channel with a null loadinfo.

It seems somewhat silly to require all channels that forward to other channels to have an if-branch for different code paths to create inner channels that have loadinfo and channels that don't.

Comment 4

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8536894 [details] [diff] [review]
removeNSOpenURI.patch

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

LGTM - just some nits.

Re the dead code, Pat should decide what to do here. Pat - should Christoph just remove those files or do you want them kept around for the future?

::: modules/libjar/nsJARChannel.cpp
@@ +871,5 @@
>          if (NS_SUCCEEDED(rv)) {
>              // Since we might not have a loadinfo on all channels yet
>              // we have to provide default arguments in case mLoadInfo is null;
>              if (mLoadInfo) {
> +              rv = NS_NewChannelInternal(getter_AddRefs(channel),

nit: indentation is off here (off in the original as well) - should be four spaces.

@@ +876,5 @@
> +                                         mJarBaseURI,
> +                                         mLoadInfo,
> +                                         mLoadGroup,
> +                                         mCallbacks,
> +                                         mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS));

nit: split this over two lines.

@@ +881,2 @@
>              }
>              else {

nit: While you're changing a lot of code around these lines, please change this to:

  } else {

Comment 5

[Patrick McManus [:mcmanus]]

the truly abandoned tests can be removed. please do it in a separate patch.

Comment 6

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

For now maybe just let abandoned-looking tests keep using NS_OpenURI. This way, if no-one updates them or complains, we'll know that they actually are abandoned. Does that sound ok?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #6)
> For now maybe just let abandoned-looking tests keep using NS_OpenURI. This
> way, if no-one updates them or complains, we'll know that they actually are
> abandoned. Does that sound ok?

That sounds reasonable to me, we can then revisit those files and delete if no one complains after some time.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 8536894 [details] [diff] [review]
> removeNSOpenURI.patch
> 
> Review of attachment 8536894 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libjar/nsJARChannel.cpp
> @@ +882,5 @@
> >              else {
> > +              rv = NS_NewChannel(getter_AddRefs(channel),
> > +                                 mJarBaseURI,
> > +                                 nsContentUtils::GetSystemPrincipal(),
> > +                                 nsILoadInfo::SEC_NORMAL,
> 
> This is a problem in the existing code, but shouldn't this pass a null
> loadingPrincipal/triggeringPrincipal? It seems like guessing on a system
> principal is very risky. If the outer channel doesn't have a loadinfo, then
> it seems better if the inner one also doesn't have a loadinfo.

I filed Bug 1119005 to investigate this problem. This bug should only remove NS_OpenURI.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> the truly abandoned tests can be removed. please do it in a separate patch.

I filed Bug 1119006 to do that.

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8536894 [details] [diff] [review]
removeNSOpenURI.patch

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

Thanks Chris. This plan sounds good to me. r=me (please check the nits).

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/643589c3ef94

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/643589c3ef94