948899 - Move inline scripts and styles into separate file for mobile/android/chrome/content/aboutAddons.xhtml (URL=about:addons)

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Frederik Braun [:freddy]]

With the current plan to harden the security of Firefox, we want to disallow internal privileged pages to use inline JavaScript. Since their amount is fairly limited, we start this by rewriting them bit by bit.

Comment 1

[Errietta Kostala [:errietta]]

Hi can I take this please?

Comment 2

[Frederik Braun [:freddy]]

Sure!

Comment 3

[Errietta Kostala [:errietta]]

Attachment: Work so far

Hi,
As seen in the attachment I added event listeners in aboutAddons.js and removed them from the XML file, however some functions don't work anymore.
For example, clicking on the disable button gives me E/GeckoConsole(19707): [JavaScript Error: "TypeError: this.setEnabled is not a function" {file: "chrome://browser/content/aboutAddons.js" line: 440}]. Any pointers?

Comment 4

[Frederik Braun [:freddy]]

Comment on attachment 8349392 [details] [diff] [review]
Work so far

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

I must admit that I don't really understand the underlying problem yet. I will try to find some time to build and test this tomorrow.

Thanks for this first patch, the general approach looks really good!
In the meantime, you could address the inline CSS.
Do you intend to address the inline styles in a separate patch?

Comment 5

[Errietta Kostala [:errietta]]

Hi, I found the problem with this I think. I'm working on the CSS now and then I'll provide a patch with both :)

Comment 6

[Errietta Kostala [:errietta]]

Attachment: Proposed patch

How's this?

Comment 7

[:Margaret Leibovic]

Comment on attachment 8349442 [details] [diff] [review]
Proposed patch

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

::: mobile/android/chrome/content/aboutAddons.js
@@ +85,5 @@
>    Addons.getAddons();
>    showList();
>    ContextMenus.init();
> +
> +  document.getElementById('uninstall-btn').addEventListener('click', function() { Addons.uninstall(); }, false);

Drive-by comment: It looks like your previous problem was caused by the callback function getting called with the wrong `this` context, so that's why wrapping the callback in a function like this fixed the issue.

However, I think a better solution would be to do Addons.uninstall.bind(this);

Also, in general we use double quote ("") instead of single quotes ('') to wrap strings, so we should do that in here to be consistent.

