1557781 - Better life-time management for BodyStream/FetchStream

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Consider this js code:

var b = new Blob([""]); b.foobarExpando = b.stream().getReader(); b = null;

This leaks a BodyStream object until the current context (window or worker) is released.
This happens because BodyStream is not a cycle-collectable class.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The question is that when can we collect the stream. If nothing will ever read from it, and nothing outside points to the cycle where stream is in, it can be collected, but if something may read, then we can't collect before read has occurred, right?

Comment 2

[Andrea Marchesini [:baku]]

The reason why BodyStream is not CC, is because finalize() could be call on any thread. See:
https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/js/public/Stream.h#144-153

Discussing this issue with Jonco, he suggested to use JSCLASS_BACKGROUND_FINALIZE flag in here:
https://searchfox.org/mozilla-central/rev/fe7dbedf223c0fc4b37d5bd72293438dfbca6cec/js/src/builtin/Stream.cpp#3620-3623

After that, I'll make BodyStream CC and traverse/unlink the BodyStreamHolder. This should fix the issue.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We can keep background finalize and just do an asyncrelease on the "right" thread, right?

Comment 4

[Andrea Marchesini [:baku]]

A CC class cannot be thread-safe. We must be sure that the object doesn't receive AddRef()/Release() call.
Btw, bz, what you are suggesting is the current setup:

https://searchfox.org/mozilla-central/source/dom/fetch/FetchStream.cpp#251-258
https://searchfox.org/mozilla-central/source/dom/fetch/FetchStream.cpp#421-469

Comment 5

[Boris Zbarsky [:bzbarsky]]

No, the current setup does a Release() of the FetchStream/BodyStream on whatever thread finalize() was called on. And if that thread is not the main thread, it does refcounting of that object inside FetchStream::ReleaseObjects.

We could just have the finalizer queue a runnable over to the "main thread" (or whatever thread the object conceptually lives on), without doing any refcounting, that does the ReleaseObjects/Release dance there. If this is the only place that does off-thread refcounting, that would be enough to make it CCable, right?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

ok, so if we make BodyStream CCable, what keeps the objects alive long enough if read will happen and nothing outside the cycle keeps the stream alive?

(new Blob([""])).stream().getReader().read().then(callSomeFunction);
If GC/CC run before the read()'s promise is resolved, will callSomeFunction still get called?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1557781 - Better life-time management for BodyStream/FetchStream, r?jorendorff

This is a WIP. As discussed on IRC, the main issue is that finalize() can be
called at any time, even when the object has been already released by CC/GC.

Comment 8

[Jason Orendorff [:jorendorff]]

I talked with baku about this a lot in private chat. I can't prove it, but I'm sure there's no weak pointer or "registering" scheme that can help here. At least I'm very confident I'm not smart enough to trick the CC into doing exactly what I want, short of just telling the CC the truth about every edge.

Here the root problem is that the ReadableStream JSObject has a pointer to an XPCOM object (whatever implements ReadableStreamUnderlyingSource), but it doesn't tell the CC about that edge.

Comment 9

[Boris Zbarsky [:bzbarsky]]

but it doesn't tell the CC about that edge

Shouldn't we just fix that? That is, in CycleCollectedJSRuntime::NoteGCThingXPCOMChildren detect ReadableStream objects, get their source, have some way to get to CC info from ReadableStreamUnderlyingSource, and that should be good, no?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Alternately, since we only have one caller of JS::NewReadableExternalSourceStreamObject we could just assume that the underlying source is the thing it is. We do need a way to get the underlying source from a stream to do this; there doesn't seem to be API for it right now.

Comment 11

[Jason Orendorff [:jorendorff]]

Shouldn't we just fix that?

Yes, absolutely.

Right now, JSCLASS_PRIVATE_IS_NSISUPPORTS and a handful of special hacks in NoteGCThingXPCOMChildren are the ways we have to tell the CC about an edge from a JS object to an XPCOM object. At best, we introduce one nice, uniform way, and ditch the hacks. At worst, we add another special hack, a la comment 10.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 1 - no lock needed, r?smaug

Comment 13

[Andrea Marchesini [:baku]]

Attachment: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 1 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r?jonco,jorendorff

Depends on D35527

Comment 14

[Andrea Marchesini [:baku]]

Attachment: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 2 - BodyStream, r?jonco,smaug

Depends on D35528

Comment 15

[Andrea Marchesini [:baku]]

Attachment: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - JS::ReadableStreamReleaseCCObject, r?jorendorff

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b43fac4ccd1d
Better life-time management for BodyStream/FetchStream - part 1 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r=jonco,jorendorff
https://hg.mozilla.org/integration/autoland/rev/ea96413122a8
Better life-time management for BodyStream/FetchStream - part 2 - BodyStream, r=smaug
https://hg.mozilla.org/integration/autoland/rev/00eaa63cc4d7
Better life-time management for BodyStream/FetchStream - part 3 - JS::ReadableStreamReleaseCCObject, r=jorendorff

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b43fac4ccd1d
https://hg.mozilla.org/mozilla-central/rev/ea96413122a8
https://hg.mozilla.org/mozilla-central/rev/00eaa63cc4d7