this.onSelected is not a function

RESOLVED FIXED in Firefox 65

Status

defect
P3
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: jlast, Assigned: yjzuo6, Mentored)

Tracking

({good-first-bug})

unspecified
Firefox 65

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
console.error: "TypeError: this.onSelected is not a function: onDetachedFront@resource://devtools/client/inspector/computed/computed.js:1458:5\nemit@resource://devtools/shared/event-emitter.js:178:15\nemit@resource://devtools/shared/event-emitter.js:255:5\n_onMutations@resource://devtools/client/framework/selection.js:105:7\nemit@resource://devtools/shared/event-emitter.js:178:15\nemit@resource://devtools/shared/event-emitter.js:255:5\nWalkerFront<.getMutations</<@resource://devtools/shared/fronts/inspector.js:422:7\n"
TypeError: this.onSelected is not a function: onDetachedFront@resource://devtools/client/inspector/computed/computed.js:1458:5
emit@resource://devtools/shared/event-emitter.js:178:15
emit@resource://devtools/shared/event-emitter.js:255:5
_onMutations@resource://devtools/client/framework/selection.js:105:7
emit@resource://devtools/shared/event-emitter.js:178:15
emit@resource://devtools/shared/event-emitter.js:255:5
WalkerFront<.getMutations</<@resource://devtools/shared/fronts/inspector.js:422:7
This happens when this.onSelected() is called from the onDetachedFront method. And onDetachedFront is called here as a result of an event being emitted.
The thing is, this.onDetachedFront isn't being bound to the instance `this`, so calling this.onSelected() in there can't work.
So it's probably a one liner to fix this. It would be nice to know when this happens though.
Component: Inspector → Computed Styles Inspector
This might be a good first bug. And I'm willing to mentor anyone who would like to attempt to fix it. I've already given some information in my previous comments, but here is more info:

- before starting, please read through our contribution guide: http://docs.firefox-dev.tools/
- once you've got the code ready and know how to make and reload changes in Firefox, you will want to open file /devtools/client/inspector/computed/computed.js and look inside this class ComputedViewTool
This is where you'll find the onSelected and onDetachedFront methods I mentioned previously.
Mentor: pbrosset
Keywords: good-first-bug
Priority: -- → P3
Whiteboard: [lang=js]
(Assignee)

Comment 3

5 months ago
Hi, I'm willing to fix this!
(Assignee)

Comment 4

5 months ago
I believe I have fixed it, creating a patch now!
(Assignee)

Comment 5

5 months ago
I can't reproduce when it happens though, seems as though ComputedViewTool is only called in inspector.js: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector.js#858

Not sure what the conditions are to reproduce this
Thanks. I'm assigning the bug to you now.
Indeed, we don't have steps to reproduce this error unfortunately. However the error stack does show enough information for us to fix this, and looking at the code it's understandable that this might fail under certain conditions.
I'll take a look at your patch in a bit.
Assignee: nobody → yjzuo6
Status: NEW → ASSIGNED
Comment on attachment 9024632 [details] [diff] [review]
Bug 1506604 - Fix 'this' instance bind in onDetachedFront when calling this.onSelected(). r=pbro.patch

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

::: devtools/client/inspector/computed/computed.js
@@ +1457,4 @@
>    },
>  
>    onDetachedFront: function() {
> +    const self = this;

Unfortunately this won't be enough to fix it.
The error callstack in this bug shows that `this` is not the instance of the class when `onDetachedFront` is being called.
What's happening here is that `onDetachedFront` is being called as a result of the event `detached-front` being emitted.
Indeed, you can see that we add a listener a bit above like this:
`this.inspector.selection.on("detached-front", this.onDetachedFront);`

So, by the time `onDetachedFront` is being called when the event is emitted, `this` doesn't point to the instance of the class. So assigning `self = this` isn't going to change that.

However, what's missing is binding the function to the instance so that `this` is always the instance when the function is called.

If you look inside the constructor, you will see that we do this for `onSelected`:
`this.onSelected = this.onSelected.bind(this);`

Well, we should also do this for `onDetached`.

Does that make sense?
Attachment #9024632 - Flags: review?(pbrosset)
(Assignee)

Comment 9

5 months ago
(In reply to Patrick Brosset <:pbro> from comment #8)
> Comment on attachment 9024632 [details] [diff] [review]
> Bug 1506604 - Fix 'this' instance bind in onDetachedFront when calling
> this.onSelected(). r=pbro.patch
> 
> Review of attachment 9024632 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/inspector/computed/computed.js
> @@ +1457,4 @@
> >    },
> >  
> >    onDetachedFront: function() {
> > +    const self = this;
> 
> Unfortunately this won't be enough to fix it.
> The error callstack in this bug shows that `this` is not the instance of the
> class when `onDetachedFront` is being called.
> What's happening here is that `onDetachedFront` is being called as a result
> of the event `detached-front` being emitted.
> Indeed, you can see that we add a listener a bit above like this:
> `this.inspector.selection.on("detached-front", this.onDetachedFront);`
> 
> So, by the time `onDetachedFront` is being called when the event is emitted,
> `this` doesn't point to the instance of the class. So assigning `self =
> this` isn't going to change that.
> 
> However, what's missing is binding the function to the instance so that
> `this` is always the instance when the function is called.
> 
> If you look inside the constructor, you will see that we do this for
> `onSelected`:
> `this.onSelected = this.onSelected.bind(this);`
> 
> Well, we should also do this for `onDetached`.
> 
> Does that make sense?

Ah I see, thank you for your help, I'll whip a new patch tonight with the correct changes, my bad!
(Assignee)

Comment 10

5 months ago
Attachment #9024632 - Attachment is obsolete: true
Attachment #9025138 - Flags: review?(pbrosset)
(Assignee)

Comment 11

5 months ago
Patch submitted! Hope I did it correctly and can contribute to more bug fixes :)
Attachment #9025138 - Flags: review?(pbrosset) → review+
(In reply to JZ from comment #11)
> Patch submitted! Hope I did it correctly and can contribute to more bug
> fixes :)
Yes you did. Thanks for working on this. I have just R+'d the patch which means it's now reviewed and accepted. I will push it to our CI environment to check if all tests still pass. And then once that's done, we can push it to the mozilla-central repository so it can be part of Firefox!
TRY is green (apart from some unrelated failures). So let's ask for this patch to be checked-in to one of the integration branches. If the patch "sticks" there, then the bug will be marked as resolved and the patch will be applied to mozilla-central.

So basically that means you are done :) Thanks a lot for the fix!
If you are interested in more, here's another quick good-first-bug: bug 1506792.
Keywords: checkin-needed

Comment 15

5 months ago
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/25325172bcc8
bind onDetachedFront to proper instance. r=pbro
Keywords: checkin-needed

Comment 16

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25325172bcc8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Assignee)

Comment 17

5 months ago
(In reply to Patrick Brosset <:pbro> from comment #14)
> TRY is green (apart from some unrelated failures). So let's ask for this
> patch to be checked-in to one of the integration branches. If the patch
> "sticks" there, then the bug will be marked as resolved and the patch will
> be applied to mozilla-central.
> 
> So basically that means you are done :) Thanks a lot for the fix!
> If you are interested in more, here's another quick good-first-bug: bug
> 1506792.

Really appreciate your guidance, merci pour ton aide!
You need to log in before you can comment on or make changes to this bug.