Closed Bug 991934 Opened 6 years ago Closed 6 years ago

Leaked tabListChanged listeners on exit

Categories

(DevTools Graveyard :: WebIDE, defect, P1)

x86
macOS
defect

Tracking

(firefox29 unaffected, firefox30 verified, firefox31 verified)

VERIFIED FIXED
Firefox 31
Tracking Status
firefox29 --- unaffected
firefox30 --- verified
firefox31 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Bug 912900 made the App Manager start listening for "tabListChanged" to know about new remote browser tabs.

However, we currently add a new listener each time we update the tabs, which is exactly what that callback for the listener is!  This means you can quickly add many, many listeners. :(

Also, we don't remove them either!
After a few tab opens and closes, I saw Firefox crank up to 100% CPU and get very sluggish, so this is pretty bad.
Priority: -- → P1
Attachment #8401601 - Flags: feedback?(janx)
Comment on attachment 8401601 [details] [diff] [review]
Only listen for tabs once, and clean up too. r=paul

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

Awesome leak-bust! Thank you for fixing that, and sorry for my OCD.

::: browser/devtools/app-manager/device-store.js
@@ +33,5 @@
>    this._connection.on(Connection.Events.STATUS_CHANGED, this._onStatusChanged);
>    this._onTabListChanged = this._onTabListChanged.bind(this);
>    this._onStatusChanged();
>    return this;
> +};

Over-nit: Since you're fixing this, please also add the missing semi-colon at the very bottom of the file.

@@ +55,5 @@
>      this.object.tabs = [];
>    },
>  
>    _onStatusChanged: function() {
>      if (this._connection.status == Connection.Status.CONNECTED) {

Sorry if I don't understand what's happening here, but in the case of a status change to `Connection.Status.DESTROYED`, is there a chance that the `DESTROYED` listener gets called before the `STATUS_CHANGED` listener (as it is bound before)? If that's a possibility, `this._connection` might be null here and cause an error, so you'd need to bail out like in `_listTabs`. However, if we know that `STATUS_CHANGED` can never be called after `DESTROYED` you can disregard my concern.
Attachment #8401601 - Flags: feedback?(janx) → feedback+
(In reply to Jan Keromnes [:janx] from comment #4)
> Comment on attachment 8401601 [details] [diff] [review]
> Only listen for tabs once, and clean up too. r=paul
> 
> Review of attachment 8401601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Awesome leak-bust! Thank you for fixing that, and sorry for my OCD.
> 
> ::: browser/devtools/app-manager/device-store.js
> @@ +33,5 @@
> >    this._connection.on(Connection.Events.STATUS_CHANGED, this._onStatusChanged);
> >    this._onTabListChanged = this._onTabListChanged.bind(this);
> >    this._onStatusChanged();
> >    return this;
> > +};
> 
> Over-nit: Since you're fixing this, please also add the missing semi-colon
> at the very bottom of the file.

Haha, I actually did fix it, but then undid it, because some reviewers dislike mixing style changes in with others.  It is hard for me to resist making these changes while I happen to be in the same file...  It seems we share the same feelings, so I am happy to follow along. :)

> @@ +55,5 @@
> >      this.object.tabs = [];
> >    },
> >  
> >    _onStatusChanged: function() {
> >      if (this._connection.status == Connection.Status.CONNECTED) {
> 
> Sorry if I don't understand what's happening here, but in the case of a
> status change to `Connection.Status.DESTROYED`, is there a chance that the
> `DESTROYED` listener gets called before the `STATUS_CHANGED` listener (as it
> is bound before)? If that's a possibility, `this._connection` might be null
> here and cause an error, so you'd need to bail out like in `_listTabs`.
> However, if we know that `STATUS_CHANGED` can never be called after
> `DESTROYED` you can disregard my concern.

Well, the order[1] actually is:

DESTROYED
STATUS_CHANGED

but the DESTROYED listener removes the STATUS_CHANGED listener, so we won't run into the problem you are worrying about.

[1]: dxr.mozilla.org/mozilla-central/source/toolkit/devtools/client/connection-manager.js#249-250
Attachment #8401601 - Attachment is obsolete: true
Attachment #8401601 - Flags: review?(paul)
Comment on attachment 8402102 [details] [diff] [review]
Only listen for tabs once, and clean up too. r=paul, f=janx

Carrying over Jan's f+.
Attachment #8402102 - Flags: review?(paul)
Attachment #8402102 - Flags: feedback+
Attachment #8402102 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/eabfdab992c1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Comment on attachment 8402102 [details] [diff] [review]
Only listen for tabs once, and clean up too. r=paul, f=janx

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 912900
User impact if declined: Over time as the App Manager is used, the number of registered event listeners increases exponentially, making the whole browser quite slow.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: no
Attachment #8402102 - Flags: approval-mozilla-aurora?
Comment on attachment 8402102 [details] [diff] [review]
Only listen for tabs once, and clean up too. r=paul, f=janx

definitely do not want to slow down the browser - uplift!
Attachment #8402102 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite?
Keywords: verifyme
Ryan, I used the Win 7 64-bit environment set up in bug 912900 to verify this bug.
Using desktop Nightly 2014-04-03, I connect the LG-880 device to it and opened about 25 tabs in Fennec.
While opening the debug pages for them, I checked the CPU (around 10%) and Memory (about 500Mb) in Task Manager.

Using Firefox 31 beta 8, both CPU and Memory rates were lower (~350MB).

I didn't manage to hang the browser or see a higher CPU rate so maybe there is something more to do in order to reproduce the bug in the same way.

If that's the case, please let as know what else needs to be done. Or, if you think that the testing is enough and also you didn't experienced hangs after the fix, we will mark this as verified.
Flags: needinfo?(jryans)
Hmm, I'm not too sure why you were not able to reproduce the original issue...  It seems to me that you've done what would be needed to trigger the problem.

However, I think we would hear from users if the problem was still there, since browser hangs are quite noticeable when they do happen.  I think it's fine to say verified here.
Flags: needinfo?(jryans)
Flags: in-testsuite? → in-testsuite-
Verified fixed as per comment 14.
Status: RESOLVED → VERIFIED
Flags: in-qa-testsuite?(hskupin)
Keywords: verifyme
What was the reason that the in-testsuite? request has been denied? I don't see a description. Just switching the flag to '-' is not pretty helpful. If we could get a simple test, we could get possible leaks via the leak checks at the end of mochitest runs.
Flags: in-testsuite?
Flags: in-testsuite-
Flags: in-qa-testsuite?(hskupin)
Flags: in-qa-testsuite?
Product: Firefox → DevTools
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.