Bring back deprecated newChannel() API on nsIIOService

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1254752
(Assignee)

Updated

2 years ago
No longer blocks: 1254752
(Assignee)

Comment 2

2 years ago
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?
Flags: needinfo?(jorge)
Logging to the browser console should be sufficient. Developers and add-on reviewers should be checking that console while testing.
Flags: needinfo?(jorge)
(Assignee)

Comment 4

2 years ago
Created attachment 8731829 [details] [diff] [review]
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.
Attachment #8731829 - Flags: review?(mcmanus)
Attachment #8731829 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Depends on: 1254752
(Assignee)

Updated

2 years ago
Blocks: 1257111
(Assignee)

Updated

2 years ago
Depends on: 1256030
Attachment #8731829 - Flags: review?(mcmanus) → review+

Comment 5

2 years ago
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)
Attachment #8731829 - Flags: feedback+
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.
Attachment #8731829 - Flags: review?(jonas) → review+
(Assignee)

Comment 7

2 years ago
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?
Attachment #8731829 - Attachment is obsolete: true
Attachment #8732273 - Flags: review?(mcmanus)
(Assignee)

Comment 8

2 years ago
> > +  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
(Assignee)

Comment 9

2 years ago
Created attachment 8732277 [details] [diff] [review]
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!
Attachment #8732273 - Attachment is obsolete: true
Attachment #8732273 - Flags: review?(mcmanus)
Attachment #8732277 - Flags: review?(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
Attachment #8732277 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Blocks: 1257930

Comment 12

2 years ago
backoutbugherderlanding
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
I'm thinking it was Bug 1209273 (also checked in in the same push), but I'll wait for confirmation before re-landing anything.
Flags: needinfo?(mozilla)

Comment 15

2 years ago
Will add-ons that made changes per the original patch still work?
Relanding this because I think this patch was okay:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c31503dab99d
(Assignee)

Comment 17

2 years ago
(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.
Flags: needinfo?(mozilla)

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c31503dab99d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Updated

2 years ago
Depends on: 1259511
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?
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.
You need to log in before you can comment on or make changes to this bug.