Closed Bug 1226548 Opened 9 years ago Closed 8 years ago

Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(firefox45 affected, firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox49 --- fixed

People

(Reporter: arni2033, Assigned: miker)

References

(
URL
)

Details

Attachments

(2 files, 2 obsolete files)

testcase 1 - Event listeners tooltip breaks the syntax in popup.html
317 bytes, text/html
Details
event-listeners.diff
32.58 KB, patch
miker
: review+
Details | Diff | Splinter Review 
>>>   My Info:   Win7_64, Nightly 45, 32bit, ID 20151119030404
STR:
1. Open attached "testcase 1"
2. Open devtools -> Inspector, click (ev) button near <body> node

Result:       
 Syntax in event listeners tooltip looks broken: if there's a comment before event handler, event
 listeners popup cuts it starting from the first "," or "!" in comment, or maybe some other symbols.
  The 1st comment within a function also looks broken, but I can't get the logic.
  Probably it's just broken because the portion of the code in popup starts with invalid "world */"
 If in this testcase I remove the comment before Handler, then I still get "broken" code in popup

Expectations: 
 JS code in popup should be valid

// Probably it's Invalid or Wontfix OR maybe there's a dupe OR
// maybe some work is under way on creating "Event listeners" tab in inspector sidebar?
Has STR: --- → yes
Mike, could you please describe here how one should go about fixing this bug? This could perhaps be a good first bug mentored by you?
Also, could you please add a priority for this bug. I would say P3, but I'll leave this to you.
Flags: needinfo?(mratcliffe)
The script returned by the debugger is the whole script and scriptSource.substr(script.sourceStart, script.sourceLength) returns something like:

      () { doSomething(); }

So we work back to the preceeding \n, ; or } so we can get the appropriate function info e.g.:
      () => { doSomething(); }
      function doit() { doSomething(); }
      doit: function() { doSomething(); }

In this case we work back to "," but I can check if we have a way to make this more accurate.
Assignee: nobody → mratcliffe
Flags: needinfo?(mratcliffe)
@bgrins: Because Patrick is away I thought I would ask you for review... the patch is a no-brainer so it should be fairly straightforwards.

The debugger always returns functions, arrow functions etc. in this format:
"() { ... }"

The text manipulation is used simply to get the stuff before the opening bracket e.g.:
"someFuncName: function"
or
"function someName"
etc.

I wanted to check if we had a way to get the functions "full" text but we don't so this is the best we can do for now.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=be712d0aff57
Attachment #8742388 - Flags: review?(bgrinstead) → review?(poirot.alex)
@ochameau: you are a better match for this review.

See comment 6.

If you know of a way to get the "complete" function text then that would be awesome!
Comment on attachment 8742388 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?pbro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47183/diff/1-2/
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> I wanted to check if we had a way to get the functions "full" text but we
> don't so this is the best we can do for now.

I don't know much the Debugger API. But I don't think there is another way to get the source.

I don't see why we try so hard to only display the function and only that.
To me it would be more correct to print the line(s) where the function is defined.
We shouldn't display something different from the original sources!

Then, I imagine we would loose the hability to see exactly which function is called in compressed code where you have multiple functions defined per line.
I know nothing about the editor, but could we highlight a piece of text in it? Like using different color, or have a different background color? Then we could highlight from sourceStart to sourceLength and I imagine everything would be clear enough.
Let's stick with what we have and we can think about other ways of tackling this in the future.
(In reply to Alexandre Poirot [:ochameau] from comment #10)
> We shouldn't display something different from the original sources!

We don't... by default the debugger just provides () { ... } (no function keyword and no function name). Our efforts here are efforts to display the original function.

> I don't see why we try so hard to only display the function and only that.
That is what users want... we have had a lot of positive feedback from users about us only showing the function.

Besides, the source can be very long and displaying it all causes performance issues.

> To me it would be more correct to print the line(s) where the function is
> defined.

Then when displaying minified code they turn out with a long line of minified code instead of a beautified function.
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #12)
> > I don't see why we try so hard to only display the function and only that.
> That is what users want... we have had a lot of positive feedback from users
> about us only showing the function.

I'm not convinced the feedback is about displaying *only* the listener function.
I totally agree the feature is super useful. I just think it would help to 
 *highlight the function within original sources*
rather than
 *only displaying it.*

> 
> Besides, the source can be very long and displaying it all causes
> performance issues.

But I can easily get that point! But I feel like this performance issue should be fixed one way or another in the debugger panel and whenever it comes to display sources, we should be using a component from Debugger panel and benefit from its performances tricks and tweaks.
Comment on attachment 8742388 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?pbro

https://reviewboard.mozilla.org/r/47183/#review44147

::: devtools/server/actors/inspector.js:581
(Diff revision 2)
>      );
>  
>      if (lastEnding !== -1) {
>        let functionPrefix = scriptBeforeFunc.substr(lastEnding + 1);
>        functionSource = functionPrefix + functionSource;
>      }