@@ +98,5 @@
> +  var header_button = document.getElementById('header-button');
> +  header_button.addEventListener(
> +    'click',
> +    function() {
> +      openLink (header_button);

freddyb ran into a similar issue in bug 948894. Instead of needing to wrap the openLink call in an anonymous function, we should try updating the openLink function to take an event instead of an element.

Comment 8

[Frederik Braun [:freddy]]

(In reply to :Margaret Leibovic from comment #7)
> @@ +98,5 @@
> > +  var header_button = document.getElementById('header-button');
> > +  header_button.addEventListener(
> > +    'click',
> > +    function() {
> > +      openLink (header_button);
> 
> freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> the openLink call in an anonymous function, we should try updating the
> openLink function to take an event instead of an element.

AFAIU there are other calls of openLink which are not bound to an Event Listener, so you have to make sure that they are accommodated as well.

Comment 9

[Frederik Braun [:freddy]]

Comment on attachment 8349442 [details] [diff] [review]
Proposed patch

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

Good progress!

::: mobile/android/themes/core/aboutAddons.css
@@ +269,5 @@
> +}
> +
> +#addons-list, #addons-details {
> +  display: none;
> +}

You select a lot of tags to be either 'display: none;' or 'display: -moz-box;'. I suggest you create another class for those two cases and add it to all the affected tags isntead. What do you think?

Comment 10

[Errietta Kostala [:errietta]]

(In reply to :Margaret Leibovic from comment #7)
> @@ +98,5 @@
> > +  var header_button = document.getElementById('header-button');
> > +  header_button.addEventListener(
> > +    'click',
> > +    function() {
> > +      openLink (header_button);
> 
> freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> the openLink call in an anonymous function, we should try updating the
> openLink function to take an event instead of an element.

There's

>   outer.addEventListener("click", function() {
>      openLink(document.getElementById("header-button"));
>   }.bind(this), true);

which calls openLink on another element than the event target, so then how would we accommodate for that?


(In reply to Frederik Braun [:freddyb] from comment #9)
> Comment on attachment 8349442 [details] [diff] [review]
> Proposed patch
> 
> Review of attachment 8349442 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good progress!
> 
> ::: mobile/android/themes/core/aboutAddons.css
> @@ +269,5 @@
> > +}
> > +
> > +#addons-list, #addons-details {
> > +  display: none;
> > +}
> 
> You select a lot of tags to be either 'display: none;' or 'display:
> -moz-box;'. I suggest you create another class for those two cases and add
> it to all the affected tags isntead. What do you think?

I copied a lot of that from the original XML. I assume that show-on* and hide-on* classes are applied on what the user does so getting rid of that might break something?

Comment 11

[Frederik Braun [:freddy]]

(In reply to Errietta Kostala from comment #10)
> There's
> 
> >   outer.addEventListener("click", function() {
> >      openLink(document.getElementById("header-button"));
> >   }.bind(this), true);
> 
> which calls openLink on another element than the event target, so then how
> would we accommodate for that?
> 
So, we either keep the function-wrapped instance you have already added or you check in openLink if it has gotten an Element or an Event. You could find out if it's an Element with |if varName instnaceof Node|, you should rename the paremeter to aEventOrElement or so... which looks a bit dirty to me. Maybe we stick with the wrapped function, but frankly I don't know what's preferred in our codebase. 

Margaret, what do you think?


> > You select a lot of tags to be either 'display: none;' or 'display:
> > -moz-box;'. I suggest you create another class for those two cases and add
> > it to all the affected tags isntead. What do you think?
> 
> I copied a lot of that from the original XML. I assume that show-on* and
> hide-on* classes are applied on what the user does so getting rid of that
> might break something?


Yes, don't remove any of the existing classes. You can have more than one class per Element, i.e. |<span class="foo bar">|

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Errietta Kostala from comment #10)

> > freddyb ran into a similar issue in bug 948894. Instead of needing to wrap
> > the openLink call in an anonymous function, we should try updating the
> > openLink function to take an event instead of an element.
> 
> There's
> 
> >   outer.addEventListener("click", function() {
> >      openLink(document.getElementById("header-button"));
> >   }.bind(this), true);
> 
> which calls openLink on another element than the event target, so then how
> would we accommodate for that?

All methods that are used as event handlers (call in response to an event) are passed an event object, whether you ask for it or not. So this could work:

   outer.addEventListener("click", function(aEvent) {
      openLink(aEvent);
   }.bind(this), true);

or even this:

   outer.addEventListener("click", openLink, true);

> (In reply to Frederik Braun [:freddyb] from comment #9)

> > You select a lot of tags to be either 'display: none;' or 'display:
> > -moz-box;'. I suggest you create another class for those two cases and add
> > it to all the affected tags isntead. What do you think?
> 
> I copied a lot of that from the original XML. I assume that show-on* and
> hide-on* classes are applied on what the user does so getting rid of that
> might break something?

I agree with you. Making less changes is better. We typically file new bugs if we find some code that could be refactored, but is out of scope in the current bug.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Examples of the openLink change:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#33
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutApps.js#91

Comment 14

[Errietta Kostala [:errietta]]

Attachment: Work so far

Hi,

Here's what I have so far. However, doing document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(Addons), false); breaks it with the following error: E/GeckoConsole( 4994): [JavaScript Error: "TypeError: aKey is undefined" {file: "chrome://browser/content/aboutAddons.js" line: 252}]

I think the problem is that   uninstall: function uninstall(aAddon) {
is getting the click event instead of a valid addon object so addon.id isn't defined

Comment 15

[Errietta Kostala [:errietta]]

Attachment: proposed patch

Hi,

I was told here and on IRC to do   document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(this), false); or   document.getElementById("uninstall-btn").addEventListener("click", Addons.uninstall.bind(Addons), false);

but neither passes the right sort of thing onto Addons.uninstall and friends. I'm doing   document.getElementById("uninstall-btn").addEventListener("click", function() { Addons.uninstall() }, false); for now instead, since that works fine. Is that ok or how do we figure out how to make .bind work?

Comment 16

[Frederik Braun [:freddy]]

Comment on attachment 8356569 [details] [diff] [review]
proposed patch

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

Mh, this is confusing to me too. It seems this should be bound to the element, you're attaching the eventlistener to?

Comment 17

[Errietta Kostala [:errietta]]

When we do Addons.uninstall.bind(Addons), it passes the event object as an argument. .uninstall() expects either nothing, or an addon as an argument, so that's not going to work. The code I replaced called it with no argument at all.

Comment 18

[:Margaret Leibovic]

function() { Addons.uninstall() }

...is equivalent to...

Addons.uninstall.bind(this);

So, if you want to use bind, you should be binding the current context, not the Addons context.

Also, please make sure to flag me or another mobile peer for review when you have a patch you'd like to land.

Comment 19

[Errietta Kostala [:errietta]]

Attachment: proposed patch

Comment 20

[:Margaret Leibovic]

Comment on attachment 8357999 [details] [diff] [review]
proposed patch

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

::: mobile/android/chrome/content/aboutAddons.js
@@ +92,5 @@
> +
> +  document.getElementById("uninstall-btn").addEventListener("click", function() { Addons.uninstall() }, false);
> +  document.getElementById("cancel-btn").addEventListener("click", function() { Addons.cancelUninstall() }, false);
> +  document.getElementById("disable-btn").addEventListener("click", function() { Addons.disable() }, false);
> +  document.getElementById("enable-btn").addEventListener("click", function() { Addons.enable() }, false);

Thinking about this more, instead of adding these event listeners here, we could create a Addons.init method similar to ContextMenus.init, then doing Addons.enable.bind(this) from within Addons.init should end up binding the correct context.

In fact, you could just rename Addons.getAddons() to Addons.init(), and add these event listeners in there.

I don't have a strong preference whether we try that or just land this code as-is, so it's up to you if you want to try this and upload a new patch.

@@ +531,5 @@
>    }
>  }
> +
> +window.addEventListener('load', init, false);
> +window.addEventListener('unload', uninit, false);

Nit: Use double quotes instead of single quotes here.

Comment 21

[Errietta Kostala [:errietta]]

Attachment: Proposed patch

Hi,

I think this is better now - I broke uninstall in 2 functions, 1 for the current addon and one for passing an addon as a param and it seems to work fine.

Comment 22

[:Margaret Leibovic]

Comment on attachment 8358345 [details] [diff] [review]
Proposed patch

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

Nice improvement! Thanks for putting the extra work into this :)

::: mobile/android/chrome/content/aboutAddons.js
@@ +440,4 @@
>      if (!addon)
>        return;
>  
> +    this.uninstall (addon);

Nit: Remove the space before the (.

Comment 23

[Errietta Kostala [:errietta]]

Attachment: patch

I removed that now =]

Comment 24

[:Margaret Leibovic]

Comment on attachment 8359301 [details] [diff] [review]
patch

Thanks!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/86d408022528

Comment 26

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

https://hg.mozilla.org/mozilla-central/rev/86d408022528