Closed Bug 633879 Opened 9 years ago Closed 9 years ago

quora.com bloats out of control, part 4: wrapper proto keeps old windows alive

Categories

(Core :: XPConnect, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: gal, Assigned: gal)

References

Details

(Keywords: memory-leak, regression, Whiteboard: [hardblocker], fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #630072 +++

Numbers for linux32 bit running w/ Flash:

                             Memory according to Ubuntu
about:home                 - 27M
quora.com                  - 52M
after 200 quora DOMWindows - 430-460M

The initial load of Quora isn't bad. It's actually quite heavy in all browsers. But after clicking around for 200 DOMWindows in one tab in a debug build, our heap has grown out of control. Reading my debug logs, I don't see DocShell bloat, but I do see DOMWindow bloat.

I'll attach my shutdown log in a debug build. Notice the build shuts down cleanly (no leaks), but DOMWindows with very old serial numbers are still alive at shutdown. The last 10 DOMWindows destroyed are a grab bag:

--DOMWINDOW == 10 (0xad2cacc) [serial = 44] [outer = (nil)] [url = http://www.quora.com/Is-Mexico-a-part-of-North-America-or-Central-America]
--DOMWINDOW == 9 (0xbac54c4) [serial = 89] [outer = (nil)] [url = http://www.quora.com/Urban-Dictionary]
--DOMWINDOW == 8 (0x9a671834) [serial = 62] [outer = (nil)] [url = http://www.quora.com/Why-might-someone-think-that-being-called-a-gentleman-would-be-bad]
--DOMWINDOW == 7 (0x987fdc6c) [serial = 123] [outer = (nil)] [url = http://www.quora.com/Android-Market/Are-there-any-tools-that-help-Android-developers-monitor-sales]
--DOMWINDOW == 6 (0xc139a54) [serial = 100] [outer = (nil)] [url = http://www.quora.com/If-I-owned-a-domain-name-can-I-use-Posterous-iPhone-app-to-blog-there]
--DOMWINDOW == 5 (0xb3d40c4) [serial = 78] [outer = (nil)] [url = http://www.quora.com/Why-is-calamansi-not-in-the-Merriam-Webster-dictionary]
--DOMWINDOW == 4 (0x94c7485c) [serial = 133] [outer = (nil)] [url = http://www.quora.com/What-are-the-best-social-media-monitoring-product-websites]
WARNING: NS_ENSURE_TRUE(!(mAsyncExecutionThread)) failed: file /home/sayrer/dev/mozilla-central/storage/src/mozStorageConnection.cpp, line 674
--DOMWINDOW == 3 (0x9a31ecc) [serial = 7] [outer = (nil)] [url = http://www.quora.com/Business-Development]
--DOMWINDOW == 2 (0x9ecf0a74) [serial = 112] [outer = (nil)] [url = http://www.quora.com/Where-do-I-find-investors-for-an-Internet-startup-idea]
--DOMWINDOW == 1 (0xa4af277c) [serial = 200] [outer = (nil)] [url = http://www.quora.com/Business-Development]
--DOMWINDOW == 0 (0xe3781e4) [serial = 192] [outer = (nil)] [url = http://www.quora.com/Business-Development]
I split off part 4. Fix for this in a sec.
OS: Windows CE → All
Hardware: x86 → All
Summary: quora.com bloats out of control, part 4: wrapper proto keeps old window salive → quora.com bloats out of control, part 4: wrapper proto keeps old windows alive
Assignee: nobody → gal
Patch in hand, asserts in chrome tests. Trying to reproduce.
Attached patch patch (obsolete) — Splinter Review
Sent to try.
Whiteboard: [hardblocker][has patch]
isCrossCompartmentWrapper is broken, I removed it and replaced it with ->flags() & CROSS_COMPARTMENT. Sent to try again.
Here is the assert jst got:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1297664639.1297667145.2171.gz

Command line to reproduce:

python runtests.py --chrome --test-path=js/src/xpconnect/tests/chrome --debugger=gdb

The wrapper is:

(gdb) x/a $2->getProxyHandler()
0x7ffff7bb9e40 <_ZN25JSCrossCompartmentWrapper9singletonE>:	0x7ffff7b006d0 <_ZTV25JSCrossCompartmentWrapper+16>

(gdb) p $2->getProto()
$3 = (JSObject *) 0x7fffd2128478
(gdb) call js_DumpObject($3)
object 0x7fffd2128478
class 0x7ffff7b6fa00 Proxy
flags: delegate
not native
proto <XPC_WN_ModsAllowed_NoCall_Proto_JSClass object at 0x7fffd212b9f8>
parent <Window object at 0x7fffd2123798>
slots:

The prototype of the wrapper is a window object. This is a weird sandbox wrapper. This is really ugly. Wrappers should only have wrappers as proto. I will stop the parent-adjustment-along-the-proto-chain once we hit a non-wrapper object.
Attached patch patchSplinter Review
Attachment #512092 - Attachment is obsolete: true
Comment on attachment 512123 [details] [diff] [review]
patch

peterv, can you please review the change in the bug? (abort if we hit a non-wrapper along the proto chain) and then land the patch for me? And then lets see if we still leak with your editor patch. I can't reproduce any InstallTrigger-related leaks with this applied (plus the patch I already landed on TM). If you still see us keep InstallTrigger alive I need some STR. You said its the getter somehow?
Attachment #512123 - Flags: review?(peterv)
Can I help schedule a call between all the relevant folks on these 4 bugs, so that we work through remaining issues more synchronously?  Time isn't on our side, and hours-latency of bugmail responses isn't on the side of time.
Peterv and I have a round-the-clock thing going on here. I handed off at 2am. I am back to collect results (see other bug, leak is fixed).
I've been trying to track down a remaining window leak, but still working on figuring it out.

        --[XPCWrappedNativeScope::mGlobalJSObject]-> 0x7fffccdac2d0 [JS Object (Window) (global=7fffccdac2d0)]
        --[InstallTrigger getter]-> 0x7fffccdd0c38 [JS Object (Proxy) (global=7fffccdac2d0)]
        --[wrappedObject]-> 0x7fffd1a7de70 [JS Object (Function) (global=7fffdeb35f30)]
        --[upvars[0]]-> 0x7fffdebba0d0 [JS Object (Proxy) (global=7fffe0734510)]
        --[proto]-> 0x7fffd4762e38 [JS Object (Proxy) (global=7fffe0734510)]
        --[proto]-> 0x7fffd4762dd0 [JS Object (Proxy) (global=7fffe0734510)]
        --[wrappedObject]-> 0x7fffd47701b0 [JS Object (Object) (global=7fffd4770120)]
        --[parent]-> 0x7fffd4770120 [JS Object (Window) (global=7fffd4770120)]
        --[xpc_GetJSPrivate(obj)]-> 0x7fffd80ba3c0 [XPCWrappedNative (Window)]
        --[mIdentity]-> 0x7fffd4427878 [nsGlobalWindow]
Comment on attachment 512123 [details] [diff] [review]
patch

r=jst, and gal says mrbkap reviewed all of this except the last bit about stopping when finding a non-wrapper on the proto chain.
Attachment #512123 - Flags: review?(peterv) → review+
Andreas' patch landed:

http://hg.mozilla.org/mozilla-central/rev/9e7a0e7f1f61

Leaving bug open for now to track any remaining issues, or feel free to file new bugs if that's more appropriate here.
Whiteboard: [hardblocker][has patch] → [hardblocker]
http://hg.mozilla.org/tracemonkey/rev/f8bec3eead64

Closing, please file new bugs.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [hardblocker] → [hardblocker], fixed-in-tracemonkey
Depends on: 634236
Keywords: mlk
No longer blocks: mlk2.0
You need to log in before you can comment on or make changes to this bug.