Closed Bug 933738 Opened 6 years ago Closed 6 years ago

Please don't allow stepping into self-hosted functions we can't see and can't debug

Categories

(DevTools :: Debugger, defect, P2)

x86_64
Windows 7
defect

Tracking

(firefox25 wontfix, firefox26 wontfix, firefox27 fixed, firefox28 fixed)

RESOLVED FIXED
Firefox 28
Tracking Status
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: WeirdAl, Assigned: past)

Details

(Keywords: testcase, Whiteboard: [qa-])

Attachments

(3 files)

Self-hosted functions like Array.prototype.forEach are a pain point for debugging, because the debugger steps through these functions without showing us what's happening.  It's likely these self-hosted functions should be treated as native code, hidden from us.
needinfo? gijs for the testcase, per his request.
Flags: needinfo?(gijskruitbosch+bugs)
Attached file Testcase
Trivial testcase. Step into shows various bits of "forEach (self-hosted:388)" or such. This is confusing. These should be blackboxed by default and possibly not un-black-boxable (unless we have source for them, anyhow).
Flags: needinfo?(gijskruitbosch+bugs)
CC'ing people who might know how to fix this.
Keywords: testcase
When self-hosted functions were first introduced we blocked them from showing up as sources in the frontend, but we apparently missed the stepping case. We should always just black box sources with a "self-hosted" source URL.

I had previously hoped that chrome debugging would allow debugging even that code, but I'm not sure if Debugger.Source objects are created for those. If not, we should just black box it even in the case of chrome debugging.
I think this should suffice.
Attachment #8334261 - Flags: review?(nfitzgerald)
Assignee: nobody → past
Status: NEW → ASSIGNED
Priority: -- → P2
Comment on attachment 8334261 [details] [diff] [review]
Black-box self hosted functions

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

::: toolkit/devtools/server/actors/script.js
@@ +3745,5 @@
>  /**
>   * Must be a class property because it needs to persist across reloads, same as
>   * the breakpoint store.
>   */
>  ThreadSources._blackBoxedSources = new Set();

Drive-by nit (I'm the worst!)
You could simply = new Set(["self-hosted"]);
Attachment #8334261 - Flags: review?(nfitzgerald) → review+
Landed with Victor's simplification (thanks!):
https://hg.mozilla.org/integration/fx-team/rev/1260d25e5c27
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1260d25e5c27
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 28
This happens even in Firefox 24 and 25, and it's extremely annoying when dealing with .forEach and other built-ins. I strongly suggest we uplift this to at least Aurora, if not Beta. Panos, what do you think?
Flags: needinfo?(past)
(...although it might be too late for both, sadly)
Sure we can uplift it. It's not a recent regression, but the fix is trivial.
Flags: needinfo?(past)
This is the final version as landed.
Comment on attachment 8343106 [details] [diff] [review]
Black-box self hosted functions v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): this has been happening ever since self-hosted functions landed, probably about a year ago.
User impact if declined: stepping through Array.prototype.forEach code will land at stack frames that can't be displayed, and the source in the editor will not correspond to the selected stack frame.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): no risk, trivial patch.
String or IDL/UUID changes made by this patch: none
Attachment #8343106 - Flags: approval-mozilla-beta?
Attachment #8343106 - Flags: approval-mozilla-aurora?
Comment on attachment 8343106 [details] [diff] [review]
Black-box self hosted functions v2

The ship for beta is sailed, so a- for that branch. Given the low risk approving on aurora.
Attachment #8343106 - Flags: approval-mozilla-beta?
Attachment #8343106 - Flags: approval-mozilla-beta-
Attachment #8343106 - Flags: approval-mozilla-aurora?
Attachment #8343106 - Flags: approval-mozilla-aurora+
Panos, does this have or need tests?
Flags: needinfo?(past)
Flags: in-testsuite?
There is some coverage in existing xpcshell tests, but it's not something that we worry too much about.
Flags: needinfo?(past)
Flags: in-testsuite? → in-testsuite+
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.