1162657 - Turn NewChannel deprecation warnings into assertions

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

In Bug 1134096 we added deprecation warnings for newChannel() callers. The API is deprecated, and we should not land any more Gecko code using the deprecated API. It's time to turn the warnings into assertions.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

I think this one is crictial, hence I ask all of you for review this little patch:
* Jonas as the lead for the project
* sworkman as the network peer, and
* paolo, since he wrote the initial version of the new API in Bug 1125618.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e62a1ac96b

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Please note that the test already verifies that an exception is thrown within Netutil.jsm when wrong arguments are provided, see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_NetUtil.js#614

Comment 4

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

Comment on attachment 8602906 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

Don't you need to mark some test as having known assertions too?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Comment on attachment 8602906 [details] [diff] [review]
> bug_1162657_turn_newchannel_warnings_into_assertions.patch
> 
> Review of attachment 8602906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't you need to mark some test as having known assertions too?

The way Paolo wrote the new API within Netutil.jsm throws an exception in Netutil.jsm and does not pass the arguments along to Netutil.h or also ioservice.cpp in case some wrong arguments are passed. (see comment 3). The only thing that was missing was to account for nsIFile within Netutil.jsm.  All tests pass now.

Btw, marking tests to have known assertions using SimpleTest.ExpectAssertions() does not work for xpcshell tests; only for mochitests. I don't think there is an equivalent for xpcshell tests.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Forgot to include mochis:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1b10be273af

Comment 7

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

So that means that this still doesn't assert when someone calls newChannel*2 with aLoadingPrincipal and aLoadingNode both being null. That doesn't seem any better than calling newChannel*?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> So that means that this still doesn't assert when someone calls newChannel*2
> with aLoadingPrincipal and aLoadingNode both being null. That doesn't seem
> any better than calling newChannel*?

That's a good point. You are absolutely right, we should make sure that we should always be able to create a Loadinfo. In other words, we have to remove the following *if* here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#760

No additional assertion needed, because the Loadinfo constructor asserts that we get the right arguments:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#34

I will get that fixed tomorrow.

Comment 9

[:Paolo Amadini]

Comment on attachment 8602906 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

Since this is a public API entry point, I don't think an assertion is what we need. The effect of this would be that add-on code (or locally added Gecko code) would make debug builds crash but would still work in release builds.

There are two cases depending on what the plan is:
* If we want to make sure add-ons are manually updated to use the new API, we should simply return an XPCOM error code.
* If we just want to prevent new Gecko code using the old API from landing, we should use a mechanism that causes our automated tests to fail, both in debug and release builds - in fact most Firefox front-end developers use release builds locally, and they'd like to catch mistakes early. I think we have something like that for JS, but I'm not sure how to do this for C++. Yoric may know more.

Comment 10

[:Paolo Amadini]

Comment on attachment 8602906 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

