function grips' definition site should be a new request type, not on the object grip

RESOLVED FIXED in Firefox 28

Status

P2
normal
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 28
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [debugger-docs-needed][qa-])

Attachments

(3 attachments, 3 obsolete attachments)

We need to start source mapping the location of function grips.
Making the function grips return a promise, makes createValueGrip return a promise, makes half of the debugger server need to be rewritten in a gross way. So much churn for something so little.

Instead, allowing you to return a packet with a (nested) property be a promise and we can deep resolve packets before sending them.
Created attachment 806379 [details] [diff] [review]
wip part 1 - allow promises to be nested in packets

test_profiler.js seems to be failing locally, haven't dug too deep in yet.
Blocks: 771597
Priority: -- → P2
Talked with Jim and Panos, going to split this info out into its own request.
Summary: function grips' url and line aren't source mapped → function grips' definition site should be a new request type, not on the object grip
Whiteboard: [debugger-docs-needed]
Created attachment 827712 [details] [diff] [review]
part 1 - RDP changes
Attachment #806379 - Attachment is obsolete: true
Attachment #827712 - Flags: review?(past)
Created attachment 827730 [details] [diff] [review]
part 2 option 1 - fix onEventListeners in the object actor

This is one of two options. The second option is coming soon.

So this patch just adds the definition site to the function grips in the "eventListeners" response.

Pros:

* No need to change frontend code

* No extra round trip request/response for the definition site info

Cons:

* Bit of code bloat

* Why even have a "definitionSite" request type if we are going to go around it all the time in the server?
Attachment #827730 - Flags: review?(past)
Created attachment 827735 [details] [diff] [review]
part 2 option 2 - request definitionSite before adding listeners to the view

So this patch makes the debugger controller request the definitionSite of each listener's function after it gets the list of event listeners, but before it adds them to the view.

--------------------

Option 3 would be for the view to partially render without the url, and have it request the definitionSite and incrementally update the UI when it gets a response back. This would give the fastest initial UI response for the user, but

a) it might be jarring to have the UI update incrementally (or maybe not)

b) it would require enough changes to browser_dbg_break-on-events-03.js that I didn't want to do it unless we decided this was definitely the correct path forward.
Attachment #827735 - Flags: review?(past)
The more I think about it, the more I am leaning towards option 3... but I'd like to hear more opinions.

