Closed Bug 1360310 Opened 8 years ago Closed 7 years ago

False positive ghost windows on RSS reader sites

Categories

(Core :: DOM: Core & HTML, defect, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

(Keywords: memory-leak, Whiteboard: Not a Firefox leak)

Attachments

(2 files, 1 obsolete file)

memory-report.json.gz
183.12 KB, application/x-gzip
Details
Bug 1360310 - Make ghost window reporter use tab groups instead of eTLD.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details 
Mossop is seeing some ghost windows in a recent Nightly even without the profiler enabled.
See Also: → 1360280
Attached file memory-report.json.gz — 
Here is a memory report. Only one ghost window so far which seems like a frame related to feedly which is still open in my tabs.
(In reply to Dave Townsend [:mossop] from comment #1)
> Here is a memory report. Only one ghost window so far which seems like a
> frame related to feedly which is still open in my tabs.

Hmm that could just be a bogus heuristic for ghost windows. We don't count a window that is not being displayed as a ghost window if it is same origin with another window that is open, but maybe in this case it is still keeping it alive somehow.
The question is if it only happens with windows opened by feedly, and do they go away if you close feedly. If the answer to both is "yes" then maybe this isn't a real issue. We could try to improve the heuristic somehow.
Does it go away if you close feedly and wait a bit?
Flags: needinfo?(dtownsend)
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Does it go away if you close feedly and wait a bit?

Yes it does. Maybe this shouldn't be reported as a ghost window at all?
Flags: needinfo?(dtownsend)
(In reply to Dave Townsend [:mossop] from comment #5)
> Yes it does. Maybe this shouldn't be reported as a ghost window at all?

Yeah, it shouldn't, but I'm not sure how easily we can figure that out.
Summary: Ghost windows in current nightly (without profiler enabled) → Ghost window in current nightly (without profiler enabled) on RSS site
Whiteboard: [MemShrink][qf] → Not a Firefox leak
Well, it is a leak in the web site (or at least seems like so). Sounds like similar issue what Google Reader had - leaking window objects of iframes until the top level page was loaded.
We try to optimize such windows from cycle collector's graph, but they are still leaks, just not from browser code. 
In general it would be hard to detect what kind of ghost is caused by a leak in browser code and what kind of leak caused by a web site.
Worth to report this to Feedly.
We could make ghost window detection smarter, by adding a rule like, if window A is being displayed, and contains a JS reference to window B, then window B is not a ghost window. In bug 816784, Ting is adding a cache with information about which compartments point to other compartments, so we could hopefully do that cheaply.
Depends on: 816784
Summary: Ghost window in current nightly (without profiler enabled) on RSS site → False positive ghost windows on RSS reader sites
Assignee: nobody → continuation
Priority: -- → P2
This is a basic prototype of a JS api function that does a flood fill from an initial set of compartments, following through compartments that it has wrappers to.

It takes advantage of the two levels of hash tables that are used for wrappers.

The idea is that you'd give it the set of top level windows for each tab. Any content window not in the resulting set is a ghost window. Maybe that is reasonable.
Olli made a comment in an email about this approach. Just pasting it here so we remember it:

    Not quite right. There can easily be C++ edges keeping a window object alive without JS from other compartments pointing to it.
    Just do something like
    e = new Event("foo");
    iframe.contentWindow.dispatchEvent(e);
    iframe.remove();
    Now event.target, which is very much C++ only thingie, keeps Window object alive, and it is the other JS global scope keeping the event object alive.
Olli also said we could maybe take advantage of tab groups for this. Is there some way to know if the tab for a tab group is still open?
That comment was about explicitly unlinking windows. Not sure it would help in this case.
But anyhow, TabGroup  doesn't seem to have such API, but adding would mean just iterating through the windows and checking if one has still docshell attached, I guess.
(In reply to Olli Pettay [:smaug] from comment #13)
> But anyhow, TabGroup  doesn't seem to have such API, but adding would mean
> just iterating through the windows and checking if one has still docshell
> attached, I guess.

I could iterate over all of the active windows and record their tab group (define these as the "active tab groups"), then consider any content window not in an active tab group as a ghost window, if it has been in that state for a minute. Does that make sense?
Flags: needinfo?(bugs)
Oh, right, if we want to explicitly destroy some windows, we could destroy those ones which belong to a TabGroup which doesn't have anything active. That should be safe.
Flags: needinfo?(bugs)
Attachment #8920737 - Attachment is obsolete: true
Blocks: 1388834
Comment on attachment 8929169 [details]
Bug 1360310 - Make ghost window reporter use tab groups instead of eTLD.

https://reviewboard.mozilla.org/r/200448/#review205784

::: dom/base/nsWindowMemoryReporter.cpp:600
(Diff revision 1)
>  
>    MOZ_COLLECT_REPORT(
>      "ghost-windows", KIND_OTHER, UNITS_COUNT, ghostWindows.Count(),
>  "The number of ghost windows present (the number of nodes underneath "
>  "explicit/window-objects/top(none)/ghost, modulo race conditions).  A ghost "
> -"window is not shown in any tab, does not share a domain with any non-detached "
> +"window is not shown in any tab, is not in a tab with any non-detached "

"is not in a tab group"?
Attachment #8929169 - Flags: review?(bzbarsky) → review+
Yeah, I was trying to avoid technical lingo, but I should be precise. "tab group" is close enough to "tab" that people can probably figure out what it means.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d171efe2eba5
Make ghost window reporter use tab groups instead of eTLD. r=bz
https://hg.mozilla.org/mozilla-central/rev/d171efe2eba5
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
This appears to have caused the 95% of ghost windows on telemetry to go from 1 to 0. Somehow, the mean is barely affected. I guess the 99% of users might have hundreds of ghost windows.
(the mean is around 0.5)
Here's a telemetry evolution link: https://mzl.la/2zLBRYH
Do we want to uplift this to 58 or can it ride 59?
status-firefox57: --- → wontfix
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Flags: needinfo?(continuation)
There's no reason to uplift this. It just changes a measurement.
status-firefox58: affected → wontfix
status-firefox-esr52: unaffected → wontfix
Flags: needinfo?(continuation)
I was a bit suspicious, but the data from the measurement dashboard is a little more helpful:
https://mzl.la/2iOzlJQ

So 3.74% of sessions see one or more ghost windows. And some people do have a ton of them. 0.07% of users have more than 127 ghost windows. So that explains the mean I guess (which itself has been dropping each day).

Since we probably don't care about the number of ghost windows people have, the 3.74% measurement is probably the most useful one here.
Great, thanks. Yeah, I think that now that we shouldn't have false positives from cross-domain iframes being leaked by a page, the number of users with ghost windows makes sense as a metric.
So what are we measuring as a ghost these days? Only leaks once a tab is closed?
Aren't we then missing temporary leaks?
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #30)
> So what are we measuring as a ghost these days? Only leaks once a tab is
> closed?
> Aren't we then missing temporary leaks?

But "ghost windows" are a tool for us to find browser leaks.  Its confusing to try to use them also to find content leaks in sites.  We should provide tools to find those as well, but it would be nice to have some easy way to quickly tell the difference between clearly browser problems and possibly site problems.
(In reply to Olli Pettay [:smaug] (ni?s and f?s processed after r?s) from comment #30)
> So what are we measuring as a ghost these days? Only leaks once a tab is
> closed?
> Aren't we then missing temporary leaks?

Those still show up as detached right?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: