961524 - Remove inline scripts/styles from browser/devtools/debugger/debugger.xul

Status: RESOLVED INVALID

(DevTools :: Debugger, defect, P3)

Description

[Brian Grinstead [:bgrins]]

https://mxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul.

There are some inline event handler scripts in the debuggerCommands element and debuggerPopupset element.

Comment 1

[k]

Where should the adding of the listeners be moved to?

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to k from comment #1)
> Where should the adding of the listeners be moved to?

Since the commands touch so many different parts of the view, I would almost add just add new methods for all the event binding in debugger-view.js for _initializeCommands / _destroyCommands that would be called at the same time as _initializePanes / _destroyPanes.

Nick, does this seems like a good place in the debugger js to move the inline scripts from debuggerCommands and debuggerPopupset, or do you think they would fit somewhere else?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I would say that if we are going to move, just put them in each corresponding view's initialize method. Why are we moving them?

Victor might have an opinion as well.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> I would say that if we are going to move, just put them in each
> corresponding view's initialize method. Why are we moving them?

See Bug 960728 and Bug 923902, it is to help secure the browser UI / chrome code.

Comment 5

[Victor Porof [:vporof][:vp]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> I would say that if we are going to move, just put them in each
> corresponding view's initialize method. Why are we moving them?
> 
> Victor might have an opinion as well.

_initializeCommands / _destroyCommands in DebuggerView, analog to _initializePanes / _destroyPanes sounds like a good idea.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #5)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #3)
> > I would say that if we are going to move, just put them in each
> > corresponding view's initialize method. Why are we moving them?
> > 
> > Victor might have an opinion as well.
> 
> _initializeCommands / _destroyCommands in DebuggerView, analog to
> _initializePanes / _destroyPanes sounds like a good idea.

Yes, I think putting it all in one place (even though it touches different views) keeps it closer to how it is working now / makes it easier to copy and paste when adding a new command in the future.

Kay, are you going to be working on this?

Comment 7

[k]

Yepp

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to k from comment #7)
> Yepp

Cool

Comment 9

[k]

Attachment: moved listener adding into debugger-view.js

Here a first revision.

Where should the pop-up listeners go to?

Comment 10

[Victor Porof [:vporof][:vp]]

Comment on attachment 8365654 [details] [diff] [review]
moved listener adding into debugger-view.js

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

Drive-by.

::: browser/devtools/debugger/debugger-view.js
@@ +62,5 @@
>      this.WatchExpressions.initialize();
>      this.EventListeners.initialize();
>      this.GlobalSearch.initialize();
>      this._initializeVariablesView();
> +    deferred.promise.then(this._initializeCommands);

Does _initializeCommands need to do something after creating the source editor? Doesn't look like it to me. I think you should simply call this method normally.

Comment 11

[k]

Somehow |this.Source| wasn't completely available when I tried it before the promise resolved.

Comment 12

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vp] from comment #10)
> Comment on attachment 8365654 [details] [diff] [review]
> 
> Does _initializeCommands need to do something after creating the source
> editor? Doesn't look like it to me. I think you should simply call this
> method normally.

I think you mean this.Sources. There's no need to call _initializeCommands inside then().

Comment 13

[k]

When I do it like this:
>      this.GlobalSearch.initialize();
>      this._initializeVariablesView();
>      this._initializeCommands();

I get the error that this.Sources.selectNextItem() is undefined.

Comment 14

[Victor Porof [:vporof][:vp]]

Hah, it's great that you found this. Those methods actually don't exist, and the fact that you put them in the .then() call of a promise basically silenced the error :)

Remove them completely, they're obsolete and not needed anymore. Remove the commands in xul as wel. They should have been removed a long time ago, but I guess me or somebody else omitted to do so.

Comment 15

[Victor Porof [:vporof][:vp]]

Do we also need to do something about onpopupshowing and friends?

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #15)
> Do we also need to do something about onpopupshowing and friends?

Yes, the onpopupshowing / onpopuphiding / onpopuphidden in debuggerPopupset need to be moved to the JS file as well (I count 4 more inline handlers).  For reference, here are the 4 commands in the popupset:

    #sourceEditorContextMenu onpopupshowing="goUpdateGlobalEditMenuItems()"
    #debuggerPrefsContextMenu onpopupshowing="DebuggerView.Options._onPopupShowing()"
    #debuggerPrefsContextMenu onpopuphiding="DebuggerView.Options._onPopupHiding()"
    #debuggerPrefsContextMenu onpopuphidden="DebuggerView.Options._onPopupHidden()"

I would think that we make a set of two functions just like the ones you already added for the commands, but called _initializeMenus / _destroyMenus.  Another option would be to throw them in the existing command handlers, but these seem different enough to warrant their own functions.

Comment 17

[k]

Attachment: moved listener adding into debugger-view.js

I removed those missing commands from JavaScript and XUL and called _initializeCommands() normally and not with then().

Also added functions for the menus.

Comment 18