This bug shows that this lastIndexOf workaround doesn't work.
We should come up with something better.
Unfortunately, I don't think it will come from the debugger API. What we are trying to display is something much more complex than just the function.
In order to implement this function sanely, we would need an AST representation object similar to what we use in eslint.
I don't think Debugger API exposes any of that?

At least we should use regexp rather than character search, this is really too weak.

Could you try to shape up a patch with something like this?
  let before = scriptBeforeFunc.match(/(\w+\s*[:=]\s*)?(function)?(\s*\w+)?\s*$/);

  if (before) {
    functionSource = before[0] + functionSource;
  }
And update previous comment with all the forms we are trying to match? (Here we want to also match doit = function () { ... })
Attachment #8742388 - Flags: review?(poirot.alex)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11)
> Let's stick with what we have and we can think about other ways of tackling this in the future.
So if I ever encounter the same bug with other syntax, I must not file it, right? Please confirm.
(In reply to arni2033 from comment #15)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #11)
> > Let's stick with what we have and we can think about other ways of tackling this in the future.
> So if I ever encounter the same bug with other syntax, I must not file it,
> right? Please confirm.

Mike was refering to my request to implement this feature differently, not to your bug report.
Please keep filing issues you see! You are making really good reports, easily actionable!!
Mike already attached a patch to address exactly what you reported, I just think we should work a bit more to prevent seeing slightly different but similar errors. Our code to handle this is very weak!
Alex is right... lets do this properly.

Here are the different ways a function can be defined:

function a() {
  return "a";
}

let b = function() {
  return "b";
};

let c = function foo() {
  return "c";
}

var d = (function() {
  return function bar() {
    return "d";
  }
})();

var e = new Function();

var f = new function() { return "e"; };

var f2 = new function(a, b, c, "return \"f2\"");

leg g = () => {
  return "arrow1";
};

leg h = a => {
  return "arrow1";
};

let i = b => b;

let obj = {
  j: function() {
    return "obj.j";
  },

  k: function kay() {
    return "obj.k";
  },

  l() {},
  m() {}
};

function* n() {
  return "n";
}

function* () {
  return "anon";
}
let o =
function foo() {
  return "o";
}

let p =
a => {
  return "arrow3";
};
There are simply too many ways to define JS functions to show the exact information so we could cut display just the lines containing the method padded with 5 lines before and 5 after... we can also highlight the function if possible.
You could at least be more confident with a RegExp rather than a multi-character search.
The RegExp would most likely cut more agressively some edge cases, but won't include unexpected junk.

But again, I think displaying the original sources (just the lines where the function is defined) and highlighting the function looks like a better overall approach, and a safe one.
It would be great for example, when you click on the "pause" button, the debugger panel would not just open the related source, but also highlight precisely the function!
(note that there is another bug. the first time you click on "pause" btn, the line of the function isn't highlighted. It is only highlighted if you already loaded the related js file in the debugger)
Getting the real line number is surprisingly hard for inline scripts because, although we have an offset in the script itself we have no offset for that script tag within the HTML file.

My plan:
- Use regexes to get the name part of the function.
- Add 3 lines of padding at the top and bottom of the script.
- Highlight that script and scroll to it.

Seems like our best approach.
Decided against padding etc.

For now we can just display the correct function source along with function names using rehexes.
Now using a regex. The regex isn't perfect e.g. it allows multiple dots or JS keywords  in function names but this doesn't matter because the code visible in the event popups is always valid JavaScript.

This method is far more robust than backtracking and works well, even with our extended tests.

Review commit: https://reviewboard.mozilla.org/r/48451/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/48451/
Attachment #8744315 - Flags: review?(poirot.alex)
Attachment #8742388 - Attachment is obsolete: true
@ochameau: The regex obviously needed to be far more complex than the one you suggested but it works just fine, even with our extended tests.
Works fine... just need to split the test up.
Comment on attachment 8744315 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48451/diff/1-2/
Attachment #8744315 - Flags: review?(poirot.alex)
Comment on attachment 8744315 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?ochameau

https://reviewboard.mozilla.org/r/48451/#review45521

Thanks for the additional tests. I wouldn't expect you to go that deep into regexp and cover so many new cases!
Need another round to triple check that complex expression.

::: devtools/server/actors/inspector.js:93
(Diff revision 2)
>  const HIDDEN_CLASS = "__fx-devtools-hide-shortcut__";
>  const XHTML_NS = "http://www.w3.org/1999/xhtml";
> -const XUL_NS = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';
> +const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
>  const IMAGE_FETCHING_TIMEOUT = 500;
> +const RX_FUNC_NAME = /(?:\s*|^)(?:(?:var|const|let)\s+)?(?:[$A-Za-z_.][0-9A-Za-z_$.]*\s*[:=]\s*)*function?(?:\s*\*\s*)?(?:\s*[$A-Za-z_][0-9A-Za-z_$]*)?\s*$/;
> +const RX_ES6_FUNC = /[$A-Za-z_][0-9A-Za-z_$]*\s*$/;

- Are "non-capturing parentheses", i.e. (?: really necessary? It makes the regexp much harder to read. Does that make the regexp any faster? Otherwise as, we just fetch the overall expression, we don't care much about capturing individual patterns or not.

`(\s*|^)((var|const|let)\s+)?([$A-Za-z_.][0-9A-Za-z_$.]*\s*[:=]\s*)*function?(\s*\*\s*)?(\s*[$A-Za-z_][0-9A-Za-z_$]*)?\s*$/`

- `function` should be enclosed by parentheses:
`function?` -> `(function)?`
I think that's the reason that force you to have another ES6 regex. With that, I think we can have only one.

`(\s*|^)((var|const|let)\s+)?([$A-Za-z_.][0-9A-Za-z_$.]*\s*[:=]\s*)*(function)?(\s*\*\s*)?(\s*[$A-Za-z_][0-9A-Za-z_$]*)?\s*$/`

- I think there is a simplification around usages of \s* combined to ?:
`(\s*\*\s*)?(\s*` -> `\s*\*?\s*(`

`(\s*|^)((var|const|let)\s+)?([$A-Za-z_.][0-9A-Za-z_$.]*\s*[:=]\s*)*(function)?\s*\*?\s*([$A-Za-z_][0-9A-Za-z_$]*)?\s*$/`

- nit: [0-9A-Za-z_$] is equivalent of [\w$]
- I do not understand why we need `(\s*|^)`?

::: devtools/server/actors/inspector.js:567
(Diff revision 2)
> -    So we need to work back to the preceeding \n, ; or } so we can get the
> -      appropriate function info e.g.:
> -      () => { doSomething(); }
> -      function doit() { doSomething(); }
> -      doit: function() { doSomething(); }
> +    So we need to use some regex magic to get the appropriate function info
> +    e.g.:
> +      () => { ... }
> +      function doit() { ... }
> +      doit: function() { ... }
> +      es6func() { ... }

Please also mention variable pattern:
  var|let|const foo = function () { ... }
and generators:
  function generator*() { ... }

::: devtools/server/actors/inspector.js:577
(Diff revision 2)
> -      scriptBeforeFunc.lastIndexOf("}"),
> -      scriptBeforeFunc.lastIndexOf("{"),
> -      scriptBeforeFunc.lastIndexOf("("),
> -      scriptBeforeFunc.lastIndexOf(","),
> -      scriptBeforeFunc.lastIndexOf("!")
> -    );
> +      functionSource = matches[0].trim() + functionSource;
> +    } else {
> +      matches = scriptBeforeFunc.match(RX_ES6_FUNC);
> +      if (matches && matches.length > 0) {
> +        functionSource = matches[0].trim() + functionSource;
> +      }

See previous comment, but I think you should need only one regexp.
Comment on attachment 8744315 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/48451/diff/2-3/
Attachment #8744315 - Flags: review?(poirot.alex)
I have optimized and simplified the regex:
/((var|const|let)\s+)?([\w$.]+\s*[:=]\s*)*(function)?\s*\*?\s*([\w$]+)?\s*$/

The source we get is always valid so there is no need to validate function names etc.

And of course, only one regex.
Comment on attachment 8744315 [details]
MozReview Request: Bug 1226548 - Event listeners tooltip breaks the syntax in popup if there's a comment before event handler in JS code r?ochameau

https://reviewboard.mozilla.org/r/48451/#review45595

Tested on various websites, works great.
Thanks for accepting my challenges ;)
Attachment #8744315 - Flags: review?(poirot.alex) → review+
needs rebasing it seems
Flags: needinfo?(mratcliffe)
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #33)
> needs rebasing it seems

Works fine for me with minimal fuzzing... here is a version rebased on the latest fx-team.
Attachment #8744315 - Attachment is obsolete: true
Flags: needinfo?(mratcliffe)
Attachment #8745318 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/412f3942777e
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #30)
> /((var|const|let)\s+)?([\w$.]+\s*[:=]\s*)*(function)?\s*\*?\s*([\w$]+)?\s*$/
I took a look at that regexp and realized I can "break" it with the following urls:
1) data:text/html,<body><script>var B='body'; document[B].onclick=_=>{};</script>
2) data:text/html,<body><script>function F(){return document.body}; F().onclick=_=>{};</script>
Event listeners tooltip shows ".onclick = _ => {}".   Is this acceptable outcome?
Flags: needinfo?(mratcliffe)
I have reproduced this bug with Nightly 45.0a1 (2015-11-20) on Windows 7, 64 Bit!

This bug's fix is verified with latest Beta 

Build  ID     20160811031722
User Agent    Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160812]
Depends on: 1321551
Flags: needinfo?(mratcliffe)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: