Add logging of devtools event emitter emit calls

RESOLVED FIXED in Firefox 30

Status

defect
RESOLVED FIXED
6 years ago
Last year

People

(Reporter: miker, Assigned: miker)

Tracking

Trunk
Firefox 30
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

It is often very difficult to follow the flow of events in devtools code, especially because of our prolific use of promises.

We should add devtools.dump.emit=false (true only for tests) as this allows us to log extra details to test logs.

e.g.
17:42:28     INFO -  EMITTING: select(webconsole)
17:42:28     INFO -  EMITTING: changed([object XULElement] (tab))
17:42:28     INFO -  EMITTING: webconsole-selected([object Object])
...
17:25:25     INFO -  EMITTING: markuploaded()
17:25:25     INFO -  EMITTING: before-new-node-front([Front for domnode/conn28.domnode37] (BODY), inspector-open)
17:25:25     INFO -  EMITTING: new-node-front([Front for domnode/conn28.domnode37] (BODY), inspector-open)
17:25:25     INFO -  EMITTING: inspector-ready([object Object])
17:25:25     INFO -  EMITTING: toolbox-ready([object Object])
17:25:25     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/devtools/inspector/test/browser_inspector_bug_699308_iframe_navigation.js | found the iframe element
17:25:25     INFO -  EMITTING: before-new-node-front([Front for domnode/conn28.domnode37] (BODY), test)
17:25:25     INFO -  EMITTING: new-node-front([Front for domnode/conn28.domnode37] (BODY), test)
17:25:25     INFO -  EMITTING: select(inspector)
17:25:25     INFO -  EMITTING: changed([object XULElement] (tab))

...
17:42:30     INFO -  EMITTING: destroyed()
17:42:30     INFO -  EMITTING: toolbox-destroyed(TabTarget:null)
Posted patch Patch v1 (obsolete) — Splinter Review
We should wait for Joe's feedback to see if it is okay to have this on for default when testing.
Attachment #8377885 - Flags: review?(fayearthur)
Status: NEW → ASSIGNED
Posted patch Patch v1.01 (obsolete) — Splinter Review
Removed a \n
Attachment #8377885 - Attachment is obsolete: true
Attachment #8377885 - Flags: review?(fayearthur)
Attachment #8377887 - Flags: review?(fayearthur)
Posted patch Patch V2 (obsolete) — Splinter Review
Now no longer logs in tests by default
Attachment #8377887 - Attachment is obsolete: true
Attachment #8377887 - Flags: review?(fayearthur)
Attachment #8377895 - Flags: review?(fayearthur)
Comment on attachment 8377895 [details] [diff] [review]
Patch V2

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

"hearth" -> "harth" in commit (:

Worried about the big try/catch:

::: browser/devtools/shared/event-emitter.js
@@ +163,5 @@
> +              dump(")");
> +            }
> +          }
> +        } catch(e) {
> +          // Do nothing

We should avoid a big try/catch if we can. What was the reason for it?
Posted patch Addressed comments (obsolete) — Splinter Review
(In reply to Heather Arthur [:harth] from comment #4)
> Comment on attachment 8377895 [details] [diff] [review]
> Patch V2
> 
> Review of attachment 8377895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> "hearth" -> "harth" in commit (:
> 

Sorry, no brain today.

> Worried about the big try/catch:
> 
> ::: browser/devtools/shared/event-emitter.js
> @@ +163,5 @@
> > +              dump(")");
> > +            }
> > +          }
> > +        } catch(e) {
> > +          // Do nothing
> 
> We should avoid a big try/catch if we can. What was the reason for it?

We don't, removed and just generally tidied.
Attachment #8377895 - Attachment is obsolete: true
Attachment #8377895 - Flags: review?(fayearthur)
Attachment #8378104 - Flags: review?(fayearthur)
Posted patch Added more info to log (obsolete) — Splinter Review
Now the logging looks like this:

EMITTING: BC_update/<() -> emit(breadcrumbs-updated, [Front for domnode/conn0.domnode29] (DIV#output)) from resource:///modules/devtools/inspector/breadcrumbs.js:695
Attachment #8378104 - Attachment is obsolete: true
Attachment #8378104 - Flags: review?(fayearthur)
Attachment #8378597 - Flags: review?(fayearthur)
Bug 976679 moves event-emitter to toolkit, so you'll need to rebase if that lands before.
Mike, you told me to hold off on reviewing this. Is it ready now?
Flags: needinfo?(mratcliffe)
Yup, this is ready for review.
Attachment #8378597 - Attachment is obsolete: true
Attachment #8378597 - Flags: review?(fayearthur)
Attachment #8382940 - Flags: review?(fayearthur)
Flags: needinfo?(mratcliffe)
We need the try / catch in order to avoid logging info about dead objects (exception). As this is an option enabled by flipping the devtools.dump.emit pref I think we should be fine.
Attachment #8382940 - Attachment is obsolete: true
Attachment #8382940 - Flags: review?(fayearthur)
Attachment #8383038 - Flags: review?(fayearthur)
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #10)
> Created attachment 8383038 [details] [diff] [review]
> event-emitter-logging-974056.patch
> 
> We need the try / catch in order to avoid logging info about dead objects
> (exception). As this is an option enabled by flipping the devtools.dump.emit
> pref I think we should be fine.

It'd be a good to keep it as small as possible. Can we only wrap the statement that would throw the exception?
Any part of the for loop that goes through any passed arguments can throw so that is about the smallest we can make it.
Attachment #8383038 - Attachment is obsolete: true
Attachment #8383038 - Flags: review?(fayearthur)
Attachment #8384690 - Flags: review?(fayearthur)
Attachment #8384690 - Flags: review?(fayearthur) → review+
https://hg.mozilla.org/mozilla-central/rev/6a73824daf37
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Depends on: 982707
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.