1491939 - Clean up prefs for streams

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Jason Orendorff [:jorendorff]]

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).

Comment 2

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bc252c9a43a1bdd749344244587c6a312a04dfd

Comment 3

[Jason Orendorff [:jorendorff]]

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.

Comment 4

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4b6a410458247ac1b8f2949b290ff693f2c54d9

Comment 5

[Andrea Marchesini [:baku]]

> 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

Comment 6

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c3e5354e2b14d20eec1d27a56e3ecd825cc1b8c

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1491939 - Part 1: Remove remaining traces of build-time ENABLE_STREAMS option. r?jwalden

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1491939 - Part 2: Throw, rather than crash, on trying to instantiate a runtime-disabled class. r?jwalden

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 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1491939 - Part 3: Centralize configuration of JS realm options from prefs. r?baku

Depends on D6554

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: 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

Depends on D6555

Comment 11

[Andrea Marchesini [:baku]]

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.

Comment 12

[Andrea Marchesini [:baku]]

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.

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc237fd92fddcdf8d0b6bd49791406d8bfde01b9

Comment 15

[Jeff Walden [:Waldo]]

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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/7a2f09c609ce
https://hg.mozilla.org/mozilla-central/rev/72e279c3075d
https://hg.mozilla.org/mozilla-central/rev/b3ef3ff4ef21
https://hg.mozilla.org/mozilla-central/rev/8ef0b50e1d1b