1166948 - Update newChannel to newChannel2 providing the right security arguments in Gaia

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Christoph Kerschbaumer [:ckerschb}]

Attachment: gaia.diff

When browsing through the code I realized that Gaia is still using the deprecated newChannel-API [1] (see also Bug 1162657). If I am not mistaken those assertions do not get triggered because TBPL runs only 'optimized' Gaia unit tests and not in debug mode. Either way, we should get that fixed.

I am providing a rudimentary patch but don't have the time to push for that till it's landed. I am happy to provide feedback though.

Alexandre, can you find an owner for that by any chance?

[1] http://mxr.mozilla.org/gaia/search?string=newChannel%28

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Sorry, forgot to actually set the needinfo flag - here we go.

Comment 2

[Christoph Kerschbaumer [:ckerschb}]

Inline the other missing update here. Please note, it's implementing nsIAboutModule, which does not have a newChannel2(), but a newChannel() extended by a loadInfo argument.

> --- a/tools/extensions/[e-mail]/bootstrap.js
> +++ b/tools/extensions/[e-mail]/bootstrap.js
> @@ -21,9 +21,9 @@ AboutGaia.prototype = {
>      return Ci.nsIAboutModule.ALLOW_SCRIPT;
>    },
>  
> -  newChannel: function(aURI) {
> +  newChannel: function(aURI, aLoadInfo) {
>      let uri = 'chrome://gaia-build/content/aboutGaia.html';
> -    let channel = Services.io.newChannel(uri, null, null);
> +    let channel = Services.io.newChannelFromURIWithLoadInfo(uri, aLoadInfo);
>      channel.originalURI = aURI;
>      return channel;
>    }

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 8608366 [details] [diff] [review]
gaia.diff

Looks good to me.

Ricky, could you test this patch and land it if that works?

The gaia-build addon is deprecated so there isn't much value to modify it.
May be we could remove it from repo?

Comment 4

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8608366 [details] [diff] [review]
gaia.diff

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

LGTM too. Please make sure Gaia build integration test (Gb) is green before landing in master.

Comment 5

[Ricky Chien [:rickychien] (inactive)]

(In reply to Alexandre Poirot [:ochameau] from comment #3)
> The gaia-build addon is deprecated so there isn't much value to modify it.
> May be we could remove it from repo?

I'm not really sure what's gaia build addon. Are you pointing to gaia/build/additional-extension.js?

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Ricky Chien [:rickychien] from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #3)
> > The gaia-build addon is deprecated so there isn't much value to modify it.
> > May be we could remove it from repo?
> 
> I'm not really sure what's gaia build addon. Are you pointing to
> gaia/build/additional-extension.js?

Alexandre, I don't know the answer do that question, do you happen to know?

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Ricky Chien [:rickychien] from comment #5)
> (In reply to Alexandre Poirot [:ochameau] from comment #3)
> > The gaia-build addon is deprecated so there isn't much value to modify it.
> > May be we could remove it from repo?
> 
> I'm not really sure what's gaia build addon. Are you pointing to
> gaia/build/additional-extension.js?

No, I'm talking about tools/extensions/[e-mail],
this is the prototype Yuren did to be able to build a gaia profile from a Firefox addon.
I don't think it works as-is anymore and would require a bunch of work to make it work again.

Comment 8

[Alexandre Poirot [:ochameau]]

So Christoph, it's not important to update anything in tools/extensions/[e-mail], we could just drop this folder.
Your current attached patch correctly refactor the code that is still in production.

As suggested by Ricky, you just need to open a pull request on github or push to try with gaia tests.

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Attachment: newchannel2_gaia.diff

Locally verified that Gaia unit test pass! Carrying over r+! Even though code within
> tools/extensions/[e-mail]
is not in production anymore, I am going to converting newChannel to newChannel2 - if not used anymore, someone can go ahead and remove that code completely. For the sake of completeness I am going to update that code for now!

Comment 10

[Christoph Kerschbaumer [:ckerschb}]

Created pull request on github:
https://github.com/mozilla-b2g/gaia/pull/30270

Comment 11

[Gregor Wagner [:gwagner]]

Can you help out a fellow austrian?

Comment 12

[Gregor Wagner [:gwagner]]

Meh I should have added that he needs to check this in.

Comment 13

[Kevin Grandon :kgrandon]

Hi Christoph,

Thanks for the patch! First of all we'll need to rename the commit to include the bug number. I recommend the following commit message: 

Bug 1166948 - Convert newChannel to newChannel2 r=ochameau


You can rename the commit using git rebase, or git commit --amend.

Please let me know when this is done, or if you have trouble and would like me to rename the commit I wouldn't mind doing that before landing.

Comment 14

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Hi Christoph,
> 
> Thanks for the patch! First of all we'll need to rename the commit to
> include the bug number. I recommend the following commit message: 
> 
> Bug 1166948 - Convert newChannel to newChannel2 r=ochameau
> 
> 
> You can rename the commit using git rebase, or git commit --amend.
> 
> Please let me know when this is done, or if you have trouble and would like
> me to rename the commit I wouldn't mind doing that before landing.

If you don't mind, it would be awesome if you could do that for me. Thank you!

Comment 15

[Kevin Grandon :kgrandon]

Turns out this pull request is throwing a JSHint error:

build/download-manager.js: line 105, col 94, Line is too long. (ERROR) 

Could you please update the code then flag me to get this landed?

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Kevin Grandon :kgrandon from comment #15)
> Turns out this pull request is throwing a JSHint error:
> 
> build/download-manager.js: line 105, col 94, Line is too long. (ERROR) 
> 
> Could you please update the code then flag me to get this landed?

Hey Kevin, I fixed the JSHint error and also updated the commit message, but I guess now the commits need to be squashed together into one? Any chance you can take it from here? Thanks so much!

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] KevinGrandon:ckerschb-newchannel2_gaia > mozilla-b2g:master

Comment 18

[Kevin Grandon :kgrandon]

In master: https://github.com/mozilla-b2g/gaia/commit/4499472aad2e6c50ee6e1d6a54e3f84ca169e3a9