975857 - Rewrite inline script/style in /browser/base/content/browser.xul

Status: NEW

(Firefox :: General, defect)

Description

[desiradaniel2007]

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

[Projjol [:dorsatum]]

Attachment: browser.patch

I'd like to work on this bug and for that, I've attached a proposed patch.

Comment 2

[Frederik Braun [:freddy]]

Comment on attachment 8432070 [details] [diff] [review]
browser.patch

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

Nice first stab. See the comments below and let's find somebody to help us with where to put all those addEventListener calls.

::: browser/base/content/browser.js
@@ +226,5 @@
>    Cu.import("resource://gre/modules/PageMenu.jsm", tmp);
>    return new tmp.PageMenu();
>  });
>  
> +let tabContextMenu = document.getElementById("tabContextMenu");

This whole code should be wrapped into something that runs after the DOM Content has been loaded. I haven't quite figured out if gBrowserInit.onLoad (line 751) is the correct place and we should probably ask a module peer! :)

@@ +229,5 @@
>  
> +let tabContextMenu = document.getElementById("tabContextMenu");
> +
> +tabContextMenu.addEventListener("popupshowing",function(event){
> +  if (event.target == this) TabContextMenu.updateContextMenu(this)

The value of "this" changes when you move the function from HTML to JS. You either need to bind your newly created function to the tabContextMenu by using something like '(function(event) { ...}).bind(tabContextMenu)' instead of your existing declaration. You could also just rewrite the function to check if 'event.target == tabContextMenu', which seems more readable to me.

@@ +1660,5 @@
> +    });
> +    homeButton.addEventListener("dragexit", function(event){
> +      homeButtonObserver.onDragExit(event)
> +    });
> +    homeButton.addEventListener("click", function(event){

These placements (back, forward and home button) look quite alright!

::: browser/base/content/browser.xul
@@ +370,5 @@
>      </panel>
>  
>      <!-- Sync Error Panel -->
>      <panel id="sync-error-panel" class="sync-panel" type="arrow" hidden="true"
> +           noautofocus="true" 

You have some trailing whitespaces here and in other places across your patched files.
Please remove them and consider setting your editor to remove them or display them automatically :)

Comment 3

[Frederik Braun [:freddy]]

Justin, can you help us find a meaningful place for all those addEventListener calls?

Comment 4

[Justin Dolske [:Dolske]]

I think invoking this from in gBrowserInit._delayedStartup() would be best.

I worry a tiny bit about this patch making it so that some things don't work until that point. (e.g., if startup on a system takes many seconds, during that time something that used to respond to clicks now won't.) But I don't think that's likely to be a problem, and we already have a lot of that kind of thing because onLoad() and _delayedStartup() do a lot of initialization themselves. :)

Could instead do this a wee bit earlier in onLoad(), but I suspect that would cause a a Talos startup regression, due to all the extra getElementById() work. Using _delayedStartup() should avoid that, and I think it's actually quite suitable because none of these listeners are strictly required for getting the window up, they're only needed to respond to future user interaction with the window. [Well, I don't know offhand if the urlbar is being blurred/focused as part of bringing up the window, so it would be worth making this patch doesn't regress that.]


Two driveby review comments:

* Please put this kind of stuff into its own separate function, since (1) it's rather long and (2) all the temporary getElementById vars are polluting the global namespace... gBrowserInit.setupEventListeners() or something like that.

* I think it would significantly help readability to avoid all the temporary assignments. I'd suggest a helper function, like this:

...
  setupEventListeners: function() {
    function foo(elementID, eventName, func) {
      let node = document.getElementById(elementID);
      if (!node) { explode(); } // free rhyme
      node.addEventListener(eventName, func);
    }

    foo("tabContextMenu", "popupshowing", function (event) { ... });
    foo("tabContextMenu", "popuphidden",  function (event) { ... });
    foo("context_reloadTab", "command", function (event) { ... });
    foo("context_pinTab", "command", function (event) { ... });
  },