Victor, what do you think?
Flags: needinfo?(vporof)
Duplicate of this bug: 902163
(In reply to Nick Fitzgerald [:fitzgen] from comment #6)
> Created attachment 827735 [details] [diff] [review]
> 
> So this patch makes the debugger controller request the definitionSite of
> each listener's function after it gets the list of event listeners, but
> before it adds them to the view.
> 

This looks like the easiest thing to implement, code-wise, breaking the least amount of tests, making our lifes as programmers easy. However, it might make the Events tab look like (actually, even *be*, on remote connections) "slow", for large numbers of event listeners.

> --------------------
> 
> Option 3 would be for the view to partially render without the url, and have
> it request the definitionSite and incrementally update the UI when it gets a
> response back. This would give the fastest initial UI response for the user,
> but
> 

This definitely requires a bit more work, and being careful about things, but the UI won't be jumpy (no reflows, due to how the widget works, except for setting labels). Tests will be tricky, but I don't think they'll be that hard to fix. 

I am ok with both options, however slightly leaning towards option 3. Can definitely be postponed for a followup if time is an issue.
Flags: needinfo?(vporof)
Comment on attachment 827735 [details] [diff] [review]
part 2 option 2 - request definitionSite before adding listeners to the view

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

::: browser/devtools/debugger/debugger-controller.js
@@ +1403,5 @@
> +        DevToolsUtils.reportException("scheduleEventListenersFetch", msg);
> +        return;
> +      }
> +
> +      promise.all(aResponse.listeners.map(listener => {

This is very cute.
Attachment #827730 - Attachment is obsolete: true
Attachment #827730 - Flags: review?(past)
Attachment #827735 - Attachment is obsolete: true
Attachment #827735 - Flags: review?(past)
Going to implement option 3.
Created attachment 829000 [details] [diff] [review]
part 2: frontend changes
Attachment #829000 - Flags: review?(vporof)
Comment on attachment 829000 [details] [diff] [review]
part 2: frontend changes

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

Pretty dense, but it looks like it'll do the trick. I'll need to have another pass through it, please address the comments and ask me again for r?.

::: browser/devtools/debugger/debugger-controller.js
@@ +49,5 @@
>    CONDITIONAL_BREAKPOINT_POPUP_HIDING: "Debugger:ConditionalBreakpointPopupHiding",
>  
>    // When event listeners are fetched or event breakpoints are updated.
>    EVENT_LISTENERS_FETCHED: "Debugger:EventListenersFetched",
> +  EVENT_LISTENER_URL_FETCHED: "Debugger:EventListenerUrlFetched",

Is this event necessary? I don't see it being used in tests.

@@ +1439,5 @@
>      // Add all the listeners in the debugger view event linsteners container.
>      for (let listener of aResponse.listeners) {
> +      let deferred = promise.defer();
> +      urlsFetched.push(deferred.promise);
> +      gThreadClient.pauseGrip(listener.function).getDefinitionSite(aResponse => {

I'd prefer having all of this gripping and getting outside of the loop and after the view items were committed, to make sure things are not stutterring while the view is populated. Something like this:

const outstanding = [];

// ...
for (let listener of aResponse.listeners) {
  let deferred = promise.defer();
  listener.url = deferred.promise;
  outstanding.push([listener, deferred]);
  DebuggerView.EventListeners.addListener(listener, { staged: true });
}

// ...
DebuggerView.EventListeners.commit();

// explain here what's happening with urls and why
for (let [listener, deferred] of outstanding) {
  gThreadClient.pauseGrip(listener.function).getDefinitionSite(... => {
     deferred.resolve/reject(...);
  });
}

return outstanding.map(([, deferred]) => deferred.promise);


Do you think the above makes more sense?

@@ +1453,5 @@
>  
>      // Flushes all the prepared events into the event listeners container.
>      DebuggerView.EventListeners.commit();
> +
> +    return promise.all(urlsFetched);

Hmm, this is indeed resolved when all the urls are fetched, but does that necessarily mean the view was updated as well? Are the tests depending on EVENT_LISTENERS_FETCHED assuming the UI already contains all the necessary information? (things used to be sync, now they're not etc.). It's probably fine, but just making sure you've thought about it.

::: browser/devtools/debugger/debugger-panes.js
@@ +1590,1 @@
>        aItem.attachment.type == type);

Nit: you can keep this predicate on a single line now.

@@ +1598,5 @@
> +        if (this._updateExistingItem(url, type, selector)) {
> +          return;
> +        }
> +        // No item for this event and url; we need to create it.
> +        const thisItem = this._addItem(type, selector, promise.resolve(url),

Why do you need promise.resolve(url)? Can't you just send the url string?

@@ +1609,5 @@
> +      });
> +      return;
> +    }
> +
> +    // There is not already an item for this type. We will go ahead and create a

A bit of funny wording here imo. "There is no item for this type yet" sounds better?

@@ +1617,5 @@
> +                                   aOptions.staged);
> +
> +    url.then(url => {
> +      if (this._updateExistingItem(url, type, selector)) {
> +        this.removeChild(thisItem);

Where is removeChild defined? Why is this necessary now?

@@ +1626,5 @@
> +      window.emit(EVENTS.EVENT_LISTENER_URL_FETCHED, thisItem);
> +    }).then(null, error => {
> +      DevToolsUtils.reportException("EventListenersView#addListener",
> +                                    error);
> +      this.removeChild(thisItem);

Ditto.

@@ +1628,5 @@
> +      DevToolsUtils.reportException("EventListenersView#addListener",
> +                                    error);
> +      this.removeChild(thisItem);
> +    });
> +  },

Whooh. This function became a bit all over the place now... Can you dry things up a bit, especially the "add new item or update existing item then set the url attachment" bits?

@@ +1711,5 @@
> +    const pushedItem = this.push([itemView.container, aActor, group], {
> +      staged: aStaged, /* stage the item to be appended later? */
> +      attachment: {
> +        url: null,
> +        urlPromise: aUrl,

urlPromise isn't being used anywhere afaict.

@@ +1712,5 @@
> +      staged: aStaged, /* stage the item to be appended later? */
> +      attachment: {
> +        url: null,
> +        urlPromise: aUrl,
> +        actor: aActor,

Also, why is the actor both added as the item's value and a property in the item's attachment object? Now, hovering the item would show a tooltup with the actor's id :)

@@ +1723,5 @@
> +    });
> +
> +    // If the item was staged, it wasn't returned by |push|, so we have to find
> +    // it again.
> +    return pushedItem || this.getItemForPredicate(aItem => {

Why is this necessary? Why should the event item be returned to the outside world from this function?

ps: getItemForPredicate should return items even if they're staged.

::: browser/devtools/shared/widgets/ViewHelpers.jsm
@@ +281,5 @@
>    getStr: function(aName) {
> +    try {
> +      return this.stringBundle.GetStringFromName(aName);
> +    } catch (e) {
> +      e.stack = e.stack || Error().stack;

Can't use safeErrorString or whatever it's called here?
Attachment #829000 - Flags: review?(vporof) → review-
Comment on attachment 827712 [details] [diff] [review]
part 1 - RDP changes

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

::: toolkit/devtools/server/actors/script.js
@@ +2754,5 @@
> +   */
> +  onDefinitionSite: function OA_onDefinitionSite(aRequest) {
> +    if (this.obj.class != "Function") {
> +      return {
> +        from: this.actorID,

Generally you don't need to add the "from" property to the return packet, it will be added automatically.

::: toolkit/devtools/server/tests/unit/test_objectgrips-12.js
@@ +47,5 @@
> +}
> +
> +function test_definition_site(func, obj) {
> +  func.getDefinitionSite(({ error, url, line, column }) => {
> +    do_check_true(!error);

Even: do_check_false(error);
Attachment #827712 - Flags: review?(past) → review+
Comment on attachment 829000 [details] [diff] [review]
part 2: frontend changes

After talking with victor in irc, we are going back to option 2.
Attachment #829000 - Attachment is obsolete: true
Attachment #827735 - Attachment is obsolete: false
Comment on attachment 827735 [details] [diff] [review]
part 2 option 2 - request definitionSite before adding listeners to the view

All tests still pass locally :)
Attachment #827735 - Flags: review?(vporof)
(In reply to Panos Astithas [:past] from comment #14)
> ::: toolkit/devtools/server/tests/unit/test_objectgrips-12.js
> @@ +47,5 @@
> > +}
> > +
> > +function test_definition_site(func, obj) {
> > +  func.getDefinitionSite(({ error, url, line, column }) => {
> > +    do_check_true(!error);
> 
> Even: do_check_false(error);

Can't because it isn't false, its undefined.
Oops, you're right. Mea culpa.
Attachment #827735 - Flags: review?(vporof) → review+
Backed out in https://hg.mozilla.org/integration/fx-team/rev/fd3869f9c83f for Windows and Linux failing like https://tbpl.mozilla.org/php/getParsedLog.php?id=30381255&tree=Fx-Team
Whiteboard: [debugger-docs-needed][fixed-in-fx-team] → [debugger-docs-needed]
note that part 1 is in and also merged to mozilla-central https://hg.mozilla.org/mozilla-central/rev/a976c1645d70 but part 2 was backed out
Tomcat, this is both patches that were backed out.
Can't repro locally (as expected, since I am on OSX), so here is a try push to make sure I am not crazy: https://tbpl.mozilla.org/?tree=Try&rev=f78cf50bec32
Created attachment 8334872 [details] [diff] [review]
part 3: fix the event-listeners test;

The event-listeners test needed the same fix as debugger-controller, since it bypasses the UI and tests the protocol interaction directly.
Attachment #8334872 - Flags: review?(nfitzgerald)
Assignee: nfitzgerald → past
Status: NEW → ASSIGNED
Comment on attachment 8334872 [details] [diff] [review]
part 3: fix the event-listeners test;

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

Thanks for taking a look at this on your linux machine, Panos! Looking forward to those try results...
Attachment #8334872 - Flags: review?(nfitzgerald) → review+
Only unrelated oranges in the try run.
https://hg.mozilla.org/mozilla-central/rev/b0217b81bee2
https://hg.mozilla.org/mozilla-central/rev/bb2535b52c74
https://hg.mozilla.org/mozilla-central/rev/15e0dbb08a2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [debugger-docs-needed][fixed-in-fx-team] → [debugger-docs-needed]
Target Milestone: --- → Firefox 28

Updated

5 years ago
Whiteboard: [debugger-docs-needed] → [debugger-docs-needed][qa-]

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.