Closed
Bug 1506604
Opened 5 years ago
Closed 5 years ago
this.onSelected is not a function
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox65 fixed)
RESOLVED
FIXED
Firefox 65
Tracking | Status | |
---|---|---|
firefox65 | --- | fixed |
People
(Reporter: jlast, Assigned: yjzuo6, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(1 file, 1 obsolete file)
873 bytes,
patch
|
pbro
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•5 years ago
|
||
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
Comment 2•5 years ago
|
||
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.
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
Attachment #9024632 -
Flags: review?(pbrosset)
Comment 7•5 years ago
|
||
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 8•5 years ago
|
||
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)
(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 years ago
|
||
Attachment #9024632 -
Attachment is obsolete: true
Attachment #9025138 -
Flags: review?(pbrosset)
Assignee | ||
Comment 11•5 years ago
|
||
Patch submitted! Hope I did it correctly and can contribute to more bug fixes :)
Updated•5 years ago
|
Attachment #9025138 -
Flags: review?(pbrosset) → review+
Comment 12•5 years ago
|
||
(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!
Comment 13•5 years ago
|
||
Here's the TRY push URL to follow how this patch is doing on CI: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9102b39ad8e2a1ce8ca18f2cddbd1610640843fb
Comment 14•5 years ago
|
||
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 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/25325172bcc8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox65:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Assignee | ||
Comment 17•5 years 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!
Updated•3 years ago
|
Component: Inspector: Computed → Inspector
You need to log in
before you can comment on or make changes to this bug.
Description
•