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
a year ago
8 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.
Reporter

Comment 1

a year ago
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)

Updated

a year ago
Whiteboard: [jsbugmon:update] → [jsbugmon:]

Comment 2

a year ago
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Reporter

Updated

a year ago
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
Reporter

Comment 3

a year ago
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)
Assignee

Comment 5

8 months ago
Bug still exists, taking.
Assignee: nobody → jorendorff
Priority: -- → P1
Assignee

Updated

8 months ago
Flags: needinfo?(till)
Closing because no crash reported since 12 weeks.
Status: NEW → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → WONTFIX
Closing because no crash reported since 12 weeks.
Reopening based on comment 5.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Assignee

Updated

8 months ago
Flags: needinfo?(jorendorff)
Assignee

Comment 9

8 months ago
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.
Reporter

Comment 12

8 months ago
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
Reporter

Updated

8 months ago
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
Assignee

Comment 14

8 months ago
(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

Comment 15

8 months ago
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
Assignee

Comment 16

8 months ago
leave-open, since I'm autolanding the two parts that have review.
Keywords: leave-open
Assignee

Updated

8 months ago
Flags: needinfo?(jorendorff)
Keywords: leave-open

Comment 17

8 months ago
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8c1b5582913
Part 3: Use GenericCreatePrototype in more places. r=arai

Comment 18

8 months ago
bugherder
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
Last Resolved: 8 months ago8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee

Updated

8 months ago
Reporter

Updated

8 months ago
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.