Closed Bug 1517667 Opened 1 year ago Closed 10 months ago

Leak from MessagePort::UpdateMustKeepAlive()

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 1552751
Tracking Status
firefox66 --- wontfix
firefox67 --- fix-optional
firefox68 --- affected

People

(Reporter: tsmith, Unassigned)

References

(Blocks 3 open bugs)

Details

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

Attachments

(6 files)

Attached file testcase.html
Not sure exactly where this belongs or if this in fact just a false positive.

Direct leak of 7168 byte(s) in 112 object(s) allocated from:
    #0 0x55804befbd93 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:146:3
    #1 0x7f0f0698bf44 in js_arena_malloc src/obj-firefox/dist/include/js/Utility.h:353:10
    #2 0x7f0f0698bf44 in js_pod_arena_malloc<unsigned char> src/obj-firefox/dist/include/js/Utility.h:535
    #3 0x7f0f0698bf44 in js_pod_malloc<unsigned char> src/obj-firefox/dist/include/js/Utility.h:540
    #4 0x7f0f0698bf44 in maybe_pod_malloc<unsigned char> src/js/src/vm/MallocProvider.h:53
    #5 0x7f0f0698bf44 in unsigned char* js::MallocProvider<JS::Zone>::pod_malloc<unsigned char>(unsigned long) src/js/src/vm/MallocProvider.h:89
    #6 0x7f0f06da273b in AllocateObjectBuffer<js::HeapSlot> src/js/src/gc/Nursery-inl.h:116:45
    #7 0x7f0f06da273b in js::NativeObject::growSlots(JSContext*, unsigned int, unsigned int) src/js/src/vm/NativeObject.cpp:412
    #8 0x7f0f06ea9efe in updateSlotsForSpan src/js/src/vm/NativeObject-inl.h:608:33
    #9 0x7f0f06ea9efe in setLastProperty src/js/src/vm/NativeObject-inl.h:646
    #10 0x7f0f06ea9efe in getChildDataProperty src/js/src/vm/Shape.cpp:349
    #11 0x7f0f06ea9efe in js::NativeObject::addDataPropertyInternal(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, unsigned int, unsigned int, js::ShapeTable*, js::ShapeTable::Entry*, js::AutoKeepShapeTables const&) src/js/src/vm/Shape.cpp:625
    #12 0x7f0f06db015f in addDataProperty src/js/src/vm/Shape-inl.h:392:10
    #13 0x7f0f06db015f in AddOrChangeProperty<IsAddOrChange::Add> src/js/src/vm/NativeObject.cpp:1443
    #14 0x7f0f06db015f in js::NativeDefineProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::PropertyDescriptor>, JS::ObjectOpResult&) src/js/src/vm/NativeObject.cpp:1748
    #15 0x7f0f06d1d3d7 in DefineDataProperty src/js/src/vm/JSObject.cpp:3028:10
    #16 0x7f0f06d1d3d7 in js::DefineDataProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::PropertyKey>, JS::Handle<JS::Value>, unsigned int) src/js/src/vm/JSObject.cpp:3066
    #17 0x7f0f073956d1 in DefineDataPropertyById src/js/src/jsapi.cpp:1975:10
    #18 0x7f0f073956d1 in DefineDataProperty(JSContext*, JS::Handle<JSObject*>, char const*, JS::Handle<JS::Value>, unsigned int) src/js/src/jsapi.cpp:2055
    #19 0x7f0f073969f5 in JS_DefineProperty(JSContext*, JS::Handle<JSObject*>, char const*, JS::Handle<JSString*>, unsigned int) src/js/src/jsapi.cpp:2099:10
    #20 0x7f0f0740a5c6 in DefineHelpProperty(JSContext*, JS::Handle<JSObject*>, char const*, char const*) src/js/src/jsfriendapi.cpp:237:10
    #21 0x7f0f07409f29 in JS_DefineFunctionsWithHelp(JSContext*, JS::Handle<JSObject*>, JSFunctionSpecWithHelp const*) src/js/src/jsfriendapi.cpp:265:12
    #22 0x7f0f06f85a3a in js::DefineTestingFunctions(JSContext*, JS::Handle<JSObject*>, bool, bool) src/js/src/builtin/TestingFunctions.cpp:6384:10
    #23 0x7f0f06956bfa in FinishObjectClassInit(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>) src/js/src/builtin/Object.cpp:2151:10
    #24 0x7f0f06c16e7d in js::GlobalObject::resolveConstructor(JSContext*, JS::Handle<js::GlobalObject*>, JSProtoKey, js::GlobalObject::IfClassIsDisabled) src/js/src/vm/GlobalObject.cpp:281:10
    #25 0x7f0f06e2bf88 in ensureConstructor src/js/src/vm/GlobalObject.h:163:12
    #26 0x7f0f06e2bf88 in getOrCreateObjectPrototype src/js/src/vm/GlobalObject.h:321
    #27 0x7f0f06e2bf88 in JS::GetRealmObjectPrototype(JSContext*) src/js/src/vm/Realm.cpp:1070
    #28 0x7f0efe18576b in mozilla::dom::EventTarget_Binding::CreateInterfaceObjects(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool) src/obj-firefox/dom/bindings/EventTargetBinding.cpp:1623:42
    #29 0x7f0efeb9234c in mozilla::dom::GetPerInterfaceObjectHandle(JSContext*, unsigned long, void (*)(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool), bool) src/dom/bindings/BindingUtils.cpp:4007:5
    #30 0x7f0efd89aa70 in GetProtoObjectHandle src/obj-firefox/dist/include/mozilla/dom/EventTargetBinding.h:724:12
    #31 0x7f0efd89aa70 in mozilla::dom::Window_Binding::GetNamedPropertiesObject(JSContext*) src/obj-firefox/dom/bindings/WindowBinding.cpp:19245
    #32 0x7f0efd899489 in mozilla::dom::Window_Binding::CreateInterfaceObjects(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool) src/obj-firefox/dom/bindings/WindowBinding.cpp:19519:42
    #33 0x7f0efeb9234c in mozilla::dom::GetPerInterfaceObjectHandle(JSContext*, unsigned long, void (*)(JSContext*, JS::Handle<JSObject*>, mozilla::dom::ProtoAndIfaceCache&, bool), bool) src/dom/bindings/BindingUtils.cpp:4007:5
    #34 0x7f0efd89322f in GetProtoObjectHandle src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:914:12
    #35 0x7f0efd89322f in bool mozilla::dom::CreateGlobal<nsGlobalWindowInner, &(mozilla::dom::Window_Binding::GetProtoObjectHandle(JSContext*))>(JSContext*, nsGlobalWindowInner*, nsWrapperCache*, JSClass const*, JS::RealmOptions&, JSPrincipals*, bool, JS::MutableHandle<JSObject*>) src/obj-firefox/dist/include/mozilla/dom/BindingUtils.h:2755
    #36 0x7f0efd8928e2 in mozilla::dom::Window_Binding::Wrap(JSContext*, nsGlobalWindowInner*, nsWrapperCache*, JS::RealmOptions&, JSPrincipals*, bool, JS::MutableHandle<JSObject*>) src/obj-firefox/dom/bindings/WindowBinding.cpp:19362:8
    #37 0x7f0efb58038b in CreateNativeGlobalForInner src/dom/base/nsGlobalWindowOuter.cpp:1605:8
    #38 0x7f0efb58038b in nsGlobalWindowOuter::SetNewDocument(nsIDocument*, nsISupports*, bool) src/dom/base/nsGlobalWindowOuter.cpp:1786
    #39 0x7f0f021f14c3 in nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, bool, bool, bool) src/layout/base/nsDocumentViewer.cpp:970:22
    #40 0x7f0f021f056f in nsDocumentViewer::Init(nsIWidget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) src/layout/base/nsDocumentViewer.cpp:716:10
    #41 0x7f0f056b051f in nsDocShell::SetupNewViewer(nsIContentViewer*) src/docshell/base/nsDocShell.cpp:8451:7
    #42 0x7f0f056aebc6 in nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*) src/docshell/base/nsDocShell.cpp:6345:17
    #43 0x7f0f056c08e9 in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIURI*, bool, bool) src/docshell/base/nsDocShell.cpp:7196:14
    #44 0x7f0efb57cafb in nsGlobalWindowOuter::SetInitialPrincipalToSubject() src/dom/base/nsGlobalWindowOuter.cpp:1361:18
    #45 0x7f0f0642a339 in nsWindowWatcher::OpenWindowInternal(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsIArray*, bool, bool, nsDocShellLoadState*, mozIDOMWindowProxy**) src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:1018:18
    #46 0x7f0f06430b73 in OpenWindow2 src/toolkit/components/windowwatcher/nsWindowWatcher.cpp:364:10
    #47 0x7f0f06430b73 in non-virtual thunk to nsWindowWatcher::OpenWindow2(mozIDOMWindowProxy*, char const*, char const*, char const*, bool, bool, bool, nsISupports*, bool, bool, nsDocShellLoadState*, mozIDOMWindowProxy**) src/toolkit/components/windowwatcher/nsWindowWatcher.cpp
