Only list evaluated scripts containing a source URL

RESOLVED FIXED in Firefox 37

Status

()

Firefox
Developer Tools: Debugger
--
enhancement
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sebo, Assigned: jlongster)

Tracking

unspecified
Firefox 38
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 wontfix, firefox37 fixed, firefox38 fixed)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

3 years ago
To avoid long lists of evaluated scripts within the debugger sources only scripts having a //# sourceURL=foo.js should be shown.

See https://www.polymer-project.org/ as example page for a huge list of evaluated scripts.

FWIW this is also how the Chrome DevTools do it.[1]

Sebastian

[1] https://developer.chrome.com/devtools/docs/javascript-debugging#breakpoints-dynamic-javascript
(Reporter)

Comment 1

3 years ago
Created attachment 8552329 [details]
Test case

Here's a test case with step by step instructions and description of the expected result.

Sebastian
(Assignee)

Comment 2

3 years ago
This is a worthwhile consideration. Having seen the usages of evals in the wild after landing eval debugging support, I think we should consider at least hiding them until the user requests them. We still want to show them if needed, but like Chrome we could hide them at first.

This well-timed because I just did a bunch of research into how slow we are at initializing scripts, so I am concerned about showing huge amounts of eval scripts. Eventually we want to show the sources in a tree like Chrome does, but until then we don't have a good way to hide them until the user requests them.

Perhaps a short-term solution is to hide them by default, but provide an option to the user to display them, and if changed we just re-fetch everything.
(Assignee)

Updated

3 years ago
See Also: → bug 1111369
(Assignee)

Comment 3

