1445854 - Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649 with --enable-streams

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

The following testcase crashes on mozilla-central revision 8863806b9e28 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion --enable-streams):

ReadableStream.prototype.tee()

Backtrace:

(gdb) bt
#0  JS::Value::toInt32 (this=<optimized out>) at /home/winworklin/shell-cache/js-dbg-64-dm-linux-8863806b9e28/objdir-js/dist/include/js/Value.h:649
#1  StreamState (stream=<optimized out>) at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:133
#2  js::ReadableStream::readable (this=<optimized out>) at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:147
#3  0x00000000008a2839 in ReadableStreamReaderGenericInitialize (cx=cx@entry=0x7ffffd416000, reader=reader@entry=..., stream=stream@entry=...)
    at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:2036
#4  0x00000000008a2a2f in CreateReadableStreamDefaultReader (cx=0x7ffffd416000, stream=stream@entry=...)
    at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:1616
#5  0x00000000008a3523 in ReadableStreamTee (cx=0x7ffffd416000, stream=..., stream@entry=..., cloneForBranch2=false, branch1Stream=branch1Stream@entry=..., 
    branch2Stream=..., branch2Stream@entry=..., cloneForBranch2=false) at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:1202
#6  0x00000000008a3c70 in ReadableStream_tee_impl (cx=cx@entry=0x7ffffd416000, args=...)
    at /home/winworklin/trees/mozilla-central/js/src/builtin/Stream.cpp:924
#7  0x00000000008a3e8b in JS::CallNonGenericMethod<Is<js::ReadableStream>, ReadableStream_tee_impl> (args=..., cx=0x7ffffd416000)
/snip

For detailed crash information, see attachment.

This seems to only occur with --enable-streams. Not sure if this is ready for fuzzing yet.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/ed1427496354
user:        Till Schneidereit
date:        Tue Jan 31 14:34:55 2017 +0100
summary:     Bug 1272697 - Part 3: Implement ReadableStream and associated classes in the JS engine. r=shu

Till, is bug 1272697 a likely regressor?

Note that I will stop testing --enable-streams until this is fixed.

Comment 2

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Some variants seem to crash opt builds at JS::Rooted with the following stack trace:

#0  0x0000000000742799 in JS::Rooted<JSObject*>::registerWithRootLists (roots=..., this=<optimized out>) at /home/ubuntu/shell-cache/js-64-linux-c56ef1c14a55/objdir-js/dist/include/js/RootingAPI.h:942
#1  JS::Rooted<JSObject*>::Rooted<JSContext*> (cx=<optimized out>, this=<optimized out>) at /home/ubuntu/shell-cache/js-64-linux-c56ef1c14a55/objdir-js/dist/include/js/RootingAPI.h:961
#2  js::ReadableStream::cancel (cx=cx@entry=0x7f8d14815000, stream=..., stream@entry=..., reason=...) at /home/ubuntu/trees/mozilla-central/js/src/builtin/Stream.cpp:1383
#3  0x00000000007431f7 in ReadableStream_cancel (cx=0x7f8d14815000, argc=0, vp=0x7ffd2998adc8) at /home/ubuntu/trees/mozilla-central/js/src/builtin/Stream.cpp:822
#4  0x0000000000529152 in js::CallJSNative (args=..., native=0x7430b0 <ReadableStream_cancel(JSContext*, unsigned int, JS::Value*)>, cx=0x7f8d14815000) at /home/ubuntu/trees/mozilla-central/js/src/vm/JSContext-inl.h:290
#5  js::InternalCallOrConstruct (cx=cx@entry=0x7f8d14815000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at /home/ubuntu/trees/mozilla-central/js/src/vm/Interpreter.cpp:467

Comment 4

[Ted Campbell [:tcampbell]]

Jason, does this still apply? I think you were mentioning removing the --enable-stream option, so this probably needs to be resolved?

Comment 5

[Jason Orendorff [:jorendorff]]

Bug still exists, taking.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Closing because no crash reported since 12 weeks.

Comment 8

[Christian Holler (:decoder)]

Reopening based on comment 5.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1445854 - Part 1: Make GenericCreatePrototype use protoClass_. r?jwalden

Originally, long ago, the builtin prototype object of a class was always an
actual instance of that class. For the oldest classes, this is still true.
Date.prototype is a Date; Function.prototype is a Function, and so on. These
objects have the private fields and weird special abilities of their
corresponding classes. Array.isArray(Array.prototype) is true.

As it turns out, this is a bad idea. Prototypes are a lot like uninitialized
objects; thus it was a common bug to have code like

    if (!obj->is<Widget>()) {  // safety check
        return ThrowTypeError(cx, ...);
    }
    obj->as<Widget>()->getWidgetPrivateData()->doThings();  // BUG

This would crash when obj happened to be Widget.prototype, because in that
case `getWidgetPrivateData()` would typically return null. Extra checks
everywhere.

It was a pain for the spec, too. The standard committee has stopped making
prototype objects special in this way. The newer ones are just plain objects
with no internal slots.

The bad practice that causes such bugs is still present in this little-used
code. Time to get rid of it.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1445854 - Part 2: Remove extra code to cope with prior bad behavior of GenericCreatePrototype. r?jwalden

Depends on D7666

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1445854 - Part 3: Use GenericCreatePrototype in more places. r?arai

Depends on D7667

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Does this mean that we should/should not test with --enable-stream? Or will it eventually be removed?

(just wondering about its value as it has turned up old bugs like this)

Comment 13

[Christian Holler (:decoder)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #12)
> Does this mean that we should/should not test with --enable-stream? Or will
> it eventually be removed?
> 

imho, once this feature is supposed to be tested, the build flag guarding it should be converted to a runtime flag instead because it would add yet another build configuration otherwise.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Christian Holler (:decoder) from comment #13)
> (In reply to Gary Kwong [:gkw] [:nth10sd] from comment #12)
> > Does this mean that we should/should not test with --enable-stream? Or will
> > it eventually be removed?
> 
> imho, once this feature is supposed to be tested, the build flag guarding it
> should be converted to a runtime flag instead because it would add yet
> another build configuration otherwise.

The build flag is already gone; the last traces of it were removed in bug 1491939.

Gary's referring to the runtime JS shell flag, which is also called --enable-streams.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed9091a418b9
Part 1: Make GenericCreatePrototype use protoClass_. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/8de0d349f144
Part 2: Remove extra code to cope with prior bad behavior of GenericCreatePrototype. r=jwalden

Comment 16

[Jason Orendorff [:jorendorff]]

leave-open, since I'm autolanding the two parts that have review.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8c1b5582913
Part 3: Use GenericCreatePrototype in more places. r=arai

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/ed9091a418b9
https://hg.mozilla.org/mozilla-central/rev/8de0d349f144
https://hg.mozilla.org/mozilla-central/rev/b8c1b5582913