Flags: in-testsuite?
Attached file lsan.supp
Attached file prefs.js
Attachment #9034329 - Attachment mime type: application/x-javascript → text/plain
Just to double check, that's the entirety of the LSan output? That makes it looks like the JS engine is leaking slots somehow.
Flags: needinfo?(twsmith)
Here is the full output(LSAN_OPTIONS=max_leaks=0).
Flags: needinfo?(twsmith)
Attachment #9034328 - Attachment mime type: application/octet-stream → text/plain

Hi Andrew, could you please take a look at the full output on comment 4 and see if it still looks like the JS engine leaking? Thanks!

Flags: needinfo?(continuation)

Oops, I actually looked over the full output, but then I forgot to comment here. Anyways, it just looks like a regular DOM leak, so I think DOM is the right place for this bug.

The test case involves MessageChannel, and I think I recall some possible issues with that causing leaks, so maybe that's at fault.

Flags: needinfo?(continuation)
Blocks: LSan
Whiteboard: [MemShrink]
Blocks: leak-fuzz
Whiteboard: [MemShrink] → [MemShrink:P2]

MessageChannel/MessagePort do not use JS slots, and I don't see these 2 classes in the leak-Sanitizier output.
Andrew, I think your first idea of JS leaking slots seems reasonable.

Flags: needinfo?(continuation)

