Closed Bug 1517626 Opened 6 years ago Closed 6 years ago

Stepping out into an HTML 'onclick' attribute shows the HTML file with an unexpected highlight

Categories

(DevTools :: Debugger, defect, P3)

Product:
DevTools
Component:
Debugger
Type:
defect

Tracking

(firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: jlast, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

debug line
(deleted), text/plain
Details
patch
(deleted), patch
loganfsmyth
: review+
Details | Diff | Splinter Review
STR: 1. Open https://firefox-dev.tools/debugger-examples/examples/babel/ 2.Open the debugger 3. Click the `nestedProperty` button and the debugger should pause 4. Click step out ER: Highlight the line in the HTML where the `onclick` attribute is AR: Highlights the second line for some reason: This bug was initially tracked in github https://github.com/devtools-html/debugger.html/issues/7293
Attached file debug line (deleted) —
No longer depends on: 1517625
The problem here starts with compilation of the event handlers on HTML elements, <button onclick="nestedProperty()"> in this case. This is mangled by BuildFunctionString in js/src/vm/CompilationAndEvaluation.cpp into the following source, which is then compiled: function onclick(event) { nestedProperty() } This source has the same URL as the HTML page but no line/column information. Because it has a matching URL the debugger treats it as part of the HTML page, despite the source not matching up with the contents of that HTML page, so stepping around in the onclick script's frame produces weird results (as will just looking at the frame on the stack). I think fixing this would best be done in two steps. This is fundamentally a similar problem to bug 1517167, where sources that are not part of an HTML page are treated as if they are. We don't have any clue from the engine that the sources are not part of the page, and while one could be added it would still leave the door open for similar problems down the road. Pursuing the general strategy outlined in bug 1517167 comment 8 would allow us to detect these cases and show the above source instead of the HTML page. While this would be an improvement, it still wouldn't be ideal. The second step would be to improve things so that the source we get from the debugger is the text which was originally on the page, 'nestedProperty()', without the stuff added by BuildFunctionString. This source could have correct line/column information and we could detect that it *is* part of the HTML page, and show it as part of that HTML page at the correct position.

One concern I have for showing this in the HTML is that JS line/column may not be the same because the string attribute could have HTML entities and things that have varying sizes.

Also, does the engine have a way to provide start column numbers right now? I think I remember it only supporting starting line numbers, which also isn't as likely to break things in <script> but would need to be added for attrs.

I'm not sure it would be worth supporting the second step you have.

There is starting column information. But if we have entity processing and BuildFunctionString mangling going on, it's not going to be very useful.

I especially like this part of Brian's suggestion in bug 1517167 comment 8:

If we reorient things so that the source's text (or a hash of it) is the basis for comparing different sources, or checking if a source came from within an HTML document, then we should be able to avoid all of these problems, and have a simple argument that the debugger is showing correct sources to users.

That is, it's certainly helpful to show the text of the handler attribute in the context in which it was written (the HTML page), but not so valuable that we should risk showing the wrong text altogether. We have the real text in hand, via the Debugger.Source; we should always make sure that we're using it.

Attached patch patch (deleted) — — Splinter Review

Potential patch (plus test), I'll run this through try before adding an r?.

Several things need to be changed for this to work as expected, alas:

  • My understanding of the isInlineSource flag on a source is that indicates whether it is inside an inline <script> and should be displayed as part of the HTML page. Per this definition, isInlineSource is set incorrectly for these event handlers.

  • The server can fold sources into the HTML page even if they aren't inline, as long as their URL matches.

  • Event handlers should be treated as eval'ed code (which they are, really) so that no URL is given to the client and they aren't shown in the sources pane or confused with tabs for the HTML page itself.

Assignee: nobody → bhackett1024

Comment on attachment 9035460 [details] [diff] [review]
patch

This looks good on try.

Attachment #9035460 - Flags: review?(lsmyth)
Comment on attachment 9035460 [details] [diff] [review] patch Review of attachment 9035460 [details] [diff] [review]: ----------------------------------------------------------------- One thing to fix, but looks good otherwise. ::: devtools/server/actors/source.js @@ +38,2 @@ > introType === "Function" || > introType === "eventHandler" || Looks like this is duplicated.
Attachment #9035460 - Flags: review?(lsmyth) → review+
Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b62455787e3f Treat event handlers as eval'ed sources, r=lsmyth.

https://hg.mozilla.org/mozilla-central/rev/b62455787e3f

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: