Closed Bug 1557781 Opened 5 months ago Closed 4 months ago

Better life-time management for BodyStream/FetchStream

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Regressed 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

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.

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?

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.

Assignee: nobody → amarchesini

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

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

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?

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?

Priority: -- → P2
Flags: needinfo?(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.

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.

Flags: needinfo?(jorendorff)

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?

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.

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.

Attachment #9071852 - Attachment is obsolete: true
Attachment #9073223 - Attachment description: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - BodyStream, r?jonco → Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - BodyStream, r?jonco, r?smaug
Attachment #9073222 - Attachment description: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 2 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r?jonco → Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 2 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r?jonco, r?jorendorff
Attachment #9073223 - Attachment description: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - BodyStream, r?jonco, r?smaug → Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - BodyStream, r?jonco,smaug
Attachment #9073221 - Attachment is obsolete: true
Attachment #9073222 - Attachment description: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 2 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r?jonco, r?jorendorff → Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 1 - JSCLASS_PRIVATE_IS_NSISUPPORTS, r?jonco,jorendorff
Attachment #9073223 - Attachment description: Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 3 - BodyStream, r?jonco,smaug → Bug 1557781 - Better life-time management for BodyStream/FetchStream - part 2 - BodyStream, r?jonco,smaug
Pushed by amarchesini@mozilla.com:
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
Regressions: 1562891
Regressions: 1562930
Regressions: 1563215
Regressions: 1563179
Regressions: 1564821
Regressions: 1564895
Regressions: 1567419
You need to log in before you can comment on or make changes to this bug.