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

RESOLVED FIXED in Firefox 64

Status

()

defect
P1
critical
RESOLVED FIXED
Last year
10 months ago

People

(Reporter: gkw, Assigned: jorendorff)

Tracking

(Blocks 2 bugs, 5 keywords)

Trunk
mozilla64
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 wontfix, firefox60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 fixed)

Details

(Whiteboard: [jsbugmon:], crash signature)

Attachments

(3 attachments)

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.
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.
Blocks: 1272697
Flags: needinfo?(till)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Crash Signature: [@ JS::Rooted]
Summary: Assertion failure: isInt32(), at dist/include/js/Value.h:649 → Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649
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
Jason, does this still apply? I think you were mentioning removing the --enable-stream option, so this probably needs to be resolved?
Flags: needinfo?(jorendorff)
Bug still exists, taking.
Assignee: nobody → jorendorff
Priority: -- → P1
Flags: needinfo?(till)
Closing because no crash reported since 12 weeks.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → WONTFIX
Closing because no crash reported since 12 weeks.
Reopening based on comment 5.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Flags: needinfo?(jorendorff)
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.
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)
Flags: needinfo?(jorendorff)
QA Contact: sdetar
Summary: Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649 → Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649 with --enable-stream
(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.
QA Contact: sdetar
Attachment #9014214 - Attachment description: Bug 1445854 - The class of most builtin prototype objects should be PlainObject::class_. r?jwalden → Bug 1445854 - Part 1: Make GenericCreatePrototype use protoClass_. r?jwalden
(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.
Attachment #9014215 - Attachment description: Bug 1445854 - Remove extra code to cope with prior bad behavior of GenericCreatePrototype. r?jwalden → Bug 1445854 - Part 2: Remove extra code to cope with prior bad behavior of GenericCreatePrototype. r?jwalden
Pushed by jorendorff@mozilla.com:
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
leave-open, since I'm autolanding the two parts that have review.
Keywords: leave-open
Flags: needinfo?(jorendorff)
Keywords: leave-open
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8c1b5582913
Part 3: Use GenericCreatePrototype in more places. r=arai
https://hg.mozilla.org/mozilla-central/rev/ed9091a418b9
https://hg.mozilla.org/mozilla-central/rev/8de0d349f144
https://hg.mozilla.org/mozilla-central/rev/b8c1b5582913
Status: REOPENED → RESOLVED
Closed: 10 months ago10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Summary: Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649 with --enable-stream → Crash [@ JS::Rooted] or Assertion failure: isInt32(), at dist/include/js/Value.h:649 with --enable-streams
You need to log in before you can comment on or make changes to this bug.