980331 - Ensure devtools/webconsole/webconsole.xul is free of inline script and styles

Status: RESOLVED WONTFIX

(DevTools :: Console, defect, P3)

Description

[Mark Goodwin [:mgoodwin]]

I see some oncommand, oncommandupdate and onpopupshowing. Also there's some inline script (goUpdateConsoleCommands) and a style attr on an hbox.

Comment 1

[Mark Goodwin [:mgoodwin]]

msucan, I've tagged this as a good first bug (which may have been as you intended in any case); please feel free to change mentor to me if you don't want to do the first pass of feedback.

Comment 2

[Mihai Sucan [:msucan]]

I've tagged a few bugs as 'mentored', which are not necessarily good first bugs - they need a sufficiently experienced contributor.

We can keep this as a good first bug.

Comment 3

[Nadia Rodriguez]

Hi, I would like to work on this bug. It's my first time working on firefox.

Comment 4

[Mark Goodwin [:mgoodwin]]

(In reply to Nadia Rodriguez from comment #3)
> Hi, I would like to work on this bug. It's my first time working on firefox.

Awesome.

You can get some background on what we're doing (and why) here: https://wiki.mozilla.org/Security/Inline_Scripts_and_Styles

If you take a look at bug 960728 you'll see it has a number of blockers that are closed; these will have similarities, to what needs doing here. If, after taking a look at these, you still have questions, just ask away.

Comment 5

[Nadia Rodriguez]

Ok, thanks! I'll look over the info you provided.

Comment 6

[Kushagra Singh [:kushagra]]

Hi mark,
if Nadia is no longer actively working on this bug then i would like to take it up and work on it.

Comment 7

[Nadia Rodriguez]

I am currently still working on this bug. I should have a proposed patch up soon so I can get some feedback.

Comment 8

[Kushagra Singh [:kushagra]]

ah, great!! Best of luck :)

Comment 9

[Nadia Rodriguez]

Attachment: bug980331_inlinescriptfixes.patch for webconsole.xul and webconsole.js

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 8397318 [details] [diff] [review]
bug980331_inlinescriptfixes.patch for webconsole.xul and webconsole.js

Nadia, please ask the mentor for feedback. Set the feedback flag to "?" then write the email address/ID of the mentor.

Mark, giving you the first pass for the patch so you can check if it meets the security requirements. Please also test the patch to see if the webconsole and the browserconsole continue to work - in some cases 'good first bugs' get untested patches that break things badly. Thank you!

I will gladly give a final review pass to the patch when you think it's ready.

Comment 11

[Nadia Rodriguez]

Mihai, thanks for pointing out about the "?" for the feedback flag.

As for my first attempt at the patch...There are a few things that I wanted to ask. Is the placement of the eventhandlers and function in the .js file correct? Also, I know there is a style that needs to be moved into a css file, but there is not one created for the webconsole folder. Should one be created or is there another way that I am not thinking of?

Comment 12

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8397318 [details] [diff] [review]
bug980331_inlinescriptfixes.patch for webconsole.xul and webconsole.js

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

Thanks for this Nadia.

It would be helpful if you could test that your changes work before flagging for review.  You can do this in 2 ways; firstly you can try to use the feature (as a user would) you've made changes to. Secondly (once you're happy stuff seems to work), if it's an existing feature, it's good to run some of the provided tests.  In this case, you could try something like 'mach mochitest-browser browser/devtools/webconsole' to run the webconsole tests.

I notice there's still a style attribute in the xul file (which I see you refer to in Comment 11) - Mihai is best placed to advise you on what to do here.

::: browser/devtools/webconsole/webconsole.js
@@ +570,5 @@
>      let toolbox = gDevTools.getToolbox(this.owner.target);
>      if (toolbox) {
>        toolbox.on("webconsole-selected", this._onPanelSelected);
>      }
>  

