Closed Bug 1451985 Opened 6 years ago Closed 6 years ago

Ghost windows from dropbox shared image link page

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Performance Impact low
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- fixed
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: bkelly, Assigned: mccr8)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: memory-leak, Whiteboard: [MemShrink:P1])

Attachments

(1 file)

STR:

1. Open a shared dropbox link like this:

https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0

2. Close the tab.
3. Observe that it takes many seconds for the tab to close.

I have also gotten a ghost window with dropbox pages like this a few times tonight, but I can't reproduce regularly.
Like this ghost window:

184.11 MB (100.0%) -- explicit
├───64.20 MB (34.87%) -- window-objects
│   ├──32.85 MB (17.84%) ++ top(https://www.irccloud.com/irc/freenode/channel/whatwg, id=4294967409)
│   ├──14.10 MB (07.66%) -- top(none)/ghost/window(https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0)
│   │  ├───9.09 MB (04.94%) ++ js-compartment(https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0)
│   │  ├───3.88 MB (02.11%) ── layout/style-sheets
│   │  └───1.13 MB (00.61%) ++ (3 tiny)
│   ├───8.82 MB (04.79%) ++ top(https://www.fastmail.com/mail/Inbox/7f81296bf1caa8c8-f163802768u10708?u=9509408f, id=4294967407)
│   └───8.43 MB (04.58%) ++ top(https://getpocket.com/a/queue/list/, id=4294967424)
Whiteboard: [MemShrink]
Unknown ref on script element:

bkelly@valen:/mnt/c/devel/tmp/cclogs$ /srv/heapgraph/find_roots.py cc-edges.9132.1522983791.log 000002804EDEAC00
Parsing cc-edges.9132.1522983791.log. Done loading graph. 

0000028061877100 [FragmentOrElement (xhtml) script https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0]
    --[[via hash] mListenerManager]--> 000002804DB87310 [EventListenerManager]
    --[mListeners event=onerror listenerType=4 [i]]--> 000002805EE4BD00 [CallbackObject]
    --[mIncumbentGlobal]--> 000002804EDEAC00 [nsGlobalWindowInner # 4294967610 inner https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0]

    Root 0000028061877100 is a ref counted object with 1 unknown edge(s).
    known edges:
       00000280697D7AC0 [JS Object (HTMLScriptElement)] --[UnwrapDOMObject(obj)]--> 0000028061877100
       000002804DF9D580 [FragmentOrElement (xhtml) head https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0] --[mAttrsAndChildren[i]]--> 0000028061877100
I can see that dropbox script resources are getting alternate bytecode data out of http cache.  From about:cache:

  key: 	https://cfl.dropboxstatic.com/static/compiled/js/modules/clean/react/file_action_button_type.min-vflOclJZd.js
  alt-data: 	1;580,javascript/moz-bytecode-20180405220026

I'll try disable bytecode cache for a while.
Blocks: GhostWindows
It still happens with bytecode cache disabled:

│   ├──39.40 MB (17.51%) -- top(none)/ghost
│   │  ├──23.74 MB (10.55%) ++ (16 tiny)
│   │  └──15.66 MB (06.96%) -- window(https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0)
│   │     ├──10.64 MB (04.73%) -- js-compartment(https://www.dropbox.com/s/21kyjktup1omju9/ghost-cat.jpg?dl=0)
│   │     │  ├───8.23 MB (03.66%) -- classes
│   │     │  │   ├──3.28 MB (01.46%) ++ class(Object)/objects
│   │     │  │   ├──2.78 MB (01.24%) -- class(Function)/objects
│   │     │  │   │  ├──2.65 MB (01.18%) ── gc-heap
│   │     │  │   │  └──0.13 MB (00.06%) ── malloc-heap/slots
│   │     │  │   └──2.17 MB (00.97%) ++ (5 tiny)
│   │     │  └───2.41 MB (01.07%) ++ (6 tiny)
│   │     ├───3.89 MB (01.73%) ── layout/style-sheets
│   │     └───1.13 MB (00.50%) ++ (3 tiny)

Note, I also wiped my http cache and restarted to ensure there was not any old bytecode left in use.
Note, opening 10+ dropbox tabs and closing them in various states of loading repeatedly seems to trigger the leak for me.
Note, those 16 "tiny" ghost windows are:

│   │  │  ├───1.74 MB (00.77%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=t3vp3yn7ncf5#73ki9ncqqou8)
│   │  │  ├───1.74 MB (00.77%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=yq9frk3cu98z#i99y58skfiwd)
│   │  │  ├───1.74 MB (00.77%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=xrf5mtq0fu5o#w5naznx65aks)
│   │  │  ├───1.73 MB (00.77%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=mspcbydv6efw#yp7yrznxwbe9)
│   │  │  ├───1.66 MB (00.74%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=3ef2831omlex#65zr1a9arc31)
│   │  │  ├───1.66 MB (00.74%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=8c9cbvpp3iaz#pf2mw33ct60t)
│   │  │  ├───1.65 MB (00.73%) ++ window(https://www.google.com/recaptcha/api2/bframe?hl=en&v=v1522045847408&k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&cb=gobjhazg8u4#psoxn0mneq7t)
│   │  │  ├───1.59 MB (00.71%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=2ikvx5o7ikmj#i99y58skfiwd)
│   │  │  ├───1.59 MB (00.71%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=fprnnppi5swd#w5naznx65aks)
│   │  │  ├───1.58 MB (00.70%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=wmjahppxylq#pf2mw33ct60t)
│   │  │  ├───1.57 MB (00.70%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=lyypxwa96cli#yp7yrznxwbe9)
│   │  │  ├───1.57 MB (00.70%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=lufeylj1i2v3#psoxn0mneq7t)
│   │  │  ├───1.57 MB (00.70%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=5j8iuur1tpwi#65zr1a9arc31)
│   │  │  ├───1.57 MB (00.70%) ++ window(https://www.google.com/recaptcha/api2/anchor?k=6LdnLyIUAAAAAOiGPtddh-g3KiJRoDGGPD-6dqXo&co=aHR0cHM6Ly93d3cuZHJvcGJveC5jb206NDQz&hl=en&v=v1522045847408&size=invisible&cb=k21lijj5miby#73ki9ncqqou8)
│   │  │  ├───0.72 MB (00.32%) ++ window(https://marketing.dropbox.com/s/%3Atkey/%2Apath?referrer=)
Summary: dropbox shared image link page is very slow to close (and sometimes leak?) → dropbox shared image link page is very slow to close and sometimes leak
[qf] for the very slow tab close behavior.
Whiteboard: [MemShrink] → [MemShrink][qf]
Whiteboard: [MemShrink][qf] → [MemShrink:P1][qf]
Assignee: nobody → continuation
See Also: → 1453753
I can reproduce this by shift clicking the link in comment 0 to open about 5 or so tabs, then manually closing them by clicking on the X in the tabs as fast as I can. This seems to only leak one.
In a very slow refcount logging run, I had to wait until the little UI with the log in stuff at the top of the image loaded in before closing the tab. That seemed to make it reproduce more.
Ben, can you provide a profile for this bug we can do a better analysis.
Depends on: 1344302
Flags: needinfo?(bkelly)
Whiteboard: [MemShrink:P1][qf] → [MemShrink:P1][qf:f64][qf:p3]
(To be clear here, I'm only looking at the leak. I didn't notice any particular slowness.)

Using DMD heap scan mode, it looks like ScriptLoadRequest is holding a strong reference to the script element. ScriptLoadRequest is cycle collected, but does not report that field to the CC. Lo and behold, that load request is being held alive by the leaking script element...
I have a patch that fixes the leak for me, but I'll spin this off into a bug blocking this one. I don't know how a leak would cause page closing to be slow, and I haven't noticed that myself.
Summary: dropbox shared image link page is very slow to close and sometimes leak → Ghost windows from dropbox shared image link page
Ben suggested I just use this bug for the leak.
Keywords: memory-leak
I audited ScriptLoadRequest and ModuleLoadRequest and it looks like all of the other fields that should be reported to the cycle collector are.
Priority: -- → P3
I think this bug was around in May 2017 when bug 1362119 landed (and is probably even older), so it likely affects all branches. This looks like a leak Ben and I have been seeing off and on for quite a while, so we should backport this.
Priority: P3 → P1
Status: NEW → ASSIGNED
Flags: needinfo?(bkelly)
I wonder why this has become an issue only now if the relevant code is after all very old.
http://52.25.115.98/viewvc/main/mozilla/content/base/src/nsScriptLoader.cpp?annotate=1.63#l118
(In reply to Olli Pettay [:smaug] from comment #18)
> I wonder why this has become an issue only now if the relevant code is after
> all very old.

It is strange. I think Ben and I have been seeing similar leaks for at least a year or two. At least, windows leaking through script elements. See bug 1421681. It does seem to have happening more recently.
Comment on attachment 8969734 [details]
Bug 1451985 - Tell the cycle collector about ScriptLoadRequest::mElement.

https://reviewboard.mozilla.org/r/238548/#review244536
Attachment #8969734 - Flags: review?(amarchesini) → review+
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/de34f6d415df
Tell the cycle collector about ScriptLoadRequest::mElement. r=baku
We're about 2 weeks from the release of 60, and I don't see any evidence on telemetry that this is a wide scale problem, so let's just wont fix it for 60.
The page is setting a beforeunload event listener, and seems to be busy running a bunch of JS not long after painting, which explains why it's a bit slow to close.
I cloned this over to bug 1456241 to investigate slowness around closing the tab.
See Also: → 1456241
https://hg.mozilla.org/mozilla-central/rev/de34f6d415df
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Wow, this actually shows up as a distinct drop in ghost windows on Telemetry.

The mean content process GHOST_WINDOWS number drops from about 0.3 to more like 0.06 (the other measurements are all 0): https://mzl.la/2ralTRv

With the new scalar telemetry MAX_GHOST_WINDOWS (which measures the max value seen across an entire telemetry ping session), the 95th percentile goes from about 1 to 0, and the mean goes from around 0.7 to around 0.3 (the other measurements are all 0): https://mzl.la/2ramGSt
Andrew, do you want to request uplift to esr60?
Flags: needinfo?(continuation)
(In reply to Julien Cristau [:jcristau] from comment #27)
> Andrew, do you want to request uplift to esr60?

This doesn't seem to match the esr uplift criteria, as it is not a stability or security fix, and it does not fix a regression.
Flags: needinfo?(continuation)
This is a simple perf fix and we're very early in the ESR60 lifecycle. I think we should take this.
Flags: needinfo?(continuation)
Comment on attachment 8969734 [details]
Bug 1451985 - Tell the cycle collector about ScriptLoadRequest::mElement.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: see comment 29
User impact if declined: memory leaks, which can lead to performance problems
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): Very low. It is a simple patch.
String or UUID changes made by this patch: none
Flags: needinfo?(continuation)
Attachment #8969734 - Flags: approval-mozilla-esr60?
Comment on attachment 8969734 [details]
Bug 1451985 - Tell the cycle collector about ScriptLoadRequest::mElement.

fix memory leak, approved for 60.1esr
Attachment #8969734 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P3
Whiteboard: [MemShrink:P1][qf:f64][qf:p3] → [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.