3 years ago
I saw we move forward with this. There are too many subtle bugs (see bug 1132443) for me to feel comfortable releasing this into the wild right now. And really, I don't think there's much gain. When someone wants to debug eval scripts, they really need to just give it a name (Chrome has this exact behavior right now).
(In reply to James Long (:jlongster) from comment #3)
> I saw we move forward with this. There are too many subtle bugs (see bug
> 1132443) for me to feel comfortable releasing this into the wild right now.
> And really, I don't think there's much gain. When someone wants to debug
> eval scripts, they really need to just give it a name (Chrome has this exact
> behavior right now).

How about make it a pref (default to don't show) that is toggle-able in the debugger's options menu?
(Reporter)

Comment 5

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> (In reply to James Long (:jlongster) from comment #3)
> > When someone wants to debug
> > eval scripts, they really need to just give it a name (Chrome has this exact
> > behavior right now).
> 
> How about make it a pref (default to don't show) that is toggle-able in the
> debugger's options menu?

This seems unnecessary to me. In case they are asking why they don't see eval scripts in the list, you would have to tell them about that option. In that case you can teach them about the source URL feature instead (which also allows the user to find it more easily).
Though I have no strong opinion against such an option.

Sebastian
See Also: → bug 1122222
(Assignee)

Comment 6

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> (In reply to James Long (:jlongster) from comment #3)
> > I saw we move forward with this. There are too many subtle bugs (see bug
> > 1132443) for me to feel comfortable releasing this into the wild right now.
> > And really, I don't think there's much gain. When someone wants to debug
> > eval scripts, they really need to just give it a name (Chrome has this exact
> > behavior right now).
> 
> How about make it a pref (default to don't show) that is toggle-able in the
> debugger's options menu?

Hey Nick, sorry I didn't get this earlier (especially since the merge is happening next week...). I'm open to something like that, but I feel strongly that right now, we just need to hide them. I want the patch to be as tiny as possibly since it's going to be uplifted right before a merge. I think it's very suitable to enable eval debugging only for scripts that have `sourceURL`, and we'll figure out how to handle all the other cases over time.

I'm going to make a patch and use this bug to land it.
(Assignee)

Comment 7

3 years ago
Created attachment 8566787 [details] [diff] [review]
1124106.patch
Attachment #8566787 - Flags: review?(nfitzgerald)
(Assignee)

Comment 8

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbcbe2053575
Comment on attachment 8566787 [details] [diff] [review]
1124106.patch

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

(Tad busy at the moment)
Attachment #8566787 - Flags: review?(nfitzgerald) → review?(ejpbruel)
(Assignee)

Comment 10

3 years ago
Eddy if you could look at this patch asap, I can land it tomorrow. I need to uplift if before next week, unfortunately. Sorry for the pressure.
Comment on attachment 8566787 [details] [diff] [review]
1124106.patch

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

No problem James. Patch looks good to me. r+.
Attachment #8566787 - Flags: review?(ejpbruel) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/7936ccc85422
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 13

3 years ago
Created attachment 8567289 [details] [diff] [review]
1124106-aurora-beta.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: Our new feature of showing eval scripts will be too aggressive. We show all eval scripts which makes the source list in the debugger huge in many sites (and slow). Lastly, we are still finding small bugs related to this because we use `eval` in various part of the debugger and those scripts are showing up.
[Describe test coverage new/current, TreeHerder]: all this does is hide them, no new additional tests needed
[Risks and why]: the risks are very small. this is a simple change that only could possibly affect the debugger
[String/UUID change made/needed]:
Attachment #8567289 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 14

3 years ago
Comment on attachment 8567289 [details] [diff] [review]
1124106-aurora-beta.patch

Approval Request Comment
[Feature/regressing bug #]: NA
[User impact if declined]: Our new feature of showing eval scripts will be too aggressive. We show all eval scripts which makes the source list in the debugger huge in many sites (and slow). Lastly, we are still finding small bugs related to this because we use `eval` in various part of the debugger and those scripts are showing up.
[Describe test coverage new/current, TreeHerder]: no new test coverage needed, only hides the scripts
[Risks and why]: not much risk, small patch could only affect debugger
[String/UUID change made/needed]:

Unfortunately this was a last minute decision that needs to be uplifted before next week's merge. I know it's not the best, but I think we could potentially release something into the wild that introduces too many bugs/surprises.

Unless more developers are on Dev Edition than release, it doesn't matter much then.
Attachment #8567289 - Flags: approval-mozilla-beta?
(In reply to James Long (:jlongster) from comment #14)
> [Feature/regressing bug #]: NA

In what release did we add the feature to show eval scripts?

Given that 36 is scheduled to release on Tuesday and this fix hasn't even hit m-c yet, I don't think we can take take the fix in 36. I also don't think this is a stop ship issue. We can consider taking this in 37 once the fix is verified.
status-firefox36: --- → ?
status-firefox37: --- → ?
status-firefox38: --- → ?
Assignee: nobody → jlong
(Assignee)

Comment 16

3 years ago
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #15)
> (In reply to James Long (:jlongster) from comment #14)
> > [Feature/regressing bug #]: NA
> 
> In what release did we add the feature to show eval scripts?
> 
> Given that 36 is scheduled to release on Tuesday and this fix hasn't even
> hit m-c yet, I don't think we can take take the fix in 36. I also don't
> think this is a stop ship issue. We can consider taking this in 37 once the
> fix is verified.

Fair enough, you are right that it's too risky. I'm fine shipping it with 36. We added the feature to show eval scripts in 36, is that what you mean?

I'll get this uplifted to at least aurora, and maybe beta if it doesn't happen before the merge.
(Assignee)

Updated

3 years ago
status-firefox36: ? → affected
status-firefox37: ? → affected
status-firefox38: ? → affected
(Assignee)

Comment 17

3 years ago
Comment on attachment 8567289 [details] [diff] [review]
1124106-aurora-beta.patch

Removing the beta flag for now. I'll request uplift to beta if this isn't uplifted to aurora before the merge next week.
Attachment #8567289 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/7936ccc85422
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
Comment on attachment 8567289 [details] [diff] [review]
1124106-aurora-beta.patch

Taking this in 37 now that it has been on m-c for a couple of days. 37 is now Beta so I have dropped the Aurora request and approved for Beta.

Beta+
Attachment #8567289 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
status-firefox36: affected → wontfix
https://hg.mozilla.org/releases/mozilla-beta/rev/d3a6c9b754f2
status-firefox37: affected → fixed

Comment 21

3 years ago
(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> (In reply to James Long (:jlongster) from comment #3)
> > I saw we move forward with this. There are too many subtle bugs (see bug
> > 1132443) for me to feel comfortable releasing this into the wild right now.
> > And really, I don't think there's much gain. When someone wants to debug
> > eval scripts, they really need to just give it a name (Chrome has this exact
> > behavior right now).
> 
> How about make it a pref (default to don't show) that is toggle-able in the
> debugger's options menu?

As a web security analyst I would really appreciate this. As someone who looks at malicious eval() code all day though various JS engines/emulators, having an actual browser(with an actual DOM!) that can displays all eval() calls is great.

In fact, I only noticed this feature when I skimped though Firefox v36's release note :) . At least this means I don't have to manually hook into IE's dll calls anymore.
You need to log in before you can comment on or make changes to this bug.