many author-style-disabled-changed observers build up during a browser-chrome test run

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: ttaubert)

Tracking

(Blocks 1 bug)

unspecified
Firefox 29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox26 unaffected, firefox27 unaffected, firefox28+ fixed, firefox29 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

After several hundred tests in mochitest-browser-chrome test runs, I see ~1k observers for author-style-disabled-changed in about:memory.  The only place those are added is here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/content/content-sessionStore.js#323

and they are never removed.  They are weak observers, but that probably doesn't matter either, as that event is rather rare.

It looks like we'd really need to be notified when a frame script is destructed, so we could do some cleanup, but there's no such notification currently.
Duplicate of this bug: 961103
Depends on: 961111
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8361973 [details] [diff] [review]
0001-Bug-961106-Remove-observers-on-frame-script-unload.patch

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

It might be useful to comment that the gFrameTree observers don't need to be removed because gFrameTree will die with the content script, and it holds the observers. And the privacy transition thing is okay because it's on the docshell, which will die along with the tab.

::: browser/components/sessionstore/content/content-sessionStore.js
@@ +342,4 @@
>   */
>  let PageStyleListener = {
>    init: function () {
>      Services.obs.addObserver(this, "author-style-disabled-changed", true);

Do you mind changing the "weak" parameter to false. I don't think that setting it to true is helping us, and I think it would be good to reduce the number of places where we use weak observers.
Attachment #8361973 - Flags: review?(wmccloskey) → review+
Is this definitely new to FF27?  Should we be looking for a regression range here?
Yeah, this has been introduced with Fx 28.

(In reply to Bill McCloskey (:billm) from comment #3)
> It might be useful to comment that the gFrameTree observers don't need to be
> removed because gFrameTree will die with the content script, and it holds
> the observers. And the privacy transition thing is okay because it's on the
> docshell, which will die along with the tab.

Good idea.

> Do you mind changing the "weak" parameter to false. I don't think that
> setting it to true is helping us, and I think it would be good to reduce the
> number of places where we use weak observers.

Will do.
https://hg.mozilla.org/mozilla-central/rev/2235c85f6d6b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Comment on attachment 8361973 [details] [diff] [review]
0001-Bug-961106-Remove-observers-on-frame-script-unload.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930967
User impact if declined: Observers are leaked when closing tabs.
Testing completed (on m-c, etc.): Will be in tomorrow's nightly.
Risk to taking this patch (and alternatives if risky): Small low-risk patch.
String or IDL/UUID changes made by this patch: None.
Attachment #8361973 - Flags: approval-mozilla-aurora?
Attachment #8361973 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.