Closed Bug 1385890 Opened 3 years ago Closed 2 years ago

Streams implementation broken in multiple-global uses

Categories

(Core :: JavaScript Engine, enhancement, P3)

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: bzbarsky, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 7 obsolete files)

46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
385.14 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-phabricator-request
Details | Review
I asked about this in bug 1272697, but apparently this was not enough to keep things from landing, nor enough to get tests written or anything...

The streams implementation landed there is broken when multiple globals are involved in various ways.  Some examples:

1)  Create a stream in global A, then do globalB.ReadableStream.prototype.getReader.call(stream).  Per spec, the reader should be in global B, but we create it in global A, because of the CallNonGenericMethod thing.  This is simple to detect by comparing the prototypes of two different readers for two different streams.  Clearly the spec tests don't actually test this...

2)  JS::ReadableStreamDefaultReaderRead doesn't actually implement the spec's https://streams.spec.whatwg.org/#readable-stream-default-reader-read becuase it requires to be called in a way such that the current global matches the stream's global.  This means that any calls to this function are going to either be non-spec-compliant or security bugs or both.

This needs to be sorted out before we ship any of this stuff.
Blocks: 1272697
> I asked about this in bug 1272697, but apparently this was not enough to keep things from landing, nor enough to get tests written or anything...

Its disabled in nightly AFAIK.  I thought you were ok with landing it disabled first and then fix before enabling.
> 2)  JS::ReadableStreamDefaultReaderRead doesn't actually implement 

Er, sorry, I meant JS::ReadableStreamGetReader, which I assume is meant to be used to implement https://streams.spec.whatwg.org/#acquire-readable-stream-reader as called from https://fetch.spec.whatwg.org/#concept-get-reader or so.

I guess shu did raise the question of how usable the C API is.  The answer is that it's a security footgun in its current state.
> I thought you were ok with landing it disabled first and then fix before enabling.

I was, but no one bothered to even file an issue to track the problems, nor responded to my concerns in bug 1272697, nor added any comments about the problem to the code, nor gave any other indication that there were any plans to address this.  Hence this bug, so we at least have an issue tracking it, and hence my frustration that I have to do the work of filing this bug.  :(
My apologies for not fixing these issues, and in particular for not addressing the comments about them in bug 1272697. You're right to be grumpy about the latter for sure, and about not shipping until this is fixed.

I'll look into these issues over the next few days and will update this bug once I can say more.
This should cover all cross-global scenarios, except for byte stream readers. I haven't fully checked those, but will do that in a follow-up patch.
Assignee: nobody → till
Status: NEW → ASSIGNED
Attachment #8893879 - Flags: review?(bzbarsky)
Attachment #8893879 - Flags: feedback?(bkelly)
Just FYI, I'm trying to get the patch set in bug 1128959.  Once I have that done and green I will see what impact this patch has.  I may want to land the DOM patches first and then make any additional changes for this stuff here.
I would like to land bug 1128959 before this one.  Switching the dependency around.

Also note, we need to examine the uses of AutoEntryScript and the globals passed in FetchStreamReader as discussed in bug 1128959 comment 151.
No longer blocks: streams, 1272697
Depends on: streams, 1272697
I would write a spec issue, but its unclear to me if this is a fetch API issue with its streams integration or an issue on streams spec itself.  Till, what do you think?
It's hard to tell.  The streams spec _can_ take the consistent position that it doesn't do anything interesting with entry/incumbent globals and only modifies the current global if calls to functions happen.  But then anything that calls into the streams spec must ensure that it sets things up correctly (or is itself running in an already set up state).  This is particularly important for anything that touches streams in an async way.
I agree that it's not 100% immediately obvious. I far as I can tell, the visible effects of this should be somewhat subtle though, right? The chunks that fetch enqueues are written into ArrayBuffers that are either allocated by content  or at least in the reader's compartment. That's not to say it's not important to settle these questions, just that I'd hope the compatibility implications might be limited. 

I'm on pto Monday and part of Tuesday, so won't be able to follow up here. I'm aware of the lack of jsapi test coverage - I ran out of time, but will add tests on Tuesday.
Comment on attachment 8893879 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios

Is there some overall documentation on what's stored in what slots on what objects here?  That would be really helpful in terms of getting this reviewed....

I'm going to try to get to this this week, but if I don't manage that, I won't be able to until after I get back from vacation.  :(
Flags: needinfo?(till)
Comment on attachment 8893879 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios

Review of attachment 8893879 [details] [diff] [review]:
-----------------------------------------------------------------

I can't speak to the contents of the patch, but it doesn't currently pass WPT tests.  A local run of the streams WPT dir gives me:

/streams/readable-byte-streams/general.dedicatedworker.html
-----------------------------------------------------------
CRASH [Parent]
/streams/readable-byte-streams/general.html
-------------------------------------------
CRASH [Parent]
/streams/readable-byte-streams/general.serviceworker.https.html
---------------------------------------------------------------
CRASH [Parent]
/streams/readable-byte-streams/general.sharedworker.html
--------------------------------------------------------
CRASH [Parent]
/streams/readable-streams/brand-checks.dedicatedworker.html
-----------------------------------------------------------
FAIL ReadableStream.prototype.cancel enforces a brand check
/streams/readable-streams/brand-checks.html
-------------------------------------------
FAIL ReadableStream.prototype.cancel enforces a brand check
/streams/readable-streams/brand-checks.serviceworker.https.html
---------------------------------------------------------------
FAIL ReadableStream.prototype.cancel enforces a brand check
/streams/readable-streams/brand-checks.sharedworker.html
--------------------------------------------------------
FAIL ReadableStream.prototype.cancel enforces a brand check

This is on top of the patches in bug 1128959 that I pushed to inbound last night.
Attachment #8893879 - Flags: feedback?(bkelly) → feedback-
The general.html crash is:

Assertion failure: isObject(), at /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dist/include
/js/Value.h:642

Thread 1 "firefox" received signal SIGSEGV, Segmentation fault.
0x00007fffea4aa768 in JS::Value::toObject (this=<optimized out>)
    at /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dist/include/js/Value.h:642
642             MOZ_ASSERT(isObject());
(gdb) bt
#0  0x00007fffea4aa768 in JS::Value::toObject (this=<optimized out>)
    at /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dist/include/js/Value.h:642
#1  ReaderFromStream (maybeCx=0x0, ac=..., stream=<optimized out>)
    at /srv/mozilla-central/js/src/builtin/Stream.cpp:296
#2  0x00007fffea4b0b21 in ReadableStreamHasDefaultReader (stream=0x0)
    at /srv/mozilla-central/js/src/builtin/Stream.cpp:1734
#3  ReadableByteStreamControllerEnqueue (cx=0x7fffd412a000, controller=..., chunk=...)
    at /srv/mozilla-central/js/src/builtin/Stream.cpp:4221
#4  0x00007fffea4d5621 in ReadableByteStreamController_enqueue_impl (cx=0x7fffd412a000, args=...)
    at /srv/mozilla-central/js/src/builtin/Stream.cpp:3539
#5  0x00007fffea4d50fd in JS::CallNonGenericMethod<&(bool Is<js::ReadableByteStreamController>(JS::Handle<JS::Value>)), &(ReadableByteStreamController_enqueue_impl(JSContext*, JS::CallArgs const&))>
    (cx=<optimized out>, args=...)
    at /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dist/include/js/CallNonGenericMethod.h:100
#6  ReadableByteStreamController_enqueue (cx=0x7fffd412a000, argc=<optimized out>,
    vp=<optimized out>) at /srv/mozilla-central/js/src/builtin/Stream.cpp:3551
#7  0x00007fffea13658e in js::CallJSNative (cx=<optimized out>, native=<optimized out>, args=...)
    at /srv/mozilla-central/js/src/jscntxtinlines.h:293
#8  0x00007fffea12a81e in js::InternalCallOrConstruct (cx=0x7fffd412a000, args=...,
    construct=js::NO_CONSTRUCT) at /srv/mozilla-central/js/src/vm/Interpreter.cpp:469
#9  0x00007fffea122de0 in js::CallFromStack (cx=0x7fffd412a000, args=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:520
#10 Interpret (cx=<optimized out>, state=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:3065
#11 0x00007fffea11abef in js::RunScript (cx=0x7fffd412a000, state=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:409
#12 0x00007fffea12a837 in js::InternalCallOrConstruct (cx=0x7fffd412a000, args=...,
    construct=js::NO_CONSTRUCT) at /srv/mozilla-central/js/src/vm/Interpreter.cpp:487
#13 0x00007fffea12aded in js::Call (cx=0x7ffff7110540 <_IO_2_1_stderr_>, fval=..., thisv=...,
    args=..., rval=...) at /srv/mozilla-central/js/src/vm/Interpreter.cpp:533
#14 0x00007fffea60767f in js::fun_apply (cx=<optimized out>, argc=<optimized out>,
    vp=<optimized out>) at /srv/mozilla-central/js/src/jsfun.cpp:1309
#15 0x00007fffea13658e in js::CallJSNative (cx=<optimized out>, native=<optimized out>, args=...)
    at /srv/mozilla-central/js/src/jscntxtinlines.h:293
#16 0x00007fffea12a81e in js::InternalCallOrConstruct (cx=0x7fffd412a000, args=...,
    construct=js::NO_CONSTRUCT) at /srv/mozilla-central/js/src/vm/Interpreter.cpp:469
#17 0x00007fffea122de0 in js::CallFromStack (cx=0x7fffd412a000, args=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:520
#18 Interpret (cx=<optimized out>, state=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:3065
#19 0x00007fffea11abef in js::RunScript (cx=0x7fffd412a000, state=...)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:409
#20 0x00007fffea12bf20 in js::ExecuteKernel (cx=<optimized out>, script=..., envChainArg=...,
    newTargetValue=..., evalInFrame=..., result=<optimized out>)
    at /srv/mozilla-central/js/src/vm/Interpreter.cpp:698
#21 0x00007fffea12c201 in js::Execute (cx=<optimized out>, script=..., envChainArg=...,
    rval=<optimized out>) at /srv/mozilla-central/js/src/vm/Interpreter.cpp:730
#22 0x00007fffea5b1c50 in ExecuteScript (cx=0x7fffd412a000, scope=..., script=...,
    rval=0x7fffffffba50) at /srv/mozilla-central/js/src/jsapi.cpp:4632
#23 0x00007fffea5b1f74 in ExecuteScript (cx=0x7fffd412a000, envChain=..., scriptArg=...,
    rval=0x7fffffffba50) at /srv/mozilla-central/js/src/jsapi.cpp:4651
#24 0x00007fffe784ccaa in nsJSUtils::ExecutionContext::CompileAndExec (this=0x7fffffffba10,
    aCompileOptions=..., aSrcBuf=..., aScript=...)
    at /srv/mozilla-central/dom/base/nsJSUtils.cpp:265
#25 0x00007fffe8a469db in mozilla::dom::ScriptLoader::EvaluateScript (this=0x7fffacf3b740,
    aRequest=0x7fffaba9a700) at /srv/mozilla-central/dom/script/ScriptLoader.cpp:2112
#26 0x00007fffe8a4554c in mozilla::dom::ScriptLoader::ProcessRequest (this=0x7fffacf3b740,
    aRequest=0x7fffaba9a700) at /srv/mozilla-central/dom/script/ScriptLoader.cpp:1770
#27 0x00007fffe8a3e925 in mozilla::dom::ScriptLoader::ProcessScriptElement (this=<optimized out>,
    aElement=0x0) at /srv/mozilla-central/dom/script/ScriptLoader.cpp:1372
#28 0x00007fffe8a3d83a in mozilla::dom::ScriptElement::MaybeProcessScript (this=<optimized out>)
    at /srv/mozilla-central/dom/script/ScriptElement.cpp:149
#29 0x00007fffe7339a99 in nsIScriptElement::AttemptToExecute (this=0x7fffaba78710)
    at /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dist/include/nsIScriptElement.h:225
#30 nsHtml5TreeOpExecutor::RunScript (this=0x7fffabc9b400, aScriptElement=<optimized out>)
    at /srv/mozilla-central/parser/html/nsHtml5TreeOpExecutor.cpp:705
#31 0x00007fffe733847c in nsHtml5TreeOpExecutor::RunFlushLoop (this=<optimized out>)
    at /srv/mozilla-central/parser/html/nsHtml5TreeOpExecutor.cpp:506
#32 0x00007fffe734897d in nsHtml5ExecutorReflusher::Run (this=<optimized out>)
    at /srv/mozilla-central/parser/html/nsHtml5TreeOpExecutor.cpp:60
#33 0x00007fffe6805b8d in nsThread::ProcessNextEvent (this=<optimized out>,
    aMayWait=<optimized out>, aResult=0x7fffffffc5f7)
    at /srv/mozilla-central/xpcom/threads/nsThread.cpp:1446
#34 0x00007fffe680759d in NS_ProcessNextEvent (aThread=0x7ffff7110540 <_IO_2_1_stderr_>,
    aMayWait=<optimized out>) at /srv/mozilla-central/xpcom/threads/nsThreadUtils.cpp:480
#35 0x00007fffe6d1403b in mozilla::ipc::MessagePump::Run (this=<optimized out>,
    aDelegate=<optimized out>) at /srv/mozilla-central/ipc/glue/MessagePump.cpp:97
#36 0x00007fffe6cb8ddd in MessageLoop::RunHandler (this=0x7fffe3db7c00)
    at /srv/mozilla-central/ipc/chromium/src/base/message_loop.cc:319
#37 MessageLoop::Run (this=0x7fffe3db7c00)
    at /srv/mozilla-central/ipc/chromium/src/base/message_loop.cc:299
#38 0x00007fffe8ac66e5 in nsBaseAppShell::Run (this=0x7fffd0aa9e10)
    at /srv/mozilla-central/widget/nsBaseAppShell.cpp:158
#39 0x00007fffe9eb9660 in nsAppStartup::Run (this=0x7fffd0af5d30)
    at /srv/mozilla-central/toolkit/components/startup/nsAppStartup.cpp:287
#40 0x00007fffe9f5439a in XREMain::XRE_mainRun (this=<optimized out>)
    at /srv/mozilla-central/toolkit/xre/nsAppRunner.cpp:4632
#41 0x00007fffe9f54e06 in XREMain::XRE_main (this=<optimized out>, argc=<optimized out>,
    argv=<optimized out>, aConfig=...) at /srv/mozilla-central/toolkit/xre/nsAppRunner.cpp:4796
#42 0x00007fffe9f553f8 in XRE_main (argc=-149874832, argv=0x0, aConfig=...)
    at /srv/mozilla-central/toolkit/xre/nsAppRunner.cpp:4891
#43 0x0000000000405ac6 in do_main (argc=<optimized out>, argv=<optimized out>,
    envp=<optimized out>) at /srv/mozilla-central/browser/app/nsBrowserApp.cpp:236
#44 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>)
    at /srv/mozilla-central/browser/app/nsBrowserApp.cpp:309
Comment on attachment 8893879 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios

>+StreamFromReader(JSContext *maybeCx, const NativeObject* reader)
>+    if (IsWrapper(streamObj)) {
>+        if (JS_IsDeadWrapper(streamObj)) {

I thought IsWrapper and JS_IsDeadWrapper were mutually exclusive...  Is that not the case?

>+ReaderFromStream(JSContext* maybeCx, Maybe<AutoCompartment>& ac, const NativeObject* stream)
>+    if (IsWrapper(readerObj)) {
>+        if (JS_IsDeadWrapper(readerObj)) {

And here.

Why are the unchecked unwraps in StreamFromReader and ReaderFromStream safe?  Can these never get called by code that is not supposed to have access to the relevant object?  There should at least be comments here explaining why the unchecked unwrap is ok.

>+    static MOZ_MUST_USE ReadableStream* stream(JSContext* cx, TeeState* teeState) {
>+        return ToUnwrapped<ReadableStream>(cx, streamVal, "", "", "");

Why is passing all those empty strings ok?

I'm going to get back to this when I get back, I guess.  I ran out of time now.  :(
No longer depends on: streams-enable
Sorry for the crashes - silly "why would I need to test this, it's obviously ok" change before uploading :(

This patch passes all readable-(byte-)streams tests and addresses bz's preliminary feedback. I'll add more documentation and tests, but this should be useful for testing already.
Flags: needinfo?(till)
Attachment #8897098 - Flags: feedback?(bkelly)
Comment on attachment 8897098 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios. v2

Now that Andrea is back from PTO I'm redirecting this to him.
Attachment #8897098 - Flags: feedback?(bkelly) → feedback?(amarchesini)
That took longer than expected - I hit a snag around error handling. That's fixed now, as are the two issues Andrea's try run[1] showed.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=13d6653f2eaa0b14df4df233ac55c93d153f8d2a
Attachment #8893879 - Attachment is obsolete: true
Attachment #8897098 - Attachment is obsolete: true
Attachment #8893879 - Flags: review?(bzbarsky)
Attachment #8897098 - Flags: feedback?(amarchesini)
Flags: needinfo?(bzbarsky)
Attachment #8907148 - Flags: feedback?(amarchesini)
Priority: -- → P3
Comment on attachment 8907148 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios. v3

Formally requesting review now that bz's review queue is open again.
Flags: needinfo?(bzbarsky)
Attachment #8907148 - Flags: review?(bzbarsky)
Comment on attachment 8907148 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios. v3

Review of attachment 8907148 [details] [diff] [review]:
-----------------------------------------------------------------

I did some tests. I looks good to me.
Attachment #8907148 - Flags: feedback?(amarchesini) → feedback+
Comment on attachment 8907148 [details] [diff] [review]
Make relevant ReadableStream methods and JSAPI functions work in cross-global usage scenarios. v3

I'm really sorry for the horrible lag here.  :(

>+++ b/js/src/builtin/Stream.cpp
> enum StreamSlots {

Thank you for the documentation.

It might also be good to point to https://streams.spec.whatwg.org/#rs-internal-slots here and document that "StreamSlot_State" stores the [[state]] and [[disturbed]] bits from the spec.

>+* Of the stored values, Stream might be a cross-compartment wrapper.
>+* This can happen if the Reader was created by applying a different
>+* compartment's ReadableStream.prototype.getReader method.

OK.  What about ReaderSlot_ClosedPromise?  Per spec, this can be initialized in the following places, as far as I can see:

1) ReadableStreamReaderGenericInitialize.  This is called from the reader constructor, so would end up same-compartment with the reader.

2) ReadableStreamReaderGenericRelease If the stream's state is not "readable".  This, as far as I can tell, can be invoked in a different compartment, via ReadableStreamDefaultReader.prototype.releaseLock.call(someReaderFromAnotherCompartment).

3) ReadableStreamReaderGenericRelease can also be invoked via the stuff pipeTo defines.  I filed https://github.com/whatwg/streams/issues/845 on this.

> enum ReaderSlots {

Again, pointing to https://streams.spec.whatwg.org/#default-reader-internal-slots and https://streams.spec.whatwg.org/#byob-reader-internal-slots might be a good idea.

I assume ReaderSlot_Requests stores a same-compartment Array which may then store cross-compartment wrappers?  Might be worth documenting that too.

>+ * Throws exceptions of the value isn't an object, cannot be unwrapped, or

s/of the value/if the value/

>+ * If the caller can ensure that either no exception other than a possibly
>+ * dead wrapper exception is possible, it may omit the error messages.

s/either no/no/ ?

>+ToUnwrapped(JSContext* cx, HandleValue val, const char* description = "",
>+    if (JS_IsDeadWrapper(obj)) {
>+        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_DEAD_OBJECT);

This is just here to make the error message nicer, right?  As in, the !obj->is<T>() check would catch this case too?

If so, might be worth documenting.

I should note that JSMSG_DEAD_OBJECT doesn't list the class/method, so might not be as useful as JSMSG_INCOMPATIBLE_PROTO in practice...

>+ * If the stream cannot be unwrapped, which can only happen if it's a dead
>+ * wrapper object, a nullptr is returned. If a JSContext is available, also
>+ * report an exception.

"If a JSContext is available, this case will also report an exception."?

And maybe also just say:

> and can provide useful defaults in case unwrapping fails.

I'd just say "in case the stream is a dead wrapper".

Should probably document that the passed-in reader is expected to be an actual reader, not a cross-compartment wrapper for one.  I assume we don't have a shared base class for our readers that is not quite as generic as NativeObject that we could use as the arg type?

>+ * Returns the reader associated with the given stream.

This assumes that the stream has a reader, right?  Not all streams have readers, if I understand things correctly.

This precondition should probably be documented.

>+ * as a result to operating on streams with readers are supposed to be

s/to operating/of operating/

>+ * useful defaults in case unwrapping fails.

"in case the reader is a dead object wrapper"?

>+ReaderFromStream(JSContext* maybeCx, Maybe<AutoCompartment>& ac, const NativeObject* stream)

Again, "stream" has to be an actual ReadableStream*, right?  Is there a reason to not make the argument be that?

Again, if we had a narrower class than NativeObject for the return value that might be nice...

>@@ -516,11 +630,10 @@ class TeeState : public NativeObject
>+        RootedValue streamVal(cx, teeState->getFixedSlot(Slot_Stream));
>+        return ToUnwrapped<ReadableStream>(cx, streamVal);

I don't see documentation about compartments for TeeState slots.  Please add some.

I also don't see documentation for controller slots.  For example, is ControllerSlot_UnderlyingSource guaranteed to not be a cross-compartment wrapper?  I don't see how, fwiw, when it might be storing a TeeState, since we might have created that while not in the controller's compartment...  That means various operations on ControllerSlot_UnderlyingSource are probably bogus (e.g. Is<TeeState> tests).

>@@ -560,7 +673,10 @@ class TeeState : public NativeObject

So I'm a little confused by preexisting code in ReadableStreamControllerCancelSteps.  When it's doing "Step 2 of 3.8.5.1, step 3 of 3.10.5.1", it checks for Is<TeeState>(underlyingSource) and explicitly invokes ReadableStreamTee_Cancel.  Is that ok because TeeState objects and their prototype are never exposed to script, so we know what the property lookup will return?  Is this an optimization or needed for correctness (e.g. if there is no "cancel" property on the TeeState prototype)?  If the latter, what happens if underlyingSource is a wrapped TeeState, or can that not happen?

>+ReadableStream_tee(JSContext* cx, unsigned argc, Value* vp)

Please document that ReadableStreamTee gets its "stream" arg not in the cx compartment, but is expected to produce branch1Stream/branch2Stream in the cx compartment.

It might be worth documenting in ReadableStreamTee why the compartment of closedPromise is the caller compartment, because that's not obvious a priori; see comments above about closedPromise being something that can be from arbitrary compartments.

>+ReadableStreamAddReadOrReadIntoRequest(JSContext* cx, Handle<ReadableStream*> stream)
>+        promise = PromiseObject::createSkippingExecutor(cx);

Why is it correct, from the caller's point of view, to create this in the reader compartment, not the caller compartment?  If we change this, of course, we need to deal with possibly-wrapped-promises in all the places where we use ReaderSlot_Requests.

>@@ -1371,6 +1484,8 @@ ReadableStream::cancel(JSContext* cx, Handle<ReadableStream*> stream, HandleValu

It would be good to document what "stream" may not be same-compartment with cx.  Can "reason" be different-compartment too?  It can certainly be different-compartment from "stream", right?  All this should be documented.

>@@ -1406,19 +1521,21 @@ ReadableStreamCloseInternal(JSContext* cx, Handle<ReadableStream*> stream)

Please document that stream may not be in the cx compartment.

>+  Maybe<AutoCompartment> ac;

Document that after the ReaderFromStream call this puts us in the reader compartment?

When we go do do this:

              resultObj = CreateIterResultObject(cx, UndefinedHandleValue, true);

we're in the reader compartment.  But don't we want to be in the caller compartment, so the object gets created there?  I'd think we do.  Please add a test as needed...

If we _do_ change this, we need to make sure that "resultVal" and "readRequest" are same-compartment by the time we pass them to ResolvePromise.  "readRequest" is presumaby same-compartment with the reader here, which may not match the caller compartment.

>@@ -1475,18 +1594,25 @@ ReadableStreamErrorInternal(JSContext* cx, Handle<ReadableStream*> stream, Handl
>     val = reader->getFixedSlot(ReaderSlot_ClosedPromise);
>     Rooted<PromiseObject*> closedPromise(cx, &val.toObject().as<PromiseObject>());

As noted above, the ReaderSlot_ClosedPromise might be storing a cross-compartment wrapper, as far as I can tell...

>         cx->runtime()->readableStreamErroredCallback(cx, stream, source,

So here cx is in the stream compartment, but "e" could be in some other compartment, right?  Needs at least documentation, and checking that Gecko is OK with this...

>@@ -1544,7 +1674,10 @@ ReadableStreamFulfillReadOrReadIntoRequest(JSContext* cx, Handle<ReadableStream*
>+    RootedObject iterResult(cx, CreateIterResultObject(cx, wrappedChunk, done));

So this is creating the resolution value in the reader compartment, not the caller compartment.  Why?

>@@ -1572,10 +1713,13 @@ ReadableStreamHasBYOBReader(ReadableStream* stream)
>+    return ReaderFromStream(nullptr, ac, stream)->is<ReadableStreamBYOBReader>();

Won't this crash if the reader is a dead object wrapper?

>@@ -1584,10 +1728,13 @@ ReadableStreamHasDefaultReader(ReadableStream* stream)
>+    return ReaderFromStream(nullptr, ac, stream)->is<ReadableStreamDefaultReader>();

Again, will crash on dead object wrappers.

>@@ -1608,8 +1755,7 @@ CreateReadableStreamDefaultReader(JSContext* cx, Handle<ReadableStream*> stream)

Would be good to document that the arg may not be in the cx compartment.

>ReadableStreamDefaultReader_closed(JSContext* cx, unsigned argc, Value* vp)

This looks like it will fail if "this" is a cross-compartment wrapper for a default reader.  Please add a test.

>ReadableStreamDefaultReader_cancel(JSContext* cx, unsigned argc, Value* vp)

Will fail if "this" is a cross-compartment wrapper for a default reader.  Add a test.

>ReadableStreamDefaultReader_read(JSContext* cx, unsigned argc, Value* vp)

Will fail if "this" is a cross-compartment wrapper for a default reader.  Add a test.

>ReadableStreamBYOBReader_closed(JSContext* cx, unsigned argc, Value* vp)
>ReadableStreamBYOBReader_cancel(JSContext* cx, unsigned argc, Value* vp)
>ReadableStreamBYOBReader_read(JSContext* cx, unsigned argc, Value* vp)

Same thing for all of these.

For ReadableStreamBYOBReader_read the "view" arg could also be a cross-compartment wrapper, as far as I can tell, and we need to support that.

>+ReadableStreamReaderGenericCancel(JSContext* cx, HandleNativeObject reader, HandleValue reason_)
>+//    if (!IsObjectInContextCompartment(stream, cx)) {

etc.  Why is this commented-out code here?

>@@ -2027,10 +2190,21 @@ ReadableStreamReaderGenericInitialize(JSContext* cx, HandleNativeObject reader,
>                                       Handle<ReadableStream*> stream)

Please document that reader is same-compartment with cx but stream may not be?

> ReadableStreamReaderGenericRelease(JSContext* cx, HandleNativeObject reader)

So we guarantee that reader is in the cx compartment by making releaseLock be non-generic, right?

What about entry via js::ReadableStreamReaderReleaseLock?  I guess that asserts same-compartment too?

Might be worth it to just assert here, and make sure we have tests for what happend if releaseLock is called cross-compartment.

>     // Create an exception to reject promises with below. We don't have a
>     // clean way to do this, unfortunately.

We're going to create this exception in the reader compartent, not the releaseLock caller compartment.  Why is that correct?

I assume something guarantees that in the stream->readable() case the ReaderSlot_ClosedPromise is in fact a promise in the reader's compartment, not a cross-compartment wrapper?  It would be good to document what guarantees that.

>@@ -2118,7 +2301,9 @@ ReadableStreamBYOBReader::read(JSContext* cx, Handle<ReadableStreamBYOBReader*>

Need to document whether "reader" and "view" are in the cx compartment or might not be.  I think the answer per above is "might not be".

> ReadableByteStreamControllerPullInto(JSContext* cx,

We should document that "view" and "controller" may not be in the cx compartment (and may not be same-compartment with each other).

Is calling JS_GetArrayBufferViewBuffer with "view" not in the cx compartment ok?

Does PullIntoDescriptor::create handle "buffer" not being in the same compartment as cx and "ctor"?

Is TransferArrayBuffer ok to call with "buffer" not in the cx compartment?

Is pullIntoDescriptor->setBuffer ok to call with transferredBuffer?  What compartment is transferredBuffer in?

Appending pullIntoDescriptor (which is in the cx compartment) to pendingPullIntos (which are in the controller compartment) without wrapping doesn't look right.  I wonder whether it would trigger asserts...

I am giving up on trying to audit the calltree under here, because it's pretty clear that hasn't happened.  I expect there are all sorts of bugs under there, not least the bits that assume the controller and TeeState are same-compartment.  :(

If you really do need me to audit this, let me know, but that can't possibly happen until at least Monday.

Anyway, we definitely need tests for all these codepaths in the cross-compartment cases.

>@@ -2573,32 +2764,43 @@ ReadableStreamControllerCancelSteps(JSContext* cx, HandleNativeObject controller
>+    // The following steps up to but not including calling
>+    // |underlyingSource.cancel(reason)| need to happen in the controller's
>+    // compartment.

Do they?

Why is calling ReadableStreamTee_Cancel in the controller's compartment the right thing here?  I mean in terms of the compartment that the promise gets created in.

>+            return ReadableStreamTee_Cancel(cx, teeState, defaultController, reason);

As far as I can tell, this isn't sound.  ReadableStreamTee_Cancel( assumes that teeState and reason are same-compartment (and sets slots accordingly), but I see absolutely nothing that would ensure that in this code.  teeState is in the controller compartment; we got it from a slot.  "reason" is in the caller compartment.  If I'm right, this needs test coverage, and if those tests don't fail with this patch as-is, I would like to know why.

>+            rval = cx->runtime()->readableStreamCancelCallback(cx, stream, source,
>+                                                               stream->embeddingFlags(), reason);

OK, so this is being called in the stream/controller compartment.  "source" is a void*, so it's ok, but "reason" may not be same-compartment with the other stuff here....

Maybe we should just wrap "reason" into the controller compartment either before calling ReadableStreamControllerCancelSteps or on entry into it or something?  Or at least before calling ReadableStreamTee_Cancel and the runtime callback?  Or document that the runtime callback can get called with compartment mismatches or something?

Of course in our case the runtime callback ignores "reason", so this is just a latent bug...

>+            return PromiseObject::unforgeableResolve(cx, rval);

Why is this supposed to be called in the controller's compartment?  It's not clear to me why that matches what the spec says to do, in terms of which compartment the promise objects are created in.  Do we have tests covering this?

>     return PromiseInvokeOrNoop(cx, underlyingSource, cx->names().cancel, reason);

I don't see why this is OK.  underlyingSource is in the controller compartment, by definition (we got it from a slot on the controller).  cx may not be in that compartment.  PromiseInvokeOrNoop ends up calling things like JS::GetProperty ad JS::Call which I'm pretty sure assume all the compartments match (though they don't assert this at least in the JS::GetProperty case????).

>DequeueValue(JSContext* cx, HandleNativeObject container, MutableHandleValue chunk)

Is it OK if "container" is not same-compartment with "cx" here?  Document, please.

>ReadableStreamControllerError(JSContext* cx, HandleNativeObject controller, HandleValue e)

It sure looks to me like this can be called with "controller" not same-compartment with "cx" once all the above bits are fixed, and then ResetQueue will do the wrong thing.  ResetQueue should probably have compartment asserts, at the very least.

>ReadableStreamErrorInternal(JSContext* cx, Handle<ReadableStream*> stream, HandleValue e)

As far as I can tell, this can be called with "stream" and "e" in different compartments, if the various bits above are fixed.  The StoredError slot then gets messed up.

I have NOT so far audited the following functions, any of which might be harboring bugs, nor the things they call:

* ReadableByteStreamControllerPullSteps

* ReadableStreamAddReadOrReadIntoRequest -- need to double-check that retval is in the cx compartment and stream not in cx compartment is handled.

* ReadableByteStreamControllerFillPullIntoDescriptorFromQueue -- need to double-check what happens when controller is not same-compartment with cx and pullIntoDescriptor.  I _think_ those two are always same-compartment with each other?

* ReadableByteStreamControllerHandleQueueDrain -- need to double-check behavior when controller is not in cx compartment.

* ReadableStreamControllerCallPullIfNeeded -- what if controller not in cx compartment?

* ReadableByteStreamControllerClearPendingPullIntos -- what if controller not in cx compartment?

* ReadableStreamDefaultControllerEnqueue -- controller can be not in cx compartment?  What about chunk?  Which compartment is it in?

>+++ b/js/src/jsapi-tests/testReadableStream.cpp
>+// Cross-global tests:

This is ... quite limited.  I don't see attempts to do cross-global calls on a reader (i.e. ones where the callee and the reader are not same-global), for example.

Same thing for calls on controllers.

I would assume we want to exercise every API defined in the streams spec under these conditions, and ideally in a situation where the reader is from compartment A, the stream from compartment B, and all calls come from compartment C.

>+++ b/js/src/jsapi.cpp
>+JS::ReadableStreamGetEmbeddingFlags(const JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

I don't see how that's OK.  If the caller has an opaque wrapper or dead object, you get a null deref or random memory read respectively, no?

> JS::IsReadableStream(const JSObject* obj)
>+    // Since we don't have a cx here, we return false if unwrappin fails.

"unwrapping".

>+JS::ReadableStreamIsReadable(const JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

That's not OK.  You probably want to either require that this be called only on things on which IsReadableStream is true, or otherwise handle this.

>+JS::ReadableStreamIsLocked(const JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

Again, not OK.

>+JS::ReadableStreamIsDisturbed(const JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

Again.

>+JS::ReadableStreamCancel(JSContext* cx, HandleObject streamObj_, HandleValue reason)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

This will do bad things if streamObj is a dead object wrapper, no?  (Or if it's not a stream at all, but presumably that's just unsupported.)

>+JS::ReadableStreamGetMode(const JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

Not OK.

>+JS::ReadableStreamGetReader(JSContext* cx, HandleObject streamObj_, ReadableStreamReaderMode mode)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Looks wrong in the dead wrapper case.

>+JS::ReadableStreamGetExternalUnderlyingSource(JSContext* cx, HandleObject streamObj_,
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Wrong in the dead wrapper case.

>+JS::ReadableStreamReleaseExternalUnderlyingSource(JSObject* streamObj)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

Not OK.

>+JS::ReadableStreamUpdateDataAvailableFromSource(JSContext* cx, JS::HandleObject streamObj_,
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

>+JS::ReadableStreamTee(JSContext* cx, HandleObject streamObj_,
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

Really, we should factor out this "Get me a ReadableStream*" bit into a separate method instead of copy/pasting it so many times.  Then we make that method correct.

> JS::ReadableStreamGetDesiredSize(JSObject* streamObj, bool* hasValue, double* value)
>+    // Since we don't have a cx here, we assume the unwrapping to succeed.

Not ok.

>+JS::ReadableStreamClose(JSContext* cx, HandleObject streamObj_)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

>+JS::ReadableStreamEnqueue(JSContext* cx, HandleObject streamObj_, HandleValue chunk_)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

>+JS::ReadableByteStreamEnqueueBuffer(JSContext* cx, HandleObject streamObj_, HandleObject chunkObj_)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

>+JS::ReadableStreamError(JSContext* cx, HandleObject streamObj_, HandleValue error_)
>     Rooted<ReadableStream*> stream(cx, &streamObj->as<ReadableStream>());

Not OK if dead wrapper.

>+++ b/js/src/tests/whatwg/shell.js
>+            if ((exc === val) === (val === val) && (val !== 0 || 1 / exc === 1 / val))

Is this basically reinventing Object.is()?

>diff --git a/js/src/tests/whatwg/streams/readable-stream-globals.js b/js/src/tests/whatwg/streams/readable-stream-globals.js

As in the C++ case, there needs to be a _lot_ more extensive testing here.

>diff --git a/js/src/tests/whatwg/streams/shell.js b/js/src/tests/whatwg/streams/shell.js
>new file mode 100644
>index 000000000000..e69de29bb2d1

Empty file?  Not clear what's going on here.
Attachment #8907148 - Flags: review?(bzbarsky) → review-
Just as an update, the changes required here are much more significant than I had thought. I'm working through them and it's progressing nicely, but it's a ton of work.
Attached patch Intradiff from last patch (obsolete) — Splinter Review
At long last, a new patch!

I've written more tests for JS usage, and addressed all of your feedback on the Streams implementation itself. A large part of that consists of removing support for byte streams: nobody else is shipping those, and I'm not convinced the spec is baked enough for us to ship them. At the very least, they're not on a critical path, and removing them significantly reduces the review burden. (I do have the implementation here, and have gone through it and fixed all cross-global issues I could find.)

There's still a lot of stuff in the code that's seemingly only relevant to byte streams, but that's because the implementation of streams with host-provided underlying sources makes heavy use of that infrastructure.

I added comments to all relevant functions about their expectations and products wrt globals (i.e., where they can operate on wrapped objects, which global they will create results in).

What I haven't yet done is update the jsapi tests, nor fixed all issues in jsapi functions. I'll work on that next, but wanted to get the review going for the implementation itself.


Finally, here's a select few of your comments with responses. Everything else I consider addressed in a way that obviates the need for responses.

-----

>@@ -560,7 +673,10 @@ class TeeState : public NativeObject

> So I'm a little confused by preexisting code in ReadableStreamControllerCancelSteps.  When it's doing "Step 2 of 3.8.5.1, step 3 of 3.10.5.1", it checks for Is<TeeState>(underlyingSource) and explicitly invokes ReadableStreamTee_Cancel.  Is that ok because TeeState objects and their prototype are never exposed to script, so we know what the property lookup will return?

Correct.

> Is this an optimization or needed for correctness (e.g. if there is no "cancel" property on the TeeState prototype)?  If the latter, what happens if underlyingSource is a wrapped TeeState, or can that not happen?

It's an optimization that lets us avoid creating the JSFunction for "cancel" and the overhead of the generic function call. I added documentation about it to ReadableStreamControllerCancelSteps.


>+ReadableStream_tee(JSContext* cx, unsigned argc, Value* vp)

> It might be worth documenting in ReadableStreamTee why the compartment of closedPromise is the caller compartment, because that's not obvious a priori; see comments above about closedPromise being something that can be from arbitrary compartments.

closedPromise must be from the caller compartment because it's created under the CreateReadableStreamDefaultReader earlier in ReadableStreamTee. I decided not to add documentation about that because the way the code works now it doesn't actually matter: JS::AddPromiseReactions can deal with wrapped Promises just fine and won't change its behavior in this particular case.
Attachment #8948889 - Flags: review?(bzbarsky)
I forgot to respond to the feedback comments regarding the various shell.js files: those are copied over from other locations, or, in case of the empty one, created as an empty file intentionally. The test harness requires these files.
Comment on attachment 8948889 [details] [diff] [review]
Intradiff from last patch

Jason is going to take over this review.

Jason, you might want to create a combined patch from this and the previous one, since this is an interdiff...
Attachment #8948889 - Flags: review?(bzbarsky) → review?(jorendorff)
Till, I want to start reviewing this tomorrow. Would you please rebase? A combined patch would be fine.
Flags: needinfo?(till)
This should apply to current tip - or at least it did a few hours ago. Compiles, except the jsapi test is slightly broken. I'll fix that and post a diff for that later, but it shouldn't hold up reviewing the implementation.
Attachment #8907148 - Attachment is obsolete: true
Attachment #8948889 - Attachment is obsolete: true
Attachment #8948889 - Flags: review?(jorendorff)
Flags: needinfo?(till)
Attachment #8967881 - Flags: review?(jorendorff)
Comment on attachment 8967881 [details] [diff] [review]
roll-up patch relative to current m-c

Review of attachment 8967881 [details] [diff] [review]:
-----------------------------------------------------------------

Clearing review for now. Below, I've asked for you to land some parts as separate changesets. Please go ahead with those. Everything looks OK, but please address the TODO parts and tests that don't compile, and re-post.

Also: this patch seems to end in the middle of a hunk, in package.json (see the raw attachment: <https://bug1385890.bmoattachments.org/attachment.cgi?id=8967881>). Am I missing something?

I couldn't tell why a lot of BYOB code was removed, but then a lot was also left in place. I assume that it doesn't matter, because none of it is currently active, even with options().streams() enabled. (?)

Another error path to test: say we have a chrome reader for a content stream (or some other cross-realm edge from a less-privileged compartment to a more-privileged compartment in an internal slot), and then content tries to do something that would touch the reader. It should throw. At least, I think this kind of security situation can be created, though not in the standalone JS shell. (I can never remember where such tests live; I think you can choose either a chrome mochitest or an xpcshell test using a sandbox...)

::: js/public/Stream.h
@@ +417,2 @@
>  /**
>   * Returns true if the given ReadableStream reader is locked, false otherwise.

Pre-existing typo: This comment on ReadableStreamReaderIsClosed should say "is closed", not "is locked".

::: js/src/builtin/Promise.cpp
@@ +2459,4 @@
>  
>  // ES2016, 25.4.5.3., steps 3-5.
>  MOZ_MUST_USE bool
> +js::OriginalPromiseThen(JSContext* cx, HandleObject promiseObj,

It took me longer than I care to admit to figure out that this just changes the function from not following the usual convention (that arguments are all same-compartment) to following it.

Please land the changes in Promise.cpp and the related change in jsapi.cpp in a separate changeset, r=me.

::: js/src/builtin/Stream.cpp
@@ +77,5 @@
> + *
> + * Both ReadableStreamDefaultController and ReadableByteStreamController are
> + * queue containers and must have these slots at identical offsets.
> + *
> + * The queue is guaranteed to be in the  same compartment as the container,

Style nit: extra space before same.

@@ +124,5 @@
> +/**
> + * Memory layout for ReadableByteStreamControllers, starting after the
> + * slots shared among all types of controllers.
> + *
> + * PendingPullIntos is guaranteed to be in the  same compartment as the

And here.

@@ +166,5 @@
>  }
>  
> +template<class T>
> +MOZ_ALWAYS_INLINE bool
> +IsMaybeWrapped(const HandleValue v)

Consider adding a member function template to JSObject that does this: `obj->isRealOrWrapped<ReadableStream>()`. At least put this in a header somewhere so that jsapi.cpp can use it too.

@@ +244,5 @@
>      return !reader->getFixedSlot(ReaderSlot_Stream).isUndefined();
>  }
>  
>  bool
>  js::ReadableStreamReaderIsClosed(const JSObject* reader)

Argument type should be `const ReadableStreamReader*`, for consistency. The caller can do the downcast.

@@ +290,5 @@
> +inline bool
> +JSObject::is<ReadableStreamReader>() const
> +{
> +    return is<ReadableStreamDefaultReader>();
> +}

These JSObject::is specializations are good and they should definitely be in Stream.h.

As it stands, this patch's Stream.h puts `class ReadableStreamReader` in scope but not this specialization. It's kind of asking for One Definition Rule violations (which compilers don't catch, and which can be masked by unified builds and uncovered later).

@@ +300,5 @@
> + * unwrap is the only possible source of exceptions.
> + */
> +template<class T>
> +static MOZ_ALWAYS_INLINE T*
> +ToUnwrapped(JSContext* cx, JSObject* obj)

Please move all of these ToUnwrapped<T> templates into a header file in the `js` namespace. They're good stuff! If we're not already doing this exact dance elsewhere, we probably should be, and will need do more of it in the future. Feel free to land it now, in a separate changeset, with review comments below addressed, r=me.

The comment is wrong, though.

/**
 * Downcast obj to T*, unwrapping if needed.
 *
 * Returns null, with an exception pending, if `obj` is a wrapper and
 * unwrapping fails. But if `obj` is neither a T object nor a wrapper for
 * a T, the behavior is undefined.
 */

This overload should have a different name from the others, which are safer (to avoid undefined behavior, all you have to do is pass well-defined arguments of the right types -- the Rust meaning of "safe", roughly translated to C++).

Either include the assertSameCompartment call from the jsapi.cpp version, or remember to re-insert it at the few places in jsapi.cpp where ToUnwrapped is used.

@@ +310,5 @@
> +            return nullptr;
> +        }
> +    }
> +
> +    return &obj->as<T>();

I think this will assert, or return an invalid pointer, if it is passed a nuked wrapper (a DeadObjectProxy). If so, and we have any cases where we are using this signature on a wrapper that may have been nuked, it's a bug...

I don't think this signature is used except in the byte-stream code, so maybe we're OK for now.

Maybe the fix is just to make this function safer.

@@ +2733,5 @@
>  };
>  
> +const Class ReadableStreamController::class_ = {
> +    "ReadableStreamController"
> +};

This is unused.

@@ +3222,5 @@
>      return ReadableStreamErrorInternal(cx, stream, e);
>  }
>  
> +inline static double QueueSize(const NativeObject* container);
> +inline static void SetQueueSize(NativeObject* container, double size);

These inline functions are called in a bunch of places before being defined, which I thought would get you a warning in some compilers...

Anyway, it seems like they could just be defined here.

@@ +3570,4 @@
>                  auto cb = cx->runtime()->readableStreamWriteIntoReadRequestCallback;
>                  MOZ_ASSERT(cb);
>                  // TODO: use bytesWritten to correctly update the request's state.
> +                // TODO: make this compartment-safe.

TODO!

@@ +4094,5 @@
>      return true;
>  }
>  
> +inline static double
> +QueueSize(const NativeObject* container)

While we're doing abstract base classes, consider adding a base class QueueContainer, so this can have a proper argument type?

@@ +4216,2 @@
>  MOZ_MUST_USE bool
>  js::ReadableStreamReaderCancel(JSContext* cx, HandleObject readerObj, HandleValue reason)

Please change the type of the argument to Handle<ReadableStreamReader>.

@@ +4223,5 @@
>      return ReadableStreamReaderGenericCancel(cx, reader, reason);
>  }
>  
>  MOZ_MUST_USE bool
>  js::ReadableStreamReaderReleaseLock(JSContext* cx, HandleObject readerObj)

And here.

@@ +4377,4 @@
>              auto cb = cx->runtime()->readableStreamWriteIntoReadRequestCallback;
>              MOZ_ASSERT(cb);
>              // TODO: use bytesWritten to correctly update the request's state.
> +            // TODO: make cross-compartment safe.

TODO!

::: js/src/builtin/Stream.h
@@ +17,5 @@
>  
> +class ReadableStreamReader : public NativeObject
> +{
> +  public:
> +    static const Class class_;

The class_ here is unused.

I think the JSObject::is specialization for this abstract class should be declared in this header file, so that it can be used in jsapi.cpp.

::: js/src/js.msg
@@ +612,4 @@
>  
>  // ReadableStream
>  MSG_DEF(JSMSG_READABLESTREAM_UNDERLYINGSOURCE_TYPE_WRONG,0, JSEXN_RANGEERR,"'underlyingSource.type' must be \"bytes\" or undefined.")
> +MSG_DEF(JSMSG_READABLESTREAM_BYTES_TYPE_NOT_IMPLEMENTED, 0, JSEXN_RANGEERR,"'underlyingSource.type' must be \"bytes\" or undefined.")

This needs a different error message.

::: js/src/jsapi-tests/testReadableStream.cpp
@@ +142,5 @@
> +    virtual JSContext* createContext() override {
> +        JSContext* cx = JS_NewContext(8L * 1024 * 1024);
> +        if (!cx)
> +            return nullptr;
> +        cx->options().setStreams(true);

As a style nit, it'd be better to call the base class method JSAPITest::createContext(), instead of the few lines of copy-and-paste here, but don't bother with it -- this option itself should be short-lived, right?

@@ +710,5 @@
> +    RootedObject stream(cx, NewDefaultStream(cx));
> +    CHECK(stream);
> +
> +
> +    uint32_t availableData;

This test file had a lot of compiler errors and warnings, so I didn't review it. Please update the patch.

::: js/src/jsapi.cpp
@@ +5317,5 @@
> +                                   JS::HandleObject proto /* = nullptr */)
> +{
> +    MOZ_ASSERT(!cx->runtime()->isAtomsCompartment(cx->compartment()));
> +    AssertHeapIsIdle();
> +    CHECK_REQUEST(cx);

Pre-existing, but I think this should also `assertSameCompartment(cx, underlyingSource, size, proto);`.

@@ +5358,5 @@
>  JS::IsReadableStream(const JSObject* obj)
>  {
> +    if (IsWrapper(const_cast<JSObject*>(obj)))
> +        obj = CheckedUnwrap(const_cast<JSObject*>(obj));
> +    return obj && obj->is<ReadableStream>();

Use IsMaybeWrapped<ReadableStream>(obj).

@@ +5366,5 @@
>  JS::IsReadableStreamReader(const JSObject* obj)
>  {
> +    if (IsWrapper(const_cast<JSObject*>(obj)))
> +        obj = CheckedUnwrap(const_cast<JSObject*>(obj));
> +    return obj && obj->is<ReadableStreamDefaultReader>();

Same.

Elsewhere in this review I claim that the JSObject::is<ReadableStreamReader> specialization should be in Streams.h. Here is one place where it would be nice to use it (otherwise, this code has a subtle gotcha).

@@ +5374,5 @@
>  JS::IsReadableStreamDefaultReader(const JSObject* obj)
>  {
> +    if (IsWrapper(const_cast<JSObject*>(obj)))
> +        obj = CheckedUnwrap(const_cast<JSObject*>(obj));
> +    return obj && obj->is<ReadableStreamDefaultReader>();

And again.

@@ +5601,2 @@
>  {
> +    reader = ToUnwrapped<NativeObject>(cx, const_cast<JSObject*>(reader));

Another place JSObject::is<ReadableStreamReader> should be used (indirectly, via ToUnwrapped<ReadableStreamReader>).

::: js/src/tests/whatwg/shell.js
@@ +24,5 @@
> +        var throwMethod;
> +        if (overrides && overrides.throw)
> +            throwMethod = overrides.throw;
> +        var iterator = {
> +            throw: throwMethod,

Simpler:

    let iterator = {
        throw() {
            throw new Error("throw method not implemented");
        },
        next(x) {
            return { done: false };
        },
        return(x) {
            return { done: true };
        },
        ...overrides
    };

@@ +42,5 @@
> +    };
> +})(this);
> +
> +if (typeof assertThrowsInstanceOf === 'undefined') {
> +    var assertThrowsInstanceOf = function assertThrowsInstanceOf(f, ctor, msg) {

Please move this directory under js/src/tests/non262 and remove the utility functions here that are already in non262/shell.js.

::: js/src/tests/whatwg/streams/readable-stream-globals.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("ReadableStream"))

Line 2 should read:

// Stream builtins must still work when applied to streams/readers/controllers from other globals.

I'd rather see 20 tiny tests and a shell.js with some shared code in it--it would be much easier to review, the filenames serve as documentation, it would be easier whenever someone has to cope with a test failure. (No change required, but below I will ask for either this change, or some comments.)

@@ +1,4 @@
> +// |reftest| skip-if(!this.hasOwnProperty("ReadableStream"))
> +
> +async function test() {
> +    if (typeof newGlobal !== 'undefined') {

newGlobal is always available in tests; there's a straightforward browser implementation:
https://searchfox.org/mozilla-central/source/js/src/tests/browser.js#164

@@ +7,5 @@
> +
> +    OtherReadableStream = otherGlobal.ReadableStream;
> +
> +    ReadableStreamReader = new ReadableStream().getReader().constructor;
> +    OtherReadableStreamReader = new otherGlobal.ReadableStream().getReader().constructor;

One cute trick for this kind of thing is:

    function testGlobal() {
        let g = newGlobal();
        g.StreamReader = new g.ReadableStream().getReader().constructor;
        ...etc...
        return g;
    }

    let A = testGlobal(), B = testGlobal();

Then you end up talking about `A.StreamReader` and `B.BYOBRequest` and so on, which is nice to read, and maybe easier to keep sorted mentally than BYOBRequest and OtherBYOBRequest (in particular the former is deceptively unqualified). Just a thought, no action required.

@@ +84,5 @@
> +    getFreshInstances();
> +    otherReader = new OtherReadableStreamReader(stream);
> +
> +    getFreshInstances();
> +    otherReader = new OtherReadableStreamReader(stream);

These two lines are repeated -- it seems like a mistake, or maybe you were planning to write another test.

@@ +85,5 @@
> +    otherReader = new OtherReadableStreamReader(stream);
> +
> +    getFreshInstances();
> +    otherReader = new OtherReadableStreamReader(stream);
> +    let cancelSucceeded = false;

This variable is unused.

@@ +92,5 @@
> +    assertEq(await cancelPromise, undefined);
> +
> +    getFreshInstances();
> +    otherReader = new OtherReadableStreamReader(stream);
> +    let closeSucceeded = false;

Same here.

@@ +140,5 @@
> +    // assertEq(request instanceof otherGlobal.Promise, true);
> +    // controller.enqueue(chunk);
> +    // assertEq((await request).value, chunk);
> +
> +    // reader.releaseLock();

I stopped reading a bit after this point. General comments:

-   It's great to see a significant bunch of tests.

-   It was unclear to me why some things are commented out. In particular,
    when I uncommented the test above, it just passed.

-   It seems like many of these methods return promises that could be
    awaited and an expected result asserted with one or two more lines,
    and it seems almost weird not to do it (?) but I'm probably just missing
    the purpose... Same comment on the jsapi-tests, fwiw.

-   Somewhere below, the tests got more and more opaque to me.
    It's probably a bad use of time to guess what each test is trying to do.
    Please add a minimal comment to each one, or put them in separate files
    so the filenames provide a clue. If the purpose of a test is just
    "don't crash calling Reader.prototype.blorp cross-realm", then
    that makes a fantastic comment!

@@ +226,5 @@
> +    assertEq(subPromiseCreated, false);
> +    assertEq(speciesInvoked, false);
> +    assertEq(otherGlobal.subPromiseCreated, true);
> +    assertEq(otherGlobal.speciesInvoked, true);
> +    await 1;

`await 1`? What does this do?

@@ +266,5 @@
> +    assertEq(otherGlobal.subPromiseCreated, true);
> +    assertEq(otherGlobal.speciesInvoked, true);
> +
> +    if (!byteStreamsSupported) {
> +        return;

It seems better to put the byte-stream tests in another file and have it reviewed with the byte-streams patch.

@@ +352,5 @@
> +async function runTest() {
> +    try {
> +        await test();
> +    } catch (e) {
> +        assertEq(false, true, `Unexpected exception ${e}\n${e.stack}`);

All an assertEq failure does is throw an exception.

If you want output in the log, I think you have to print() it, and then `throw e;` to avoid falling through to reportCompare(), which would make the test pass erroneously. Blah. But it's no use calling assertEq() like this.

::: js/src/vm/ArrayBufferObject.cpp
@@ +1950,2 @@
>          return nullptr;
> +    }

If possible, please land the changes to this file as a separate changeset, r=me.

::: js/src/vm/SelfHosting.cpp
@@ -35,3 @@
>  #include "builtin/TypedObject.h"
>  #include "builtin/WeakMapObject.h"
> -#include "gc/HashUtil.h"

How did you determine that these particular includes were safe to remove? Using IWYU? (To be clear: I'm asking because I would like to be able to do this too.)

::: package.json
@@ +9,5 @@
>      "eslint-plugin-mozilla": "file:tools/lint/eslint/eslint-plugin-mozilla",
>      "eslint-plugin-no-unsanitized": "3.0.0",
>      "eslint-plugin-react": "7.1.0",
> +    "eslint-plugin-spidermonkey-js": "file:tools/lint/eslint/eslint-plugin-spidermonkey-js",
> +    "npm": "^5.7.1"

This looks like it belongs in a separate commit, with a different reviewer.
Attachment #8967881 - Flags: review?(jorendorff)
Flags: needinfo?(till)
At the All Hands, we agreed that Jason will take over driving this into the tree.
Flags: needinfo?(till) → needinfo?(jorendorff)
Starting work on this Monday if not sooner.
Flags: needinfo?(jorendorff)
Hi, just checking on progress here. This issue is a dependency for work we're doing on Firefox Send.
Flags: needinfo?(jorendorff)
Attachment #8967881 - Attachment is obsolete: true
Flags: needinfo?(jorendorff)
Bug 1497686 - Better error message when a jsapi-test fails with an exception pending. r?jwalden
Differential Revision: https://phabricator.services.mozilla.com/D8154

Bug 1385890 - Fix Streams implementation in multiple-global uses.
Differential Revision: https://phabricator.services.mozilla.com/D8212

Bug 1385890 - TO FOLD - Remove unnecessary cast introduced in the previous patch. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8213

Bug 1385890 - TO FOLD - Fix up tests to pass. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8214

Bug 1385890 - TO FOLD - Fixup to compile without warnings on try server. r?jandem
Differential Revision: https://phabricator.services.mozilla.com/D8215

Bug 1385890 - TO FOLD - Fix downstream users of JS Stream APIs that became fallible. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8216

Bug 1385890 - TO FOLD - Streams tests no longer crash with assertions. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8217

Bug 1385890 - TO FOLD - Fix a rooting hazard (controller being a raw pointer) and another almost-a-hazard close call (byobRequestVal not being rooted across ToUnwrapped). r?tcampbell
Differential Revision: https://phabricator.services.mozilla.com/D8244

Bug 1389628 - Enable Streams API by default. r?baku
Assignee: till → jorendorff
Attachment #9016022 - Flags: feedback?(choller)
An "underlying source" is some JS or C++ object that represents the underlying
source of data for a ReadableStream. When the underlying source is a C++ object,
a pointer to it is stored as a PrivateValue in a slot on an object (the
stream's controller). This works fine as long as the address is even. For one
jsapi-test, we were using a global `char[]` variable as the underlying source
for some test streams. On linux32 this was given an odd address, causing
assertions; this patch fixes the test, documents the API quirk, and enforces it
with an assertion.
This is an automated crash issue comment:

Summary: Hit MOZ_CRASH(*** Compartment mismatch 0x7fbdbf004f80 vs. 0x7fbdbf005b80 at argument 0
) at js/src/vm/JSContext-inl.h:53

Build version: mozilla-central revision 725a692947dd+
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --ion-offthread-compile=off --enable-streams

Testcase:

otherGlobal = newGlobal();
function getFreshInstances(type, otherType = type) {
    stream = new ReadableStream({
        start(c) {
            controller = c;
        },
        type
    });
}
getFreshInstances();
let [branch1, branch2] = otherGlobal.ReadableStream.prototype.tee.call(stream);
cancelPromise1 = ReadableStream.prototype.cancel.call(branch1, {
    name: "cancel 1"
});
cancelPromise2 = ReadableStream.prototype.cancel.call(branch2, {
    name: "cancel 2"
});

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  MOZ_CrashPrintf (aFilename=aFilename@entry=0x555556903050 "js/src/vm/JSContext-inl.h", aLine=aLine@entry=53, aFormat=aFormat@entry=0x55555690bd68 "*** Compartment mismatch %p vs. %p at argument %d\n") at mfbt/Assertions.cpp:68
#1  0x00005555559413a6 in js::ContextChecks::fail (argIndex=0, c2=<optimized out>, c1=<optimized out>) at js/src/vm/JSContext-inl.h:52
#2  js::ContextChecks::check (this=this@entry=0x7fffffffc2a0, c=<optimized out>, argIndex=argIndex@entry=0) at js/src/vm/JSContext-inl.h:68
#3  0x0000555555961ab5 in js::ContextChecks::check (argIndex=0, c=<optimized out>, this=0x7fffffffc2a0) at js/src/vm/JSContext-inl.h:67
#4  js::ContextChecks::check (this=0x7fffffffc2a0, obj=<optimized out>, argIndex=0) at js/src/vm/JSContext-inl.h:82
#5  0x00005555559e4b41 in JSContext::checkImpl<JS::Handle<JSObject*>, JS::Handle<JS::Value> > (head=<synthetic pointer>, argIndex=0, this=0x7ffff5f18000) at js/src/vm/JSContext-inl.h:194
#6  JSContext::check<JS::Handle<JSObject*>, JS::Handle<JS::Value> > (this=0x7ffff5f18000) at js/src/vm/JSContext-inl.h:203
#7  ResolvePromiseInternal (cx=<optimized out>, promise=promise@entry=..., resolutionVal=resolutionVal@entry=...) at js/src/builtin/Promise.cpp:804
#8  0x00005555559e6d5f in js::PromiseObject::resolve (cx=<optimized out>, promise=promise@entry=..., resolutionValue=resolutionValue@entry=...) at js/src/builtin/Promise.cpp:4403
#9  0x0000555555ca0fc2 in ReadableStreamTee_Cancel (reason_=..., branch=..., teeState=..., cx=<optimized out>) at js/src/builtin/Stream.cpp:1460
#10 ReadableStreamControllerCancelSteps (reason=..., controller=..., cx=<optimized out>) at js/src/builtin/Stream.cpp:2920
#11 js::ReadableStream::cancel (cx=<optimized out>, stream=..., stream@entry=..., reason=...) at js/src/builtin/Stream.cpp:1710
#12 0x0000555555ca1340 in ReadableStream_cancel (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Stream.cpp:1083
#13 0x0000555555969b25 in CallJSNative (cx=0x7ffff5f18000, native=0x555555ca1270 <ReadableStream_cancel(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:461
#14 0x000055555595c167 in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f18000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:553
#15 0x000055555595c78d in InternalCall (cx=cx@entry=0x7ffff5f18000, args=...) at js/src/vm/Interpreter.cpp:607
#16 0x000055555595c910 in js::Call (cx=cx@entry=0x7ffff5f18000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:626
#17 0x0000555555ff8648 in js::fun_call (cx=0x7ffff5f18000, argc=<optimized out>, vp=<optimized out>) at js/src/vm/JSFunction.cpp:1308
#18 0x0000555555969b25 in CallJSNative (cx=0x7ffff5f18000, native=0x555555ff83f0 <js::fun_call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:461
[...]
#32 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:10703
rax	0x5555577f8520	93825028556064
rbx	0x5555577f8580	93825028556160
rcx	0x7ffff6c1c2dd	140737333281501
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffc090	140737488339088
rsp	0x7fffffffbfa0	140737488338848
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x35	53
r13	0x7ffff5f18000	140737319632896
r14	0x7fffffffc2a0	140737488339616
r15	0x7fffffffc440	140737488340032
rip	0x5555557de7eb <MOZ_CrashPrintf(char const*, int, char const*, ...)+303>
=> 0x5555557de7eb <MOZ_CrashPrintf(char const*, int, char const*, ...)+303>:	movl   $0x0,0x0
   0x5555557de7f6 <MOZ_CrashPrintf(char const*, int, char const*, ...)+314>:	ud2



Please also note that the Summary line at the beginning of the comment is not a typo: The compartment mismatch message ends with a linebreak even though it is outputted in MOZ_CRASH. This should be fixed as well because it breaks the assertion/MOZ_CRASH parser and makes the message invisible to automation.
Streams have multiple parts that are JS objects, and different parts can be in
different realms or even different compartments. (That is what this bug is
about in a nutshell.)
Differential Revision: https://phabricator.services.mozilla.com/D8212

Bug 1385890 - TO FOLD - Remove unnecessary cast introduced in the previous patch. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8213

Bug 1385890 - TO FOLD - Fix up tests to pass. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8214

Bug 1385890 - TO FOLD - Fixup to compile without warnings on try server. r?jandem
Differential Revision: https://phabricator.services.mozilla.com/D8215

Bug 1385890 - TO FOLD - Fix downstream users of JS Stream APIs that became fallible. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8216

Bug 1385890 - TO FOLD - Streams tests no longer crash with assertions. r?baku
Differential Revision: https://phabricator.services.mozilla.com/D8217

Bug 1385890 - TO FOLD - Fix a rooting hazard (controller being a raw pointer) and another almost-a-hazard close call (byobRequestVal not being rooted across ToUnwrapped). r?tcampbell
Differential Revision: https://phabricator.services.mozilla.com/D8244

Bug 1385890 - TO FOLD - Assert underlying source pointers are aligned so they can be stored using PrivateValue(). r?tcampbell
Differential Revision: https://phabricator.services.mozilla.com/D8315

Bug 1385890 - TO FOLD - Fix a compartment bug. r?tcampbell
Differential Revision: https://phabricator.services.mozilla.com/D8426
Comment on attachment 9016363 [details] [diff] [review]
omnibus patch for fuzzers, v2

Note: with this omnibus patch, you still need to run the shell with --enable-streams to get ReadableStream and friends.
Attachment #9016363 - Attachment description: Fix Streams implementation in multiple-global uses → omnibus patch for fuzzers, v2
Attachment #9016363 - Flags: feedback?(nth10sd)
Attachment #9016363 - Flags: feedback?(choller)
Attachment #9016022 - Attachment is obsolete: true
Attachment #9016022 - Flags: feedback?(choller)
Streams have multiple parts that can be JS objects from different compartments.
For example, the [[reader]] internal slot of a stream can point to a reader
object in another compartment.

This patch makes the ReadableStream implementation robust against mixing and
matching stream-related objects and methods from different globals.

This also removes ReadableStreamBYOBReader and ReadableStreamBYOBRequest for
now, with a view toward enabling basic ReadableStream features by default in
bug 1389628.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f93ead3508287a3fe044d35e4913e2f9436f6d
Bug 1385890 - Fix Streams implementation in multiple-global uses. r=baku,tcampbell,jorendorff
https://hg.mozilla.org/mozilla-central/rev/a4f93ead3508
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9016363 [details] [diff] [review]
omnibus patch for fuzzers, v2

Canceling feedback because this already landed on m-c now.
Attachment #9016363 - Flags: feedback?(nth10sd)
Attachment #9016363 - Flags: feedback?(choller)
Attachment #9015882 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.