There's some DOM stuff in the output. I'll try running the test case in a debug build. The BloatLog is easier to interpret for big leaks like this.

Assignee: nobody → continuation

I looked at the BloatView. There is a document and window leaking, and 2 MessagePorts, but no MessageChannel. I'll see what the CC logs say.

Flags: needinfo?(continuation)
Priority: -- → P2
Summary: LSan: Direct leak in [@ mozilla::dom::EventTarget_Binding::CreateInterfaceObjects] → Leak from MessagePort::UpdateMustKeepAlive()

To investigate this leak, I first looked at the cycle collector log. The DOM window is being held alive by a MessagePort.

0x7ffb2187a400 [MessagePort file:///home/amccreight/tests/1517667.html]

Root 0x7ffb2187a400 is a ref counted object with 1 unknown edge(s).
known edges:
   0x7ffb2187a640 [MessagePort file:///home/amccreight/tests/1517667.html] --[mUnshippedEntangledPort]--> 0x7ffb2187a400
   0x7ffb216d1400 [JS Object (MessagePort)] --[UnwrapDOMObject(obj)]--> 0x7ffb2187a400

There is a known reference from the reflector, another from the other MessagePort via the mUnshippedEntangledPort field, and one unknown reference.

Then I used DMD heap scan mode to find all references to one of the leaking MessagePorts in memory. There are only the two strong references known to the CC.

I then looked at the refcount log for this MessagePort, in an unoptimized build to get better stacks. I have attached an annotated version of this log. I also removed some of the stack frames that didn't seem useful.

Anyways, it looks like this message port is leaking because MessagePort::UpdateMustKeepAlive() calls AddRef() on the object, but Release() is never called. I looked at the refcount log for the other MessagePort, and it is basically the same thing, except that the other port has no reflector.

I think I've gotten as far as I can without knowing anything about MessagePort. Could you take a look again, Baku? Thanks.

Assignee: continuation → nobody
Flags: needinfo?(amarchesini)

I've debugged this issue a bit. What happens is this:

When the form is submitted, we free the inner objects:

#0 0x00007fa565b62248 in nsGlobalWindowInner::FreeInnerObjects(bool) (this=0x55e7d8e33c00, aForDocumentOpen=false) at /home/baku/Sources/m/barfoo/src/dom/base/nsGlobalWindowInner.cpp:1091
...
#30 0x00007fa56b3f3f14 in nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, nsIPrincipal*) (this=0x55e7d94c6660, aContent=0x55e7d8d9d080, aURI=0x55e7d942fb70, aTargetSpec=..., aFileName=..., aPostDataStream=0x0, aHeadersDataStream=0x0, aNoOpenerImplied=false, aDocShell=0x7ffe912777c0, aRequest=0x55e7d8d9d1c0, aIsUserTriggered=false, aTriggeringPrincipal=0x0) at /home/baku/Sources/m/barfoo/src/docshell/base/nsDocShell.cpp:12654
...
#32 0x00007fa5678ba183 in mozilla::dom::HTMLFormElement::SubmitSubmission(mozilla::dom::HTMLFormSubmission*) (this=0x55e7d8d9d080, aFormSubmission=0x55e7d9433490)
at /home/baku/Sources/m/barfoo/src/dom/html/HTMLFormElement.cpp:710

But then we create the MessagePort, which creates a loop with the entangled port:

#0 0x00007fa5687253e8 in mozilla::dom::MessagePort::UnshippedEntangle(mozilla::dom::MessagePort*) (this=0x55e7d95a71d0, aEntangledPort=0x55e7d8bfa7f0) at /home/baku/Sources/m/barfoo/src/dom/messagechannel/MessagePort.cpp:233
#1 0x00007fa568725213 in mozilla::dom::MessageChannel::Constructor(nsIGlobalObject*, mozilla::ErrorResult&) (aGlobal=0x55e7d8e33cf8, aRv=...) at /home/baku/Sources/m/barfoo/src/dom/messagechannel/MessageChannel.cpp:76

Note that CheckInnerWindowCorrectness() returns NS_OK because the MessagePort's window (globalObject) is the current inner window.

The problem is that, after this point, FreeInnerObjects() is not called again and the window is leaked.
I suspect that this problem can be reproduced with many other DOM APIs. In particular, anything that is released in FreeInnerObjects:

https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/dom/base/nsGlobalWindowInner.cpp#1246-1282

Smaug, should FreeInnerObjects() be called (also) at the end of the current JS execution instead?

Flags: needinfo?(amarchesini) → needinfo?(bugs)

Why should FreeInnerObjects be called the end of current JS execution?

Creating object should be work, I think, since throwing might cause unexpected issues. But worth to test what other browsers do. But perhaps the created object should be somehow dummy if the owner global is being destroyed.

Flags: needinfo?(bugs)

Why should FreeInnerObjects be called the end of current JS execution?

Right now, FreeInnerObjects() is called when JS executes form.submit(), but more things happen after that. New objects are created, localStorage and indexedDB can be used for instance. What's the point of this cleanup: https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/dom/base/nsGlobalWindowInner.cpp#1246-1282 ... if all of that can be re-initialize immediately after?

Creating object should be work, I think, since throwing might cause unexpected issues. But worth to test what other browsers do. But perhaps the created object should be somehow dummy if the owner global is being destroyed.

I don't think we should move this extra complexity in any possible object.

Btw, this problem is extremely similar to the worker shutdown. Maybe we can try to find a way to unify the 2 paths (long term project).

Flags: needinfo?(bugs)

(In reply to Andrea Marchesini [:baku] from comment #14)

Why should FreeInnerObjects be called the end of current JS execution?

Right now, FreeInnerObjects() is called when JS executes form.submit(), but
more things happen after that. New objects are created, localStorage and
indexedDB can be used for instance. What's the point of this cleanup:
https://searchfox.org/mozilla-central/rev/
4587d146681b16ff9878a6fdcba53b04f76abe1d/dom/base/nsGlobalWindowInner.
cpp#1246-1282 ... if all of that can be re-initialize immediately after?

But at which point would you call FreeInnerObjects. In principal cycle collector should be able to unlink everything, not some explicit "FreeFooBar" call

Flags: needinfo?(bugs)

But at which point would you call FreeInnerObjects. In principal cycle collector should be able to unlink everything, not some explicit "FreeFooBar" call

This is right. But FreeInnerObjects() is what calls DisconnectEventTargetObjects():
https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/dom/base/nsGlobalWindowInner.cpp#1216

And in DisconnectEventTargetObjects() we execute DisconnectFromOwner():
https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/dom/base/nsIGlobalObject.cpp#158-160

Many objects relay on ::DisconnectFromOwner() to break circular references.

It seems to me that actually the issue is in how we execute the form.submit(). This should be async:

https://html.spec.whatwg.org/#submit-mutate-action
"[...] Plan to navigate to parsed action."

https://html.spec.whatwg.org/#plan-to-navigate
"Each form element has a planned navigation, which is either null or a task; when the form is first created, its planned navigation must be set to null. In the behaviors described below, when the user agent is required to plan to navigate to a particular resource destination, it must run the following steps:
[...]
If the form has a non-null planned navigation, remove it from its task queue.
Let the form's planned navigation be a new task that consists of running the following steps:
Let the form's planned navigation be null.
Navigate target browsing context to destination. If replace is true, then target browsing context must be navigated with replacement enabled.
[...]
Queue the task that is the form's new planned navigation."

This is what I wrote so far. It makes the form submission async, as the spec says. But it introduces an extra about:blank load event and that breaks a few tests. Maybe somebody has time to continue this patch.

I might be able to take it after bug 1518442.

Component: DOM → DOM: Core & HTML
Duplicate of this bug: 1528457

Andrea and I looked at this a bit today. What's happening is that the form submit is trying to find a window target named "x", there isn't one, so we go to open a new window/tab. That calls into the window provider machinery, which spins the event loop before returning a window. That event loop spin ends up processing the timeout and doing window.close(), which tears down the window; then we unwind and create the MessageChannel.

So a simpler testcase might be:

<iframe></iframe>
<script>
  var win = frames[0];
  win.frameElement.remove();
  var chan = new win.MessageChannel();
</script>

or something along those lines.

I filed bug 1552505 on the event loop spinning thing.

This has been fixed by bug 1552751.

Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1552751
You need to log in before you can comment on or make changes to this bug.