Closed Bug 1491939 Opened 1 year ago Closed 1 year ago

Clean up prefs for streams

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(4 files)

Streams are enabled by a pref, and at the moment the pref can be flipped at any time and the behavior of existing realms when you've done that is pretty random.

The patch in bug 1385890 further adds C++ APIs that assume, if you've got a stream object, that streams are enabled. Which currently isn't true, leading to crashes in some tests.

In general the current scheme seems quite hard to explain or reason about. Easier to say:

-   Streams are enabled in a realm if the pref is enabled when that realm is created; you'll see stream constructors on a global (and so on) iff streams are enabled for that realm.

-   Changing the pref later doesn't affect existing realms.

SharedArrayBuffer already works this way; let's do that.
Summary: Enable streams in a realm iff enabled at realm creation → Clean up prefs for streams
I just found out that if you set dom.streams.enabled=true and javascript.options.streams=false, open a web page, then paste this into the console:

fetch(document.location.href).then(x => console.log(x.body))

...the content process crashes near NULL in the JS engine, just like the tests mentioned in comment 0 above.

This is because Response.body is enabled by dom.streams.enabled, and its implementation calls into the JS engine via C++ APIs that just assume the stream constructors and so forth will be there.

So I'm going to fix that in this bug; and also collapse everything to a single pref (deleting dom.streams.enabled and dom.workers.options.streams, keeping javascript.options.streams).
OK, this change makes it hard to get some existing tests to work, because they dynamically enable the prefs from script, after the global is already created.

https://searchfox.org/mozilla-central/source/dom/tests/mochitest/fetch/test_readableStreams.html#20-21

Setting the prefs before creating the iframe wouldn't help; the tests exercise `(await fetch("/")).body` in `self` (the current global) as well as the iframe.

baku, Is there a way to have the test harness set the pref before running the test?

Or have the test itself navigate to a second URL, after setting the prefs but before running the test code, to create a fresh global?

I'm not sure what other options we've got. Just flipping the pref on for everyone is no good -- we'll want a pref for other stream features as we go.
Flags: needinfo?(amarchesini)
> baku, Is there a way to have the test harness set the pref before running
> the test?

Yes: in the mochitest.ini add, in the [DEFAULT] section: prefs= prefname=true.
For example: https://searchfox.org/mozilla-central/rev/0b8ed772d24605d7cb44c1af6d59e4ca023bd5f5/dom/animation/test/mochitest.ini#2-7

alternatively, you can create a test, which set the pref, and then runs any new test inside an iframe. This is what this test does: https://searchfox.org/mozilla-central/source/dom/tests/mochitest/fetch/test_readableStreams.html#20-21
Flags: needinfo?(amarchesini)
The new error is thrown if, for example, you try to create a ReadableStream via
the C++ API while streams are disabled.

Depends on D6553
Comment on attachment 9011156 [details]
Bug 1491939 - Part 4: Enable streams on a per-realm basis. Drop dom.streams.enabled and dom.workers.options.streams; use only javascript.options.streams. r?baku

Andrea Marchesini [:baku] has approved the revision.
Attachment #9011156 - Flags: review+
Comment on attachment 9011155 [details]
Bug 1491939 - Part 3: Centralize configuration of JS realm options from prefs. r?baku

Andrea Marchesini [:baku] has approved the revision.
Attachment #9011155 - Flags: review+
Comment on attachment 9011153 [details]
Bug 1491939 - Part 1: Remove remaining traces of build-time ENABLE_STREAMS option. r?jwalden

Jeff Walden [:Waldo] has approved the revision.
Attachment #9011153 - Flags: review+
Comment on attachment 9011154 [details]
Bug 1491939 - Part 2: Throw, rather than crash, on trying to instantiate a runtime-disabled class. r?jwalden

Jeff Walden [:Waldo] has approved the revision.
Attachment #9011154 - Flags: review+
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a2f09c609ce
Part 1: Remove remaining traces of build-time ENABLE_STREAMS option. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/72e279c3075d
Part 2: Throw, rather than crash, on trying to instantiate a runtime-disabled class. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/b3ef3ff4ef21
Part 3: Centralize configuration of JS realm options from prefs. r=baku
https://hg.mozilla.org/integration/autoland/rev/8ef0b50e1d1b
Part 4: Enable streams on a per-realm basis. Drop dom.streams.enabled and dom.workers.options.streams; use only javascript.options.streams. r=baku
Assignee: nobody → jorendorff
You need to log in before you can comment on or make changes to this bug.