[Victor Porof [:vporof][:vp]]

Comment on attachment 8366414 [details] [diff] [review]
moved listener adding into debugger-view.js

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

Couple of extra comments below. r+ with them addressed.

Besides pushing to try (which I'll do in a minute), have you tried also manually testing all those event listeners? Some of them may not be automatically verified in tests.

::: browser/devtools/debugger/debugger-view.js
@@ +125,5 @@
> +
> +    for( let id in this._menuMapping ) {
> +      let listeners = this._menuMapping[id];
> +      let menu = document.getElementById(id);
> +      for( let event in listeners ) {

Nit: whitespace "for (let event...)"

@@ +130,5 @@
> +        menu.addEventListener(event, listeners[event]);
> +      }
> +    }
> +  },
> +  

Nit: please remove trailing whitespace here.

@@ +135,5 @@
> +  /**
> +   * Removes the listeners for all menus
> +   */
> +  _destroyMenus: function() {
> +    for( let id in this._menuMapping ) {

Nit: make formatting consistent with the rest of the file: "for (let event...)"

@@ +138,5 @@
> +  _destroyMenus: function() {
> +    for( let id in this._menuMapping ) {
> +      let listeners = this._menuMapping[id];
> +      let menu = document.getElementById(id);
> +      for( let event in listeners ) {

Ditto.

@@ +187,5 @@
> +      startTracing: tracer._onStartTracing.bind(tracer),
> +      clearTraces: tracer._onClear.bind(tracer)
> +    };
> +
> +    for( let id in this._commandMapping ) {

Ditto.

@@ +197,5 @@
> +  /**
> +   * Removes listeners from all commands
> +   */
> +  _destroyCommands: function() {
> +    for( let id in this._commandMapping ) {

Ditto.

::: browser/devtools/debugger/debugger.xul
@@ -42,5 @@
> -             oncommand="DebuggerView.Sources.toggleBreakpoints()"/>
> -    <command id="nextSourceCommand"
> -             oncommand="DebuggerView.Sources.selectNextItem()"/>
> -    <command id="prevSourceCommand"
> -             oncommand="DebuggerView.Sources.selectPrevItem()"/>

These two commands also have corresponding keysets in #debuggerKeys. Remove them from there as well.

Comment 19

[Victor Porof [:vporof][:vp]]

Try is closed at the moment, I'll push later :)

Comment 20

[k]

Attachment: moved listener adding into debugger-view.js

okay, removed the key-bindings and cleaned up the formatting a bit :)

Comment 21

[Victor Porof [:vporof][:vp]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0a6259d235ea

Comment 22

[Victor Porof [:vporof][:vp]]

Let's do another one, it seems the previous run got oranged by another patch :)
If this one is green, please add a checkin-needed flag in the keywords section.

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

Comment 23

[Victor Porof [:vporof][:vp]]

I've played with this patch a bit more and there are a few issues:

* the Cmd+P command (file search) doesn't work anymore
* the Cmd+D command (function search) doesn't work anymore
* the Cmd+Alt+V command (variables search) doesn't work anymore
* the Cmd+Shift+V command (variables focus) doesn't work anymore
* the Cmd+B command (add breakpoint) doesn't work anymore
* the Cmd+Shift+B command (add conditional breakpoint) doesn't work anymore
* the Cmd+Shift+E command (add watch expression) doesn't work anymore
* the Cmd+Alt+E command (remove all watch expressions) doesn't work anymore
* the startTracing command isn't used anywhere; it should be removed

Brian, any clues?

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #23)
> I've played with this patch a bit more and there are a few issues:
> 
> * the Cmd+P command (file search) doesn't work anymore
> * the Cmd+D command (function search) doesn't work anymore
> * the Cmd+Alt+V command (variables search) doesn't work anymore
> * the Cmd+Shift+V command (variables focus) doesn't work anymore
> * the Cmd+B command (add breakpoint) doesn't work anymore
> * the Cmd+Shift+B command (add conditional breakpoint) doesn't work anymore
> * the Cmd+Shift+E command (add watch expression) doesn't work anymore
> * the Cmd+Alt+E command (remove all watch expressions) doesn't work anymore
> * the startTracing command isn't used anywhere; it should be removed
> 
> Brian, any clues?

I have seen this, but haven't yet been able to look further into it.  My best guess is that we may need to prevent default on the command event or use capturing in addeventlistener, as is seems that all the shortcuts that aren't working seem to have default browser actions that were able to be overridden with the oncommand="" syntax.

A couple of other things worth trying:
* It may be worth also checking to see if setting command.oncommand=this._commandMapping[id] works more like the XUL attributes do.
* Maybe set an empty oncommand handler in xul, while still using the addeventlistener and see if this restores the old behavior 

I will take a closer look at this next week if these ideas don't help.

Comment 25

[k]

Tried all these ideas, but non worked.

Moved one command-listener back to XUL, to see if I broke anything else, but it worked.

I also tried to move the call of  _initializeCommands() to another place (with oncommand=... and addEventListener) but it didn't yield any results :\

Comment 26

[Brian Grinstead [:bgrins]]

So it seems like if I add  oncommand=";" to each of the commands in the xul, everything starts working again.  Not saying this is what we should do, just noticing that somehow adding *some* handler there is allowing these shortcuts to start working.

Comment 27

[Brian Grinstead [:bgrins]]

As best as I can tell, the keyboard shortcuts are not working because the oncommand attributes are missing, an issue related to Bug 371900 and as mentioned in Comment 26.

Mark, it seems like this could be worked around by adding oncommand=";" as an inline script to each <command> element, but I'm not sure if this is any improvement towards the effort of applying a CSP (and it seems like a not very nice hack).

Frederik, I don't know of any other workaround for this problem with missing oncommand attributes and keyboard shortcuts not working, but maybe you've bumped into it when removing inline scripts in other chrome code?

Comment 28

[Frederik Braun [:freddy]]

I am not sure I completely understand the problem at hand. Don't the event handlers for "command" fire?
I noticed that other patches used |addEventListener("command", ..| but they are mostly WIP and untested, so maybe you're unto something?!

Comment 29

[Brian Grinstead [:bgrins]]

(In reply to Frederik Braun [:freddyb] from comment #28)
> I am not sure I completely understand the problem at hand. Don't the event
> handlers for "command" fire?
> I noticed that other patches used |addEventListener("command", ..| but they
> are mostly WIP and untested, so maybe you're unto something?!

No, the command callbacks don't fire and instead the normal browser shortcuts happen.  For instance, CMD + P usually focuses the debugger search box.  If there is no inline oncommand handler, the box doesn't get focused and instead the print dialog opens.  If we add an inline oncommand callback the box gets focused again.  It is possible that it is something weird with this situation, but through trial and error I've noticed that the registered callback doesn't work in 3 of these 4 cases:

<command>                <-- doesn't work
<command oncommand>      <-- doesn't work
<command oncommand="">   <-- doesn't work
<command oncommand=";">  <-- works

Comment 30

[Mark Goodwin [:mgoodwin]]

(In reply to Brian Grinstead [:bgrins] from comment #27)
> Mark, it seems like this could be worked around by adding oncommand=";" as
> an inline script to each <command> element, but I'm not sure if this is any
> improvement towards the effort of applying a CSP (and it seems like a not
> very nice hack).

Yeah, any inline script kind of defeats the point (as we'd have to allow all inline scripts for this to work)

Comment 31

[k]

So...what should happen now?

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to k from comment #31)
> So...what should happen now?

I've found Bug 547189, which is the same issue we are seeing.  We will have to wait for this to be resolved before proceeding.

Comment 33

[Brian Grinstead [:bgrins]]

(In reply to k from comment #31)
> So...what should happen now?

Kay, can you upload a patch with oncommand=";" in the markup for all of the commands?  We can can land it this way, and then have a follow up that removes this attribute after Bug 371900 lands.

Comment 34

[k]

Okay

Comment 35

[k]

Attachment: moved listener adding into debugger-view.js v2

added oncommand=";" listeners inside .xul.

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8384167 [details] [diff] [review]
moved listener adding into debugger-view.js v2

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

The only change between this patch and the last is the oncommand=";" lines [0], so r+ with green tests (I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=62f80fbd53c0).  I've tested locally and keyboard shortcut behavior seems to be restored.

Victor, can you have a look and make sure everything is working as expected?

[0]: http://benjamin.smedbergs.us/interdiff/interdiff.php?patch1url=https%3A%2F%2Fbug961524.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8366489&patch2url=https%3A%2F%2Fbug961524.bugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D8384167

Comment 37

[Victor Porof [:vporof][:vp]]

Comment on attachment 8384167 [details] [diff] [review]
moved listener adding into debugger-view.js v2

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

::: browser/devtools/debugger/debugger.xul
@@ +31,5 @@
>  
>    <commandset id="editMenuCommands"/>
>  
>    <commandset id="debuggerCommands">
> +    <command id="blackBoxCommand" oncommand=";"/>

Maybe add a comment above this as to why we have all the empty oncommands? With a link to a bug.

Comment 38

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #37)
> Comment on attachment 8384167 [details] [diff] [review]
> moved listener adding into debugger-view.js v2
> 
> Review of attachment 8384167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/debugger/debugger.xul
> @@ +31,5 @@
> >  
> >    <commandset id="editMenuCommands"/>
> >  
> >    <commandset id="debuggerCommands">
> > +    <command id="blackBoxCommand" oncommand=";"/>
> 
> Maybe add a comment above this as to why we have all the empty oncommands?
> With a link to a bug.

Good idea - the best bug to link to would be: https://bugzilla.mozilla.org/show_bug.cgi?id=978115

Comment 39

[Bryan Clark (DevTools PM) [:clarkbw]]

With the new debugger released this old debugger.xul work isn't going to get picked up.