...

Bonus points for using arrow functions!

    foo("context_pinTab", "command", event => { ... });

Comment 5

[Projjol [:dorsatum]]

Thank you both for your reviews, I'll incorporate them and submit a patch asap.

Comment 6

[Projjol [:dorsatum]]

Attachment: browser.patch

Added them under gBrowserInit._delayedStartup() and created a function for the eventlisteners.

Comment 7

[Frederik Braun [:freddy]]

Comment on attachment 8440562 [details] [diff] [review]
browser.patch

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

Please set up a test environment and try to see if your changes break something, before you submit another patch.
Setting up takes a while and the first compile will be long, but after that, builds should go really quick (less than 5 minutes!) since you're only changing XUL and JS files!
There are a lot of useful resources on MDN, which I recommend you take a look at. E.g., https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

::: browser/base/content/browser.js
@@ +352,5 @@
>    forwardButton.setAttribute("type", "menu");
>    forwardButton.appendChild(popup);
> +  forwardButton.addEventListener("click", function(event){
> +    checkForMiddleClick(this, event)
> +  });

The event listeners for the forward and backward button need to go into the setupEventListener function.

@@ +972,5 @@
> +      function setup(elementID, eventName, func_obj){
> +        let node = document.getElementById(elementID);
> +        node.addEventListener(eventName, func_obj);
> +  },
> +

Sorry, but this is not correct style and indentation.
If you start to patch important things like browser.js, you need to write code *and test it* before you ask for a review.

This function is also never called.

@@ +1349,5 @@
> +    });
> +    setup("sync-button","command",event=>{
> +      gSyncUI.handleToolbarButton()
> +    });
> +    setup("tabs-closebutton close-icon","command",event=>{

"tabs-closebutton close-icon" does not look like a valid ID to me ;-)

@@ +1368,5 @@
> +    setup("sync-start-panel","click",event=>{
> +      this.hidePopup()
> +    });
> +  }
> +}

This big list of live in the setupEventListeners function as explained in comment 4.
Please also add some spaces for readability, e.g.: |setup("foo", "bar", event => {|

@@ +1440,5 @@
> +      homeButtonObserver.onDragExit(event)
> +    });
> +    homeButton.addEventListener("click", function(event){
> +      BrowserGoHome(event)
> +    });

The homeButton event listeners need to go into the setupEventListener function too!

Comment 8

[Projjol [:dorsatum]]

(In reply to Frederik Braun [:freddyb] from comment #7)
 
> @@ +1349,5 @@
> > +    });
> > +    setup("sync-button","command",event=>{
> > +      gSyncUI.handleToolbarButton()
> > +    });
> > +    setup("tabs-closebutton close-icon","command",event=>{
> 
> "tabs-closebutton close-icon" does not look like a valid ID to me ;-)

Hi, sorry for taking such a long time to reply. I've edited the file with the rest of the changes that you'd mentioned, but for this particular error of mine, I have a query. The error I made corresponds to this line: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1126

I'm not sure about the id that should be listened for in this case, could you help me out with that? As soon as this is wrapped up, I'll be able to submit a patch.

Comment 9

[Frederik Braun [:freddy]]

The line you are referring to says |<splitter id="social-sidebar-splitter"|.

What's wrong with this id?

Comment 10

[Projjol [:dorsatum]]

Pardon me, I inadvertently added the wrong line. The correct one is: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#1110

It's here that my confusion lies, should I be considering the sidebarheader id, in this case?

Sorry for the mix-up in line numbers.

Comment 11

[Frederik Braun [:freddy]]

I suggest you add an id for this element in the XUL file. Something like sidebar-close-button maybe?

Comment 12

[Projjol [:dorsatum]]

Attachment: browser.patch

Marked the previous one obsolete for this contains a number of fixes previously absent. Also, this is with respect to the updated m-c tree.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8460925 [details] [diff] [review]
browser.patch

Patch hadn't r? set and is 2.5 weeks old.

Comment 14

[Frederik Braun [:freddy]]

Comment on attachment 8460925 [details] [diff] [review]
browser.patch

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

Sorry about the delay. Can you re-generate the patch for a more recent revision? It doesn't apply cleanly anymore for browser.xul.

::: browser/base/content/browser.js
@@ +1068,5 @@
>      this._boundDelayedStartup = null;
>    },
>  
> +  setupEventListeners: function () {
> +    dump("setting up event listeners..\n");

Please remove the dump statements, once you're done testing locally.

@@ +1500,5 @@
>        }
>      }
>  
> +
> +

