Closed Bug 1049248 Opened 7 years ago Closed 6 years ago

Tracer on about:config gives TypeError: location is undefined

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 35

People

(Reporter: jryans, Assigned: past)

References

Details

Attachments

(1 file, 3 obsolete files)

STR:

1. Enable the tracer
2. Go to about:config
3. Open the Debugger

You'll start seeing these stacks at ~1/sec:

notify event 'traces' threw an exception: TypeError: location is undefined
Stack: TracerView.prototype<._createView@chrome://browser/content/devtools/debugger-panes.js:1444:5
TracerView.prototype<.addTrace@chrome://browser/content/devtools/debugger-panes.js:1422:16
Tracer.prototype._onCall@chrome://browser/content/devtools/debugger-controller.js:1566:1
Tracer.prototype.onTraces@chrome://browser/content/devtools/debugger-controller.js:1546:9
eventSource/aProto.emit@resource://gre/modules/devtools/dbg-client.jsm:189:9
DebuggerClient.prototype.onPacket/<@resource://gre/modules/devtools/dbg-client.jsm:922:9
resolve@resource://gre/modules/devtools/deprecated-sync-thenables.js:40:40
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:20:43
then@resource://gre/modules/devtools/deprecated-sync-thenables.js:58:9
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:864:1
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:545:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:83:14
Line: 1444, column: 4
Assignee: nobody → jjurovych
Attached patch bug-1049248.patch (obsolete) — Splinter Review
Attachment #8471861 - Flags: review?(nfitzgerald)
Comment on attachment 8471861 [details] [diff] [review]
bug-1049248.patch

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

So the problem stems from installing the tracer's onEnterFrame hook when there aren't any traces being recorded?

It seems like the proper solution would be to delay installing the tracer's the hook until there are traces being recorded, and to uninstall the hook whenever all existing traces have ended. This would also use less resources when we aren't actively tracing.

Am I missing something?

::: toolkit/devtools/server/actors/tracer.js
@@ +259,5 @@
>    onEnterFrame: function(aFrame) {
>      if (aFrame.script && aFrame.script.url == "self-hosted") {
>        return;
>      }
> +    // Do not send packets at all if no traces has been requested

Nit: "if no traces *have* been requested." Period too, please.

Another nit: empty line above the comment as well, please.

@@ +262,5 @@
>      }
> +    // Do not send packets at all if no traces has been requested
> +    let requestedTraces = Object.keys(this._requestsForTraceType)
> +                            .filter(k => !!this._requestsForTraceType[k]);
> +    if (!requestedTraces.length) {

I think it would be easier (and more efficient) to check |this._activeTraces.size|.
Attachment #8471861 - Flags: review?(nfitzgerald)
Attached patch bug-1049248-2.patch (obsolete) — Splinter Review
This is a prettier check whether tracing is active.
Alternatively we can install/uninstall onEnterFrame hook in onStartTrace/onStopTrace, but imo this is easier to read and is reasonable fast.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=17247bff9c45
Attachment #8471861 - Attachment is obsolete: true
Attachment #8478720 - Flags: review?(nfitzgerald)
Comment on attachment 8478720 [details] [diff] [review]
bug-1049248-2.patch

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

I'd still prefer to delay setting the hooks until we actually have traces going, but I'd be fine to r+ this if it fixes the failure and has a regression test so we know its actually fixed.
Attachment #8478720 - Flags: review?(nfitzgerald)
Duplicate of this bug: 1068516
Stealing as it is getting very annoying and it's already in Aurora.
Attachment #8490702 - Flags: review?(nfitzgerald)
Assignee: jjurovych → past
Status: NEW → ASSIGNED
Attachment #8490702 - Flags: review?(nfitzgerald) → review+
There were some test timeouts in slow platforms, so I made sure the test forces JS execution in order to get traces. This version appears to work well on try:

https://tbpl.mozilla.org/?tree=Try&rev=0830e3dc4e5e
Attachment #8478720 - Attachment is obsolete: true
Attachment #8490702 - Attachment is obsolete: true
Attachment #8491447 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/26fede342996
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.