1121393 - Make JS callers of ios.newChannel call ios.newChannel2 in mozmill/

Status: RESOLVED INCOMPLETE

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The mentioned interfaces have been updated and Mozmill needs an update as it looks like. Christoph already pushed a PR to Github: https://github.com/mozilla/mozmill/pull/120

So there is one question from my side. With that change how does the backward compatibility looks like? Mozmill should stay compatible down to 31.0ESR and I doubt that this code would work in that release? If that is the case we will have to check first, which interface to use.

Marking it as blocker for the 2.1 release for now. Chris, is there a planned time when the old interface or method gets obsoleted?

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Henrik Skupin (:whimboo) from comment #0)
> So there is one question from my side. With that change how does the
> backward compatibility looks like? Mozmill should stay compatible down to
> 31.0ESR and I doubt that this code would work in that release? If that is
> the case we will have to check first, which interface to use.

It should work, because we are supporting both interfaces NewChannel() as well as NewChannel2() [same for NewChannelFromURI2, and asyncFetch in NetUtil.jsm] at the moment, see e.g.
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsIOService.cpp#611

We are facing similar problems for addons, which can implement their on Protocolhandlers and therefore might not implement NewChannel2. We are about to implement a shim that forwards calls in case NewChannel2() and soon asyncFetch2 is not implemeneted.

> Marking it as blocker for the 2.1 release for now. Chris, is there a planned
> time when the old interface or method gets obsoleted?

You are right, those interfaces will become obsolete eventually. We are not there yet. I will bring it up in our next meeting and follow up with you as soon as I have more information available.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Or is the question that newer versions of mozmill call newChannel2() e.g. on Firefox 31, while in fact NewChannel2() API is not available yet?

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Or is the question that newer versions of mozmill call newChannel2() e.g. on
> Firefox 31, while in fact NewChannel2() API is not available yet?

That's exactly the problem, yes. Mozmill has only a single version, which has to keep support for all supported versions of Firefox. So if the interface is not available in Firefox 31 we need the fallback. 

Will you have time to work on that change or should I find someone else?

Comment 4

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> > Or is the question that newer versions of mozmill call newChannel2() e.g. on
> > Firefox 31, while in fact NewChannel2() API is not available yet?
> 
> That's exactly the problem, yes. Mozmill has only a single version, which
> has to keep support for all supported versions of Firefox. So if the
> interface is not available in Firefox 31 we need the fallback. 
> 
> Will you have time to work on that change or should I find someone else?

Thanks for clarification. If you could find someone else to fix that, that would be great obviously. I am definitely available for feedback and any other support of course.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sounds good. So my proposal would be to check if the new methods are available on the io service and make use of them. Otherwise use the old methods.

Comment 6

[Ujjwal Wahi [:ujjwal]]

I would like work on this. I have Mozmill ready on my machine. Please guide further on how to proceed.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Ujjwal Wahi [:ujjwal] from comment #6)
> I would like work on this. I have Mozmill ready on my machine. Please guide
> further on how to proceed.

That's great - I think whimboo explains it in Comment 5 what we have to do. You can see the current patch here: https://github.com/ckerschb/mozmill/commit/249aee411980bb8474470df7d2d9dd953a1d3645

I think it's best if you wrap the call to newChannel2 into a try-catch block and call newChannel in case newChannel2 is not implemented. Does that make sense, otherwise let me know. Probably whimboo can also provide some guidance since he is the expert in mozmill code.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

That's right. Christoph explained it very well in addition to my comment 5. Use a try-catch to figure out which API to use. And when you are testing it please check with Firefox Nightly and ESR31 (which is the oldest release of Firefox we support). Mozmill has to work with all of them.

Please let me know if you need further information to get started. Thanks.

Comment 9

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1121393_api_changes.patch

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8563423 [details] [diff] [review]
bug1121393_api_changes.patch

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

Thanks for working on this and the quick turnaround time. Leaving some drive by notes.

