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.

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.

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

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 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?

(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.

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

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*?

(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 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 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.

> * 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?

(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++.

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 ...

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.

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.

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

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?

(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?

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.

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.

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.

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 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?

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 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.

> ::: 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?

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.

(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?

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?

> 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.

(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 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.)

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

(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 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".

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.

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

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