Update newChannel to newChannel2 providing the right security arguments in Gaia

RESOLVED FIXED

Status

Firefox OS
Gaia
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

unspecified
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(b2g-master fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8608366 [details] [diff] [review]
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
(Assignee)

Updated

3 years ago
Blocks: 1162657
(Assignee)

Comment 1

3 years ago
Sorry, forgot to actually set the needinfo flag - here we go.
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 2

3 years ago
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/gaia-build@gaiamobile.org/bootstrap.js
> +++ b/tools/extensions/gaia-build@gaiamobile.org/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 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?
Flags: needinfo?(poirot.alex)
Attachment #8608366 - Flags: review?(ricky060709)
Attachment #8608366 - Flags: feedback+
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.
Attachment #8608366 - Flags: review?(ricky060709) → review+
(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?
(Assignee)

Comment 6

3 years ago
(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?
Flags: needinfo?(poirot.alex)
(Assignee)

Updated

3 years ago
See Also: → bug 1120487
(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/gaia-build@gaiamobile.org,
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.
Flags: needinfo?(poirot.alex)
So Christoph, it's not important to update anything in tools/extensions/gaia-build@gaiamobile.org, 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.
(Assignee)

Updated

3 years ago
Assignee: nobody → mozilla
(Assignee)

Comment 9

3 years ago
Created attachment 8611542 [details] [diff] [review]
newchannel2_gaia.diff

Locally verified that Gaia unit test pass! Carrying over r+! Even though code within
> tools/extensions/gaia-build@gaiamobile.org
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!
Attachment #8608366 - Attachment is obsolete: true
Attachment #8611542 - Flags: review+
(Assignee)

Comment 10

3 years ago
Created pull request on github:
https://github.com/mozilla-b2g/gaia/pull/30270
Can you help out a fellow austrian?
Flags: needinfo?(kgrandon)
Meh I should have added that he needs to check this in.
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.
Flags: needinfo?(kgrandon) → needinfo?(mozilla)
(Assignee)

Comment 14

3 years ago
(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!
Flags: needinfo?(mozilla)
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?
Flags: needinfo?(mozilla)
(Assignee)

Comment 16

3 years ago
(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!
Flags: needinfo?(mozilla) → needinfo?(kgrandon)
Created attachment 8612248 [details] [review]
[gaia] KevinGrandon:ckerschb-newchannel2_gaia > mozilla-b2g:master
In master: https://github.com/mozilla-b2g/gaia/commit/4499472aad2e6c50ee6e1d6a54e3f84ca169e3a9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-b2g-master: --- → fixed
Flags: needinfo?(kgrandon)
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Blocks: 1120487
You need to log in before you can comment on or make changes to this bug.