::: mozmill/mozmill/extension/resource/stdlib/securable-module.js
@@ +318,5 @@
> +       // new interface newChannelFromURI2 may not be available
> +       try {
> +
> +         // look for newChannelFromURI2
> +         var channel = ios.newChannelFromURI2(newURI,

please define:
var channel = null;
outside the try-catch block and just assign within.

@@ +343,5 @@
> +       // new interface newChannel2 may not be available
> +       try {
> +
> +         // look for newChannel2
> +         var channel = ios.newChannel2(path,

same here, please define the channel outside the try-catch.

@@ +354,5 @@
> +                                       Ci.nsIContentPolicy.TYPE_OTHER);
> +       } catch(e) {
> +
> +         // fallback to newChannel
> +         var channel = ios.newChannel2(path, null, null);

that should be newChannel not newChannel2.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +464,5 @@
> +    null, // aLoadingNode
> +    Services.scriptSecurityManager.getSystemPrincipal(),
> +    null, // aTriggeringPrincipal
> +    Ci.nsILoadInfo.SEC_NORMAL,
> +    Ci.nsIContentPolicy.TYPE_OTHER);

After some review rounds in other patches we decided to align asyncFetch2 slightly different, have a look:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/browser/browser_favicon_setAndFetchFaviconForPage.js#33

Would be great if you could align it in the same fashion.

Comment 11

[Christoph Kerschbaumer [:ckerschb}]

:whimboo, we also need to make sure we are passing the right arguments to newChannel and asyncFetch; please find a description of the arguments here:
  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.idl#69

Any chance you can have a look at it?

Comment 12

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1121393_api_changes.patch

Indentation may be required in newChannel2, newChannelFromURI2 calls. Let me know if there is a need.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> :whimboo, we also need to make sure we are passing the right arguments to
> newChannel and asyncFetch; please find a description of the arguments here:
>  
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIIOService.
> idl#69

Shouldn't those be the defaults as also used in Firefox? We do not set anything specific for our tests framework. At least running all of the tests didn't show any problem by using the values as you have referenced.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8564585 [details] [diff] [review]
bug1121393_api_changes.patch

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

Functionality-wise all looks fine here. I added a couple of comments regarding style and method re-using. Please check those and update your patch. Thanks!

::: mozmill/mozmill/extension/resource/stdlib/securable-module.js
@@ +315,5 @@
>           baseURI = ios.newURI(base, null, null);
>         var newURI = ios.newURI(path, null, baseURI);
> +       var channel = null;
> +
> +       // new interface newChannelFromURI2 may not be available

It would be good to add here when this new interface was added to Firefox. So please add the version number or even better the bug reference.

@@ +317,5 @@
> +       var channel = null;
> +
> +       // new interface newChannelFromURI2 may not be available
> +       try {
> +

nit: please remove this empty line.

@@ +318,5 @@
> +
> +       // new interface newChannelFromURI2 may not be available
> +       try {
> +
> +         // look for newChannelFromURI2

This comment is not necessary.

@@ +320,5 @@
> +       try {
> +
> +         // look for newChannelFromURI2
> +         channel = ios.newChannelFromURI2(newURI,
> +                                              null, // aLoadingNode

Please fix your indentation. It should all be aligned with 'n'ewURI.

@@ +326,5 @@
> +                                              null, // aTriggeringPrincipal
> +                                              Ci.nsILoadInfo.SEC_NORMAL,
> +                                              Ci.nsIContentPolicy.TYPE_OTHER);
> +       } catch(e) {
> +

nit: please remove the empty line above and add a blank between `catch` and the opening bracket.

@@ +356,5 @@
> +                                       Ci.nsIContentPolicy.TYPE_OTHER);
> +       } catch(e) {
> +
> +         // fallback to newChannel
> +         channel = ios.newChannel(path, null, null);

Same comments as above apply here.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +448,5 @@
>  
> +  // new interface asyncFetch2 may not be available
> +  try {
> +
> +    // look for asyncFetch2

This comment is not necessary. Also please remove the above empty line.

@@ +451,5 @@
> +
> +    // look for asyncFetch2
> +    NetUtil.asyncFetch2(
> +      dataURI,
> +      function (aInputStream, aAsyncFetchResult) {

The callback method you define here is exactly the same as in the catch block. To avoid duplication of code please declare it outside of the try block, and use a reference here and under catch.

@@ +470,5 @@
> +      Ci.nsIContentPolicy.TYPE_OTHER);
> +  } catch(e) {
> +
> +    // fallback to asyncFetch
> +    NetUtil.asyncFetch(dataURI, function (aInputStream, aAsyncFetchResult) {

Please remove the above comment and the blank line.

Comment 15

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1121393_api_changes.patch

Comment 16

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1121393_api_changes.patch

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

While checking the last patch I observing the NetUtils code I wonder why asyncFetch2 is marked as deprecated: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#161

Chris that is that about? Was that missed to be updated?

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8571205 [details] [diff] [review]
bug1121393_api_changes.patch

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

Code-wise all looks fine here. Please fix the remaining nits and we might get it landed soon. I only want to wait for the answer of my question to Chris.

::: mozmill/mozmill/extension/resource/stdlib/securable-module.js
@@ +314,5 @@
>         else
>           baseURI = ios.newURI(base, null, null);
>         var newURI = ios.newURI(path, null, baseURI);
> +       // new interface newChannelFromURI2 in place of
> +       // newChannelFromURI - Bug 1121393

For references we prefer "Bug XYZ - description". Sorry that I missed to mention it before.

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +446,4 @@
>      ready = true;
>    }
>  
> +  function aCallback(aInputStream, aAsyncFetchResult) {

The a prefix is only for parameters. It should not be part of the function name. Also the name needs a tweak so it's understandable what this method does. Maybe you can simply call it `completed`.

Comment 19

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Henrik Skupin (:whimboo) from comment #17)
> While checking the last patch I observing the NetUtils code I wonder why
> asyncFetch2 is marked as deprecated:
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#161
> 
> Chris that is that about? Was that missed to be updated?

Thanks for reaching out - that is a good point. Indeed asyncFetch2 and newChannel2 in Netutil.jsm are deprecated. Unfortunately we have uploaded all the patches for review before the deprecation happened. *But* we are foremost interested in the right arguments to newChannel2 (e.g. passing the right loadingPrincipal is essential because our security checks are going to depend on that information soon). Once we have all the callsites updated to pass the right arguments (in other words once bug 1087720 is fixed), we are going to do a textual replacement and are going to use Netutil.newChannel() with this signature:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#287

* What does that mean for mozmill?
Since we do not have direct control over mozmill we can not land a simple textual replacement on mc right away, right? So I think the better choice for this bug would be to use this newChannel-API (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#287) right away so we don't have to bother you folks again to update it. Does that make sense?

At the moment only those tests (which might be helpful for updating your patch) are using the API that we should end up using:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_NetUtil.js

Comment 20

[Christoph Kerschbaumer [:ckerschb}]

Henrik, Ujjwal,

Is there any progress on this bug? Just a headsup, we have converted all the callsites that create a new channel to use the new API and early in the next cycle (beginning of April) we are going to land a channelwrapper (bug 1120487) that wraps channels that do not call the new API which might very likely break mozmill [1,2].

I can help and finish this bug up if you want me to!


[1] http://mxr.mozilla.org/mozilla-central/search?string=.newChannel%28
[2] http://mxr.mozilla.org/mozilla-central/search?string=.newChannelFromURI%28&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Christoph, I was driven away the last days to some more important work. I didn't wanted to reply here before I understand all the changes you are doing. But sadly I haven't found the time to actually have even a tiny look at it. So if possible please continue with the patch, or give detailed instructions what to do. I assume Ujjwal shouldn't have problems with it either. Thanks.

Comment 22

[Ujjwal Wahi [:ujjwal]]

Christoph,
Sorry I missed update of your comment #19, my mistake. Yes, if you can provide some more information, I will be able to continue my work on this bug.

Comment 23

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Ujjwal Wahi [:ujjwal] from comment #22)
> Christoph,
> Sorry I missed update of your comment #19, my mistake. Yes, if you can
> provide some more information, I will be able to continue my work on this
> bug.

That's great - please find a description of all the arguments here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/NetUtil.jsm#287

and also you might find these tests useful which might you help get used to the API:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_NetUtil.js#301

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Christoph, I assume all this is backward compatible down to at least ESR31?

Comment 25

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Henrik Skupin (:whimboo) [away 03/25 - 03/31] from comment #24)
> Christoph, I assume all this is backward compatible down to at least ESR31?

Yes, it is. Did any progress happen here? We are getting closer to the merge day. Once in the new cycle, we are going to land our channelwrapper which will break mozmill.

ujjwal: Do you have time to finish up the patch?

Comment 26

[Ujjwal Wahi [:ujjwal]]

Attachment: bug1121393_api_changes.patch

Christoph, Please have a look at my code.

Comment 27

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8584719 [details] [diff] [review]
bug1121393_api_changes.patch

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

Ujjwal, please address my feedback and re-flag for review. Please also make sure it works correctly. E.g. keep in mind that ios.newChannel() has a different function signature than NetUtil.newChannel().

::: mozmill/mozmill/extension/resource/stdlib/securable-module.js
@@ +325,5 @@
> +                                          Ci.nsILoadInfo.SEC_NORMAL,
> +                                          Ci.nsIContentPolicy.TYPE_OTHER);
> +       } catch (e) {
> +         channel = ios.newChannelFromURI(newURI);
> +       }

I think you can remove the try/catch and have something like this:
channel = ios.NewChannelFromURI2 ?
            ios.newChannelFromURI2(newURI,
                                   null, // aLoadingNode
                                   Services.scriptSecurityManager.getSystemPrincipal(),
                                   null, // aTriggeringPrincipal
                                   Ci.nsILoadInfo.SEC_NORMAL,
                                   Ci.nsIContentPolicy.TYPE_OTHER) :
            ios.newChannelFromURI(newURI);

@@ +342,5 @@
> +         uri: path,
> +         loadingPrincipal: Services.scriptSecurityManager.getSystemPrincipal()
> +         securityFlags: Ci.nsILoadInfo.SEC_NORMAL,
> +         contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
> +       });

I think you can remove the comment and simplify as:
channel = ios.NewChannel2 ?
            ios.newChannel2(newURI,
                            null, // aCharset
                            null, // aBaseURI
                            null, // aLoadingNode
                            Services.scriptSecurityManager.getSystemPrincipal(),
                            null, // aTriggeringPrincipal
                            Ci.nsILoadInfo.SEC_NORMAL,
                            Ci.nsIContentPolicy.TYPE_OTHER) :
            ios.newChannel(path, null, null);

::: mozmill/mozmill/extension/resource/stdlib/utils.js
@@ +451,5 @@
> +    uri: dataURI,
> +    loadingPrincipal: Services.scriptSecurityManager.getSystemPrincipal()
> +    securityFlags: Ci.nsILoadInfo.SEC_NORMAL,
> +    contentPolicyType: Ci.nsIContentPolicy.TYPE_OTHER
> +  }, function (aInputStream, aAsyncFetchResult) {

NetUtil.asyncFetch2(dataUri, function(aInputStream, AsyncFetchResult) {
    ...
  },
  null, // aLoadingNode
  Services.scriptSecurityManager.getSystemPrincipal(),
  Ci.nsILoadInfo.SEC_NORMAL,
  Ci.nsIContentPolicy.TYPE_OTHER);

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill is plainly dead and tests got replaced with Marionette tests. As long as those changes do not really break the extension in one of the upcoming releases of Firefox, we are not going to fix it. Closing bug as incomplete for now.