Please remove the newlines you introduced here.

Comment 15

[Projjol [:dorsatum]]

Attachment: browser.patch

I'm sorry for the delay. Removed dump statements and additional whitespaces. Upon building with the patch, the build went off fine.

Comment 16

[Frederik Braun [:freddy]]

I have problems building & running. Testing this is blocked by bug 1060277.
Sorry if the test may take a bit longer than usual.

Comment 17

[Frederik Braun [:freddy]]

Comment on attachment 8480513 [details] [diff] [review]
browser.patch

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

This is going in the right direction, but despite the thing building. I would liked you to do some more testing. Use "mach run" to do this.
Once you're confident that everything works (I went over most of the buttons in the UI), please resubmit for feedback. I think we may then reach the point where we run through the full test suite and see what fails. Keep it up! :)


My first tests made the browser so unusable that the error console could not be started.
You can fix this by observing which command "mach run" tells you it's going to run to start Firefox with a temporary profile. Just append "-jsconsole" to make the console open on startup.

I found these issues drive-by:

::: browser/base/content/browser.js
@@ +1005,5 @@
>  
> +  setupEventListeners: function () {
> +    function setup(elementID, eventName, func_obj) {
> +      let node = document.getElementById(elementID);
> +      node.addEventListener(eventName, func_obj);

Starting Firefox gave me a *lot* of "node is null". We can eliminate a lot of them by putting the setupEventListeners call to the end of delayedStartup. But there is no guarantee that all the DOM nodes are indeed set up. Needs additional testing.

@@ +1013,5 @@
> +      if (event.target == tabContextMenu)
> +        TabContextMenu.updateContextMenu(tabContextMenu)
> +    });
> +    setup("tabContextMenu", "popuphidden", event=> {
> +      if (event.target == tabContextMenu)

tabContextMenu is not defined, same as in line 1013. As a work-around you could use document.getElementById("tabContextMenu") instead. But wait... there's little point to do this check after all?!

@@ +1083,5 @@
> +    setup("social-flyout-panel", "popuphidden", (event=> {
> +      SocialFlyout.onHidden()
> +    }).bind(this));
> +    setup("toolbar-context-menu", "popupshowing", (event=> {
> +      onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator')

The closing parenthesis for calling onViewToolbarsPopupShowing does not close. Insert after the closing for document.getElementById:
> onViewToolbarsPopupShowing(event, document.getElementById('viewToolbarsMenuSeparator'))

@@ +1273,5 @@
> +    setup("downloads-button", "drop", (event=> {
> +      DownloadsIndicatorView.onDrop(event)
> +    }).bind(this));
> +    setup("downloads-button", "dragover", (event=> {
> +      DownloadsIndicatorView.onDragOver(event

Missing closing paren. Should be:
> DownloadsIndicatorView.onDragOver(event)

Comment 18

[Projjol [:dorsatum]]

Attachment: browser.patch

Missing parenthesis handled, number of 'node is null' warning reduced and UI buttons worked upon testing.

Comment 19

[Frederik Braun [:freddy]]

Comment on attachment 8500375 [details] [diff] [review]
browser.patch

Too much bitrot. Sorry.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.