Please make an effort to match the coding style and formatting of the rest of the file (here, you've used tabs to indent, the rest of the document uses spaces).

There's a document on MDN on the mozilla coding style. Please read this (especially the section entitled "Naming and formatting code") https://developer.mozilla.org/en/docs/Developer_Guide/Coding_Style

If it's possible to do this, you may find it helpful to set up your editor to highlight tabs (and any whitespace at the end of a line).

@@ +571,5 @@
>      if (toolbox) {
>        toolbox.on("webconsole-selected", this._onPanelSelected);
>      }
>  
> +	let consoleCmd = document.getElementById("consoleCommands");

Do you mean 'doc' here? (see also other instances below).

@@ +597,5 @@
> +	zoomReset.addEventListener("command", function(){goDoCommand('cmd_fontSizeReset')});
> +
> +	let cmdClose = document.getElementById("cmd_close")
> +	cmdClose.addEventListener("command", function(){goDoCommand('cmd_close')});
> +	

There's some unnecessary whitespace on this line

@@ +600,5 @@
> +	cmdClose.addEventListener("command", function(){goDoCommand('cmd_close')});
> +	
> +	let outputContextmenu = document.getElementById("output-contextmenu")
> +	outputContextmenu.addEventListener("popupshowing", goUpdateGlobalEditMenuItems);
> +	

Again, whitespace

::: browser/devtools/webconsole/webconsole.xul
@@ +24,5 @@
>  
>    <script type="application/javascript;version=1.8"
>            src="chrome://browser/content/devtools/theme-switching.js"/>
>    <script type="text/javascript" src="chrome://global/content/globalOverlay.js"/>
> +  

You've removed the element from this line but the indenting spaces are still here; please remove

Comment 13

[Nadia Rodriguez]

Ok, thank you for the help. I will address the issues.

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Nadia Rodriguez from comment #11)
> Mihai, thanks for pointing out about the "?" for the feedback flag.

You're welcome!

> As for my first attempt at the patch...There are a few things that I wanted
> to ask.

> Is the placement of the eventhandlers and function in the .js file
> correct?

They seem fine.

> Also, I know there is a style that needs to be moved into a css
> file, but there is not one created for the webconsole folder. Should one be
> created or is there another way that I am not thinking of?

Edit webconsole.inc.css (use hg locate to find it). You can add direction:ltr to .jsterm-input-container.

Thanks for your patch!

Comment 15

[Mihai Sucan [:msucan]]

Nadia, do you have time to finish this patch? Do you have any questions? If you don't have any spare time, don't worry, but we would like to know so we can allow other contributors to fix this bug. Thank you!

Comment 16

[Mike Hoye [:mhoye]]

Hi, Nadia - You're really close with this patch. It won't take much to carry it over the line. Are you still interested in working on it?

Comment 17

[Nadia Rodriguez]

Sorry that it took me so long to respond. I don't have time to work on this patch right now. I hope another contributor can finish this patch. Thanks

Comment 18

[Mike Hoye [:mhoye]]

Thanks for getting it this far.

Comment 19

[Nachiketh]

Is this bug still open to take up? I am new to mozilla and would like to contribute.

Comment 20

[vikneshwar]

Attachment: webconsole_inline.patch

I made some few changes , haven't tested it thought , just want to make sure whether i am in right track :)

Comment 21

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8478639 [details] [diff] [review]
webconsole_inline.patch

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

Thanks for the updated patch vikneshwar. Try to run the webconsole tests to make sure nothing breaks with this patch.

::: browser/devtools/webconsole/webconsole.js
@@ +574,5 @@
>        toolbox.on("webconsole-selected", this._onPanelSelected);
>      }
>  
> +    let consoleCmd = document.getElementById("consoleCommands");
> +    consoleCmd.addEventListener("command", goUpdateConsoleCommands);

This event should be "commandupdate".

Comment 22

[vikneshwar]

Attachment: webconsole_test.patch

changed command to commandupdate :) however ./mach mochitest-devtools browser/devtools results in multiple failures http://www.pastebin.com/xqUjmTdW

Comment 23

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8485934 [details] [diff] [review]
webconsole_test.patch

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

::: browser/devtools/webconsole/webconsole.js
@@ +573,5 @@
>      if (toolbox) {
>        toolbox.on("webconsole-selected", this._onPanelSelected);
>      }
>  
> +    let consoleCmd = document.getElementById("consoleCommands");

This code does not run as part of a page, so it doesn't have direct access to a document global. You should use the local variable doc as you can see elsewhere in this method.

@@ +577,5 @@
> +    let consoleCmd = document.getElementById("consoleCommands");
> +    consoleCmd.addEventListener("commandupdate", goUpdateConsoleCommands);
> +
> +    let openURL = document.getElementById("consoleCmd_openURL");
> +    openURL.addEventListener("command", function(){goDoCommand('consoleCmd_openURL')});

A better way to write this is to use the new arrow functions from ECMAScript 6:

openURL.addEventListener("command", () => goDoCommand('consoleCmd_openURL'));

@@ +601,5 @@
> +    let cmdClose = document.getElementById("cmd_close")
> +    cmdClose.addEventListener("command", function(){goDoCommand('cmd_close')});
> +
> +    let outputContextmenu = document.getElementById("output-contextmenu")
> +    outputContextmenu.addEventListener("popupshowing", goUpdateGlobalEditMenuItems);

Use a closure (arrow function) here, too.

Comment 24

[vikneshwar]

Attachment: webconsole-edited

I updated as per the above comment , test fails still http://pastebin.com/3LSJ2y7K

Comment 25

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8489497 [details] [diff] [review]
webconsole-edited

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

::: browser/devtools/webconsole/webconsole.js
@@ +608,5 @@
> +    let cmdClose = doc.getElementById("cmd_close")
> +    cmdClose.addEventListener("command", () => goDoCommand('cmd_close'));
> +
> +    let outputContextmenu = doc.getElementById("output-contextmenu")
> +    outputContextmenu.addEventListener("popupshowing", goUpdateGlobalEditMenuItems);

That's because you didn't address my previous comment to use an arrow function here, too.

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 27

[Alexey Novak]

I would love to take care of this bug.

Also a quick question: it seems that /goUpdateCommand()/ and /goUpdateGlobalEditMenuItems()/ are undefined inside devtools/webconsole/webconsole.js
not sure how to access them from there (somehow access globalOverlay and editMenuOverlay files where those functions are defined)

