980910 - Ensure devtools/framework/toolbox-process-window.xul is free of inline script

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Mark Goodwin [:mgoodwin]]

There's an oncommand attribute on toolbox-cmd-close.

Comment 1

[KLG]

I would be interested in working on this bug.  I am new to Mozilla and would like to start with this.

Comment 2

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

Okay, great!  I've assigned the bug to you.

Take a look through our docs[1] on how to get started with the code base.  Also, when you are ready to prepare a patch, there are some good docs[2] there as well.

For this bug, the goal is to remove the "oncommand" in toolbox-process-window[3].  This file is used when you open the Browser Toolbox[4].  Instead of using the attribute, we want to reattach the "command" event handler from within the "toolbox-process-window.js" file.

If you have questions you can ask either here in the bug or in our IRC channel (#devtools).  Good luck!

[1]: https://wiki.mozilla.org/DevTools/Hacking
[2]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[3]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-process-window.xul#26
[4]: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Comment 3

[vikneshwar]

Attachment: command.patch

Comment 4

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

Comment on attachment 8420546 [details] [diff] [review]
command.patch

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

Are you having some kind of trouble getting it to work?  It seems like you have added a made-up button ID for testing?

Comment 5

[vikneshwar]

Attachment: inline-script.patch

Really Sorry , i was testing and its my mistake i dint edit the code . Uploaded a new patch . Please , check it :)

Comment 6

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

Comment on attachment 8421720 [details] [diff] [review]
inline-script.patch

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

::: browser/devtools/framework/toolbox-process-window.js
@@ +125,5 @@
>    let regex = new RegExp("[\\?&]" + name + "=([^&#]*)");
>    let results = regex.exec(window.location.search);
>    return results == null ? "" : decodeURIComponent(results[1].replace(/\+/g, " "));
>  }
> +document.getElementById("toolbox-cmd-close").addEventListener("command",function(e){ window.close(); },false);

It's better to wait until the "load" event before accessing the DOM.

So, I'd suggest adapting the existing load event handler to do this and then after that, it can call |connect| like it does today.

Also, you should have your "command" listener either remove itself, or add it to |onUnload| to be removed there.

Comment 7

[vikneshwar]

Attachment: inline-script.patch

Comment 8

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

Comment on attachment 8423259 [details] [diff] [review]
inline-script.patch

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

Also, have you tested to ensure the window does indeed still close after these changes?  I've seen some strange known issues that could be related, such as bug 371900, so please be sure to test this.

::: browser/devtools/framework/toolbox-process-window.js
@@ +51,5 @@
>  
> +window.addEventListener("load", function() {
> +   document.getElementById("toolbox-cmd-close").addEventListener("command",onCommand);
> +   connect();
> +   },false

No need to supply |false| here, as that's the default value of the 3rd parameter.

Also, move the brace down:

});

