1257339 - Bring back deprecated newChannel() API on nsIIOService

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

After some discusson and mostly because of Bug 1257111, we should bring back all of the newChannel() functions on nsIIOservice. We should also:

* log a deprecation warning to the console
* use the systemPrincipal as the loadingPrincipal
* update the docs for addon authors
* write a blog post about the deprecation

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Jorge, we are going to bring back the API on nsIOService. I was wondering where we should log a deprecation warning to? Should we simply log to the browser/web console? Is that where addon authors check for errors when developing their addons? Or should we also do something in addition to that?

Comment 3

[Jorge Villalobos [:jorgev] (he/him)]

Logging to the browser console should be sufficient. Developers and add-on reviewers should be checking that console while testing.

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1257339_bring_back_deprecated_API_nsioservice.patch.patch

As agreed, we should bring back the API on nsioService. I updated the following to improve the old code though:

1) Log a console warning that the API is deprecated.
2) Call the new API internally using SystemPrincipal as the loadingPrincipal as well as using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL as the securityFlags.
3) Added a test to make sure the old API still works correctly.

Comment 5

[Nils Maier [:nmaier]]

Comment on attachment 8731829 [details] [diff] [review]
bug_1257339_bring_back_deprecated_API_nsioservice.patch.patch

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

Thanks Christoph for reconsidering.
You didn't ask for my review, but you get it anyway :)
Except for the minor stuff, lgtm

::: netwerk/base/nsIOService.cpp
@@ +681,5 @@
> +  NS_ASSERTION(false, "Deprecated, use NewChannelFromURI2 providing loadInfo arguments!");
> +
> +  const char16_t* params[] = {
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI()").get(),
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI2()").get()

MOZ_UTF16("literal")

@@ +684,5 @@
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI()").get(),
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI2()").get()
> +  };
> +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> +                                  NS_LITERAL_CSTRING("Security by Default: "),

Really minor nit: A category name usually does not end with ": " IIRC

@@ +898,5 @@
> +  NS_ASSERTION(false, "Deprecated, use NewChannelFromURIWithProxyFlags2 providing loadInfo arguments!");
> +
> +  const char16_t* params[] = {
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURIWithProxyFlags()").get(),
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURIWithProxyFlags2()").get()

MOZ_UTF16("literal")

@@ +960,5 @@
> +  NS_ASSERTION(false, "Deprecated, use NewChannel2 providing loadInfo arguments!");
> +
> +  const char16_t* params[] = {
> +    NS_LITERAL_STRING("nsIOService::NewChannel()").get(),
> +    NS_LITERAL_STRING("nsIOService::NewChannel2()").get()

MOZ_UTF16("literal")

::: netwerk/locales/en-US/necko.properties
@@ +41,5 @@
>  TrackingUriBlocked=The resource at "%1$S" was blocked because tracking protection is enabled.
> +
> +# LOCALIZATION NOTE (APIDeprecationWarning):
> +# %1$S is the deprected API; %2$S is the API function that should be used.
> +APIDeprecationWarning =Warning: '%1$S' deprecated, please use '%2$S'

"APIDeprecationWarning =" -> "APIDeprecationWarning="

::: netwerk/test/unit/test_NetUtil.js
@@ +640,5 @@
> +  do_check_true(uri.equals(channel.URI));
> +
> +  run_next_test();
> +
> +}

No tests for:
- newChannel(string, charset, base)
- newChannel(nsIFile)

Comment 6

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

Comment on attachment 8731829 [details] [diff] [review]
bug_1257339_bring_back_deprecated_API_nsioservice.patch.patch

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

r=me if you also fix Nils comments.

::: netwerk/base/nsIOService.cpp
@@ +684,5 @@
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI()").get(),
> +    NS_LITERAL_STRING("nsIOService::NewChannelFromURI2()").get()
> +  };
> +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> +                                  NS_LITERAL_CSTRING("Security by Default: "),

I don't actually know how these categories work. Please test to make sure that this actually generates a warning as expected.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1257339_bring_back_deprecated_API_nsioservice.patch

> No tests for:
> - newChannel(string, charset, base)
> - newChannel(nsIFile)

Patrick, I extended test_NetUtil.js to also test the above, everything else remains unchanged (besides incorporating the nits from Jonas and Nils). Can you do a sanity check for test_deprecated_newChannel_API_with_nsIFile before we land that?

Comment 8

[Christoph Kerschbaumer [:ckerschb}]

> > +  nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
> > +                                  NS_LITERAL_CSTRING("Security by Default: "),
> 
> I don't actually know how these categories work. Please test to make sure
> that this actually generates a warning as expected.

I tested that we log to the console correctly. I tested for using NetUtil.newChannel() using the deprecated API as well as for ioservice::newChannel(). I don't know how this categories work either, but I checked other console message and the 'aCategory' argument is not displayed for them either. Since also other parts in the codebase use various categories [e.g. 1], I suppose it's fine to use 'Security By Default'. I am happy to update if I get any better suggestions. Either way, we log a nice warning to the console.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#2194

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1257339_bring_back_deprecated_API_nsioservice.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Created attachment 8732273 [details] [diff] [review]
> bug_1257339_bring_back_deprecated_API_nsioservice.patch
> 
> > No tests for:
> > - newChannel(string, charset, base)
> > - newChannel(nsIFile)
> 
> Patrick, I extended test_NetUtil.js to also test the above, everything else
> remains unchanged (besides incorporating the nits from Jonas and Nils). Can
> you do a sanity check for test_deprecated_newChannel_API_with_nsIFile before
> we land that?

rrh wrong file - here we go - thank you!

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8732277 [details] [diff] [review]
bug_1257339_bring_back_deprecated_API_nsioservice.patch

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

::: netwerk/test/unit/test_NetUtil.js
@@ +642,5 @@
> +
> +  run_next_test();
> +}
> +
> +function test_deprecated_newChannel_API_with_nsIFile()

lgtm

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e41e3bc842df

Comment 12

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

I had to back this out because something from the push that included this seems to have broken Windows mochitests like https://treeherder.mozilla.org/logviewer.html#?job_id=24152010&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb4d8c21d7d1

Comment 13

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

I'm thinking it was Bug 1209273 (also checked in in the same push), but I'll wait for confirmation before re-landing anything.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15654b89a062

Comment 15

[Gary [:streetwolf52]]

Will add-ons that made changes per the original patch still work?

Comment 16

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

Relanding this because I think this patch was okay:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31503dab99d

Comment 17

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Gary [:streetwolf] from comment #15)
> Will add-ons that made changes per the original patch still work?

Yes, addons that already updated to use the new API should still work.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Mike Kaply [:mkaply]]

Was a blog post ever done for this?

In particular, a "You used to do it this way, now do it this way" post? Particular for addon developers?

Comment 20

[Jorge Villalobos [:jorgev] (he/him)]

It was mentioned in https://blog.mozilla.org/addons/2016/05/17/compatibility-for-firefox-48/. But there were so many changes around this (as I recall) that I'm not sure if it's still accurate. I haven't heard many complaints from developers about this, though.

Comment 21

[Jonathan Kingston [:jkt] he/him]

This can be removed again safely right?

Comment 22

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Jonathan Kingston [:jkt] from comment #21)
> This can be removed again safely right?

Yes, indeed. We should file a new bug to get that part removed - thanks!