Comment 28

[Alexey Novak]

Sorry forgot to add 'Need more information' flag.

Comment 29

[Panos Astithas (he/him) [:past] (please ni?)]

Thanks for the interest in this bug, please go ahead and update the attached patch based on the previous comments. As you mention, goUpdateCommand and goUpdateGlobalEditMenuItems are defined in files included in webconsole.xul:

https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/webconsole.xul#29

This is why the patch needs to use arrow functions instead of direct references to them, as they will only be available at the point where the event handler is executed, not at _initUI execution time.

Comment 30

[Alexey Novak]

Attachment: Bug980331.patch

Actually my bad. I should've been more descriptive in my comment.

To be honest arrow function was the first thing I tried when I was reading through the conversations for this bug. And even with arrow function there are the same errors.
If I run |./mach test devtools/client/webconsole/|

I would get error in the browser console:
ReferenceError: goUpdateGlobalEditMenuItems is not defined on line 649

And also in the terminal I get:

15 INFO TEST-PASS | devtools/client/webconsole/test/browser_bug_871156_ctrlw_close_tab.js | Browser Console opened - 
JavaScript error: resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js, line 620: ReferenceError: goUpdateCommand is not defined
116 INFO Console message: [JavaScript Error: "ReferenceError: goUpdateCommand is not defined" {file: "resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/webconsole/webconsole.js" line: 620}]


That is why I was trying to understand how those functions are defined and could be used.

Comment 31

[Panos Astithas (he/him) [:past] (please ni?)]

There was a somewhat recent change in the way these handlers are evaluated in the global scope and what kind of environment they are provided. Try using the formulation this.window.goUpdateGlobalEditMenuItems() to call these functions.

Comment 32

[Alexey Novak]

Attachment: Bug980331.patch

Thanks that helped!

I ran all the tests:
./mach test devtools/client/webconsole/

All of them pass except of few ones. Not sure if it was related to my changes or something else since some of the tests seem to freeze or "stuck" with lots of timeouts

I put the result of the execution and attached the patch.
Will try to take a closer look if those issues are related to my changes.



-----

5546 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_bug_871156_ctrlw_close_tab.js | Test timed out - 
5547 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_bug_871156_ctrlw_close_tab.js | Found a devtools:webconsole after previous test timed out - 
5548 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_console_keyboard_accessibility.js | Test timed out - 
5549 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_console_keyboard_accessibility.js | A promise chain failed to handle a rejection:  - at chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_console_keyboard_accessibility.js:62 - TypeError: hud.jsterm is null
Stack trace:
    test<@chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_console_keyboard_accessibility.js:62:3
    asyncTest/<@chrome://mochitests/content/browser/devtools/client/webconsole/test/head.js:47:5
    Tester_execTest@chrome://mochikit/content/browser-test.js:786:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
    timeoutFn@chrome://mochikit/content/browser-test.js:849:9
    setTimeout handler*Tester_execTest@chrome://mochikit/content/browser-test.js:811:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
5550 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_change_font_size.js | input font increased with ctrl+= - Got 10px, expected 11px
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:943
    chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_webconsole_change_font_size.js:test<:31
    asyncTest/<@chrome://mochitests/content/browser/devtools/client/webconsole/test/head.js:47:5
    Tester_execTest@chrome://mochikit/content/browser-test.js:786:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
5551 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_change_font_size.js | font reset with ctrl+0 - Got 10px, expected 
Stack trace:
    chrome://mochikit/content/browser-test.js:test_is:943
    chrome://mochitests/content/browser/devtools/client/webconsole/test/browser_webconsole_change_font_size.js:test<:41
    asyncTest/<@chrome://mochitests/content/browser/devtools/client/webconsole/test/head.js:47:5
    Tester_execTest@chrome://mochikit/content/browser-test.js:786:9
    Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:677:7
    SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:735:59
SUITE-END | took 552s

Comment 33

[Alexey Novak]

Executed tests on the latest fx-team and tests do not fail. Obviously some problem with the change I've made. Will take a look into it.

Comment 34

[Alexey Novak]

Quick question on how to debug tests.

I'm looking into one of the failing tests: browser_webconsole_change_font_size.js
put a debugger; statement on line 24

Then ran
./mach mochitest devtools/client/webconsole/test/browser_webconsole_change_font_size.js --jsdebugger

It does not stop but you can pause the browser setTimeout_timer.jsm on line 29

I want to pause and step in or just see the execution to find out why the key commands did not trigger in the test since those key commands look to work on nightly build. I believe there should be some test config I' missing.

Comment 35

[Panos Astithas (he/him) [:past] (please ni?)]

Adding a debugger statement in line 24 and using --jsdebugger works as expected for me. Execution pauses and I can step through the code. I tried it on OS X, what platform are you on?

Comment 36

[Alexey Novak]

Sorry I had a deliverable to meet at work last week. I will try to resolve this bug this week.

Comment 37

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8686775 [details] [diff] [review]
Bug980331.patch

No worries, let me know how it goes.

Comment 38

[Lin Clark [:linclark]]

We're switching away from XUL in the webconsole, so I'm going to go ahead and close this out.