@@ +54,5 @@
> +   connect();
> +   },false
> +);
> +
> +function onCommand(event) {

Rename this to |onCloseCommand|

@@ +89,5 @@
>  
>  function onUnload() {
>    window.removeEventListener("unload", onUnload);
>    window.removeEventListener("message", onMessage);
> +  window.removeEventListener("command", onCommand);

You need to remove the event listener from the element it's attached to, which is not |window|.

Comment 9

[vikneshwar]

Attachment: removed-inline-script.patch

jryans , yeah i can close Browser-Toolbox window after my previous changes . Made changes as you mentioned and uploaded a new patch . Thanks :)

Comment 10

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

Comment on attachment 8423911 [details] [diff] [review]
removed-inline-script.patch

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

Okay, you're almost there!

A few nits to fix, but also you'll need to correct your patch format[1].  In particular, the author line is missing, so it does not apply as-is.  Also, since you have to update it anyway, you should change the commit message to end with "r=jryans".

The next patch should be ready for review, so use the "review" flag next time.

[1]: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: browser/devtools/framework/toolbox-process-window.js
@@ +49,5 @@
>    });
>  }
>  
> +window.addEventListener("load", function() {
> +   document.getElementById("toolbox-cmd-close").addEventListener("command",onCloseCommand);

Nits: Use a space between function arguments.  Also, max line length is 80 characters.  Read over the coding style guide[2].  In this case, it might be simplest to break up with a temporary variable:

let cmdClose = document.getElementById("toolbox-cmd-close");
cmdClose.addEventListener("command", onCloseCommand);

[2]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

@@ +88,5 @@
>  
>  function onUnload() {
>    window.removeEventListener("unload", onUnload);
>    window.removeEventListener("message", onMessage);
> +  document.getElementById("toolbox-cmd-close").removeEventListener("command", onCloseCommand);

Nits: Same issues here, needs a space, and the line is too long.

Comment 11

[vikneshwar]

Attachment: command-close.patch

Comment 12

[vikneshwar]

Attachment: onclose-commad.patch

Added extra space by mistake , so updated with new patch :)

Comment 13

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

Comment on attachment 8424139 [details] [diff] [review]
command-close.patch

Marking older patch as obsolete.

Comment 14

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

Comment on attachment 8424264 [details] [diff] [review]
onclose-commad.patch

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

This patch seems to now prevent the Browser Toolbox from starting up, which was not the case with previous versions.

Please take a look at this issue.  It's possible this is a platform-specific...  (I am testing on a Mac.)

::: browser/devtools/framework/toolbox-process-window.js
@@ +12,5 @@
>    Cu.import("resource://gre/modules/devtools/dbg-client.jsm", {});
>  let { ViewHelpers } =
>    Cu.import("resource:///modules/devtools/ViewHelpers.jsm", {});
>  
> +let cmdClose = document.getElementById("toolbox-cmd-close");

Perhaps the start up error is due to placing this here?  You can't be sure this element is available until the "load" event, so you may need to have this inside the event listener.

Comment 15

[vikneshwar]

Attachment: removed-inline-script.patch

Comment 16

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

Comment on attachment 8425444 [details] [diff] [review]
removed-inline-script.patch

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

*Please* double-check your code style with each patch!  I feel like we're going in circles here...

::: browser/devtools/framework/toolbox-process-window.js
@@ +89,5 @@
>  
>  function onUnload() {
>    window.removeEventListener("unload", onUnload);
>    window.removeEventListener("message", onMessage);
> +  let cmdClose=document.getElementById("toolbox-cmd-close");

Spaces before and after equals.

Comment 17

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

Please use the "review?" flag when asking for a patch to be reviewed, as I've already explained in comment 10.

You appear to be ignoring comments like this, which is frustrating.  Please read the comments carefully.

Comment 18

[vikneshwar]

Attachment: command

Sorry , jryans :( i was not sure about the patch , so i thought its not good to ask review now . Thank you

Comment 19

[vikneshwar]

Attachment: commad.patch

Comment 20

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

Comment on attachment 8425704 [details] [diff] [review]
command

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

Fix commit message to add r=jryans, and fix spelling: tot -> to, commad -> command.

Comment 21

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

Comment on attachment 8425709 [details] [diff] [review]
commad.patch

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

See above comments about patch comment.

Comment 22

[vikneshwar]

Attachment: command-edited.patch

Comment 23

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

Comment on attachment 8425733 [details] [diff] [review]
command-edited.patch

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

Okay, looks good.

We'll see what try thinks about it:

https://tbpl.mozilla.org/?tree=Try&rev=b62bdc1b8d6c

Comment 24

[vikneshwar]

jryans , Sorry for taking this patch so long . And can you assign this bug to me as it has been assigned to someone :)

Comment 25

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

Try looks good.

Comment 26

[[e-mail]]

(In reply to J. Ryan Stinnett from comment #8)
> Also, have you tested to ensure the window does indeed still close after
> these changes?  I've seen some strange known issues that could be related,
> such as bug 371900, so please be sure to test this.

I'd be surprised if bug 371900 didn't affect you here.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54b6736975ed

Comment 28

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

(In reply to [e-mail] from comment #26)
> (In reply to J. Ryan Stinnett from comment #8)
> > Also, have you tested to ensure the window does indeed still close after
> > these changes?  I've seen some strange known issues that could be related,
> > such as bug 371900, so please be sure to test this.
> 
> I'd be surprised if bug 371900 didn't affect you here.

Well, it does indeed seem to "work" currently, despite what bug 371900 says...  Though, to be honest, I have not recently tested the case of simple removing the close handler entirely, so perhaps something else catches close actions at the moment, rendering the code here redundant.  Something to investigate later, I suppose.

Comment 29

[[e-mail]]

(In reply to J. Ryan Stinnett from comment #28)
> (In reply to comment #26)
> > (In reply to J. Ryan Stinnett from comment #8)
> > > Also, have you tested to ensure the window does indeed still close after
> > > these changes?  I've seen some strange known issues that could be related,
> > > such as bug 371900, so please be sure to test this.
> > 
> > I'd be surprised if bug 371900 didn't affect you here.
> 
> Well, it does indeed seem to "work" currently

Well, I admit it wasn't a proper test, but I tried patching omni.ja directly and the key stopped working for me. (I might try a new build over the weekend.)

Comment 30

[[e-mail]]

Comment on attachment 8425733 [details] [diff] [review]
command-edited.patch

>-window.addEventListener("load", connect);
>+window.addEventListener("load", function() {
>+  let cmdClose = document.getElementById("toolbox-cmd-close");
>+  cmdClose.addEventListener("command", onCloseCommand);
>+  connect();
>+});
Unrelated bug I just spotted: connect tries to remove itself as a load listener, but of course it's not the load listener any more.