::: netwerk/base/NetUtil.jsm
@@ +412,5 @@
>              );
>          }
>  
> +        // We either have a string or an nsIFile that we'll need a URI for.
> +        if ((typeof uri == "string") || (uri instanceof Ci.nsIFile)) {

Not allowing an nsIFile here was also an intentional choice, see the method's documentation and bug 1125618 comment 16.

You can probably just remove any test cases that refer to a deprecated API.

Comment 11

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

> * If we want to make sure add-ons are manually updated to use the new API,
> we should simply return an XPCOM error code.

We can't do this since it would likely break too many addons.

> * If we just want to prevent new Gecko code using the old API from landing,
> we should use a mechanism that causes our automated tests to fail, both in
> debug and release builds - in fact most Firefox front-end developers use
> release builds locally, and they'd like to catch mistakes early. I think we
> have something like that for JS, but I'm not sure how to do this for C++.
> Yoric may know more.

This might be ok yes.

But if we thinking "checking in to try" is too late, and we want to try to catch this earlier, then we want something that's more aggressive than just causing tests to fail, no?

Comment 12

[:Paolo Amadini]

(In reply to Jonas Sicking (:sicking) from comment #11)
> > * If we want to make sure add-ons are manually updated to use the new API,
> > we should simply return an XPCOM error code.
> 
> We can't do this since it would likely break too many addons.
> 
> > * If we just want to prevent new Gecko code using the old API from landing,
> > we should use a mechanism that causes our automated tests to fail, both in
> > debug and release builds - in fact most Firefox front-end developers use
> > release builds locally, and they'd like to catch mistakes early. I think we
> > have something like that for JS, but I'm not sure how to do this for C++.
> > Yoric may know more.
> 
> This might be ok yes.
> 
> But if we thinking "checking in to try" is too late, and we want to try to
> catch this earlier, then we want something that's more aggressive than just
> causing tests to fail, no?

Hm, what use case do you have in mind? When we add new code we normally run its associated test locally before pushing to try anyways. If for some reason we add code without a test, we haven't a great and easy way to distinguish that from add-on code. We would just print a console warning in this case.

Asking Yoric because we've probably had similar needs in the past and maybe we have something we could use from C++.

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

I had the same concern about asserting on NewChannel - seems like Addon devs might want to use the debug build.

I'm wondering if ENABLE_TESTS would be useful here? It would only be helpful if addon devs use debug build that we provide AND if those builds don't include tests. Seems like a long shot ...

Comment 14

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

We do kind'a want addon authors to switch to calling newChannel*2. Though ideally they should use NetUtil.newChannel since that's the cleanest and most extensible API, and thus least likely to change in backwards incompatible ways.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

I removed the nsIFile-test from unit/test_NetUtil.js and also had to update one callsite from asyncFetch to asyncFetch2.
We still need a solution for the assertion. Don't know at the moment, but definitely worth adding the updated patch.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

It seems that Yoric is working on a patch that is what we are looking for here. Added dependency to Bug 1080457.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

Jonas, Paolo, here is what we can do, we can ASSERT when running on TBPL, which is exactly what we want in my opinion, because it hinders Gecko devs to land code that still uses the depercated API but still allows Addons to use it. What do you think?

Comment 18

[:Paolo Amadini]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> we can ASSERT when running on TBPL,

This still wouldn't detect the error in local builds and in release builds.

Bug 1080457 seems definitely the API that we need, however it doesn't seem to be callable from C++ in the current patch. I'm not sure what is the state of console error detection in the test suites, but maybe using the Components.utils.reportError API from C++ would cause tests to fail?

Comment 19

[David Teller [:Yoric] - still alive but not very active]

I'm pretty sure that Cu.reportError doesn't cause the tests to fail. We could probably add a Cu.reportFatalError or somesuch, but that's pretty much Bug 1080457 over again.

Comment 20

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

I don't think that we should block this bug on finding a way to fail tests. I would imagine that lots of our assertions should cause us to fail tests in optimized builds.

Comment 21

[:Paolo Amadini]

I'd be fine with landing this as an NS_ASSERTION for lack of a better alternative. Differently from MOZ_ASSERT, this would allow execution to continue, though the modal dialogs can become pretty annoying on Windows. (Actually, NS_ANNOY would be a better name for the function.)

Front-end developers would still detect the error late since they're using optimized builds where NS_ASSERTION and MOZ_ASSERT are no-ops. People using debug builds with add-ons installed would have to suppress the warning dialogs using the associated environment variable.

I don't think we should encourage even more differences between local test runs and TBPL. Instead we should file a bug blocked by bug 1080457 to add an API to fail tests from C++ and use it here.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

Paolo, hopefully you are fine with landing a MOZ_ASSERT(). Jonas, Steve, Doug and myself have been discussing in person, and we all agree that MOZ_ASSERT() is the way to go for now (lacking better alternatives at the moment).

I would still need you to review the bits in test_NetUtil.js. As requested, I removed 'test_newChannel_with_nsIFile' completely.

Comment 23

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

Comment on attachment 8604948 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

This patch still doesn't assert if newChannel*2 is called, but not providing enough information to create a LoadInfo, no?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

You are absolutely right Jonas, thanks for pointing that out again. With this patch we cannot create a Channel without providing loadInfo the right arguments anymore.

Comment 25

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

Comment on attachment 8604970 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

::: netwerk/base/nsIOService.cpp
@@ +599,5 @@
>   */
>  NS_IMETHODIMP
>  nsIOService::NewChannelFromURI(nsIURI *aURI, nsIChannel **result)
>  {
> +  MOZ_ASSERT(false, "Deprecated, use NewChannelFromURI2 providing loadInfo arguments!");

Technically you can remove these since the function that you're calling with assert as needed. Though I guess this provides a better error message.

@@ +751,5 @@
> +                                                           aTriggeringPrincipal,
> +                                                           loadingNode,
> +                                                           aSecurityFlags,
> +                                                           aContentPolicyType);
> +    if (!loadInfo) {

This doesn't work. |new whatever(..)| always succeeds unless we're out of memory. In which case it exit()s gecko.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

> ::: netwerk/base/nsIOService.cpp
> > +  MOZ_ASSERT(false, "Deprecated, use NewChannelFromURI2 providing loadInfo arguments!");
> 
> Technically you can remove these since the function that you're calling with
> assert as needed. Though I guess this provides a better error message.

Yes, I think we should keep those because of the nice error message?

> > +    if (!loadInfo) {
> 
> This doesn't work. |new whatever(..)| always succeeds unless we're out of
> memory. In which case it exit()s gecko.

What's the alternative? Just remove the check?

Comment 27

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

No, keep the previous |if| statement but add a MOZ_ASSERT(loadinfo, "...") after.

And please remove the |if (!loadinfo)|, that's just dead code that makes the code harder to understand.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #27)
> No, keep the previous |if| statement but add a MOZ_ASSERT(loadinfo, "...")
> after.
> 
> And please remove the |if (!loadinfo)|, that's just dead code that makes the
> code harder to understand.

I am afraid I can't follow. If
> if (!loadInfo) {
>   return NS_ERROR_UNEXPECTED;
> }

is dead code, because loadInfo is never null, what's
> MOZ_ASSERT(loadinfo, "...")
doing? That would also never fail then, no?

The constructor of LoadInfo asserts that we pass the right arguments, it fails if no loadingPrincipal is provided (or the constructor cannot query the loadingPrincipal from the loadingNode). Either way, if NewChannel* is called where the loadingPrincipal ends up being null in the constructor, we hit the MOZ_ASSERT.

Or is MOZ_ASSERT() doing something different in release builds that I am not aware of?

Comment 29

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

The problem is that you removed the |if (aLoadingNode || aLoadingPrincipal)| check. So now the patch *always* creates a LoadInfo. But sometimes it creates a loadinfo which contains no information. The constructor will assert, but the loadinfo object is still created.

Instead keep the |if (aLoadingNode || aLoadingPrincipal)| and add a MOZ_ASSERT *after* the if statement. loadinfo will clearly be null if we never enter the if-statement, right?

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

> Instead keep the |if (aLoadingNode || aLoadingPrincipal)| and add a
> MOZ_ASSERT *after* the if statement. loadinfo will clearly be null if we
> never enter the if-statement, right?

Oh ok, now I get what you meant. Here we go.

Comment 31

[:Paolo Amadini]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> Paolo, hopefully you are fine with landing a MOZ_ASSERT(). Jonas, Steve,
> Doug and myself have been discussing in person, and we all agree that
> MOZ_ASSERT() is the way to go for now (lacking better alternatives at the
> moment).

Then I disagree with Jonas, Steve, Doug and you :-P

"MOZ_ASSERT is fatal: no recovery is possible."
(<http://mxr.mozilla.org/mozilla-central/source/mfbt/Assertions.h#270>)

Developers regularly using debug builds with add-ons installed would find their builds suddenly unusable unless they uninstall the offending add-on. (The offending add-on author is unlikely to change the code since it works well in release builds, and it doesn't even print a console message saying something is wrong.)

NS_ASSERTION allows overriding instead. I think that when people say "don't use NS_ASSERTION in new code" what they really mean is "for checking things that cannot happen unless there is a coding mistake, i.e. what people are used to call assertions, use MOZ_ASSERT rather than the confusingly named NS_ASSERTION."

In other words the case we're checking here is definitely _not_ the use case for MOZ_ASSERT. It isn't the use case for NS_ASSERTION either, but it's the thing that allows us to do the least damage without putting too much effort in implementing the right solution (which we've already identified).

In most other cases you'd really want MOZ_ASSERT rather than NS_ASSERTION, or simply return an XPCOM error code for public interfaces, for example when arguments are incorrect.

Comment 32

[:Paolo Amadini]

Comment on attachment 8605021 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

::: toolkit/components/jsdownloads/test/unit/head.js
@@ +532,5 @@
>        do_throw("File is empty: " + aPath);
>      }
>  
>      let deferred = Promise.defer();
> +    NetUtil.asyncFetch2(

NetUtil.asyncFetch2 is deprecated, use NetUtil.asyncFetch with an object argument instead, for example:

// This test code causes main-thread I/O, do not use in production.
NetUtil.asyncFetch(
  { uri: NetUtil.newURI(file), loadUsingSystemPrincipal: true },
  function(aInputStream, aStatus) {

(Hm, rewriting this function with OS.File could be "good next bug" for a new contributor.)

Comment 33

[:Paolo Amadini]

Another low-effort solution that does basically what we need is to set a preference only in the test suites, then check it in the deprecated code paths. If it is set, return an XPCOM error code in addition to writing an NS_WARNING. This doesn't affect performance, since non-deprecated code paths would not check the preference. Most tests that expected the channel to work would fail.

Here are the preference files used by the test suites:

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#Need_to_set_preferences_for_test-suites.3F

Comment 34

[:Paolo Amadini]

(In any case, please don't feel like you have to block on any of my feedback. I hope it can be useful to find the best compromise, but I'm all for expediency and if you feel you've spent already more time than you expected on this, go ahead with what you feel is right - we can always fix issues later.)

Comment 35

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

Comment on attachment 8605021 [details] [diff] [review]
bug_1162657_turn_newchannel_warnings_into_assertions.patch

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

Have you pushed this to try? You'll likely need to mark some of the tests as asserting.

r=me on the ioservice.cpp changes with the below fixed.

::: netwerk/base/nsIOService.cpp
@@ +767,2 @@
>      }
> +    MOZ_ASSERT(loadInfo, "Can not create a channel without a loadinfo");

I think Paolo is right that we might want to use NS_ASSERTION here. To enable developers to still have working addons and just be told that they need to update those addons.

Which also means that you should change the error message to something like "Please pass in security information when creating a new channel".

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1162657_turn_newchannel_warnings_into_assertions.patch

Thanks, Jonas, Steven and Paolo for your input. Finally I think we are going to end up using NS_ASSERTIONs, which is the best compromise we can come up with at the moment.

Steve, I would still like to have you sign off on the netwerk/ changes since you are a peer.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Ideally we shouldn't have to add expectedAssrtions to any of the mochitests, since all the code we (like we in mozilla) control should use the new API. But here it goes. Let's make sure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cbdb93007bf

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66a72727e1b3

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/66a72727e1b3

Comment 40

[Mike Kaply [:mkaply]]

This is quite confusing.

Are ioservice NewChannel2 and NewChannelFromURI deprecated?

They are marked that way in the code:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#663
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#903

but they are not marked that way in the IDL:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#149
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#166

and there are no references to the "2" APIs in the documentation:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIIOService