1293285 - Crash [@ JS::Rooted<JSObject*>::set] with Promise

Reporter

Description

•

8 years ago

The following testcase crashes on mozilla-central revision e78975b53563 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

let results = [];
let p1 = new Promise(res => res('result')).then(val => {}).then(val => {
    results.push(drainJobQueue());
});


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000044f634 in JS::Rooted<JSObject*>::set (this=0x7fffffffc820, value=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/RootingAPI.h:705
#0  0x000000000044f634 in JS::Rooted<JSObject*>::set (this=0x7fffffffc820, value=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/RootingAPI.h:705
#1  JS::Rooted<JSObject*>::operator= (p=<optimized out>, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/RootingAPI.h:710
#2  DrainJobQueue (cx=cx@entry=0x7ffff6965000) at js/src/shell/js.cpp:662
#3  0x000000000044fb95 in DrainJobQueue (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=0x7fffed063098) at js/src/shell/js.cpp:680
#4  0x0000000000ae0bfb in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0x44fb70 <DrainJobQueue(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#5  0x0000000000ad1003 in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453
#6  0x0000000000acbd18 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:504
#7  Interpret (cx=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:2873
#8  0x0000000000ad0e49 in js::RunScript (cx=cx@entry=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:399
#9  0x0000000000ad1108 in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:471
#10 0x0000000000ad1356 in InternalCall (cx=<optimized out>, args=...) at js/src/vm/Interpreter.cpp:498
#11 0x0000000000ad14ae in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:517
#12 0x0000000000c367d7 in js::PromiseReactionJob (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=0x7fffffffd4b8) at js/src/builtin/Promise.cpp:545
#13 0x0000000000ae0bfb in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xc36510 <js::PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#22 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:7516
rax	0x0	0
rbx	0x0	0
rcx	0x7fffffffc800	140737488340992
rdx	0x7fffffffcb00	140737488341760
rsi	0x7ffff6952800	140737330358272
rdi	0x7ffff6965000	140737330434048
rbp	0x7fffffffc8b0	140737488341168
rsp	0x7fffffffc7c0	140737488340928
r8	0x9a	154
r9	0x7ffff6953920	140737330362656
r10	0x1d74640	30885440
r11	0x1d747f0	30885872
r12	0x7ffff6992000	140737330618368
r13	0x0	0
r14	0x7ffff6965000	140737330434048
r15	0x7ffff6992088	140737330618504
rip	0x44f634 <DrainJobQueue(JSContext*)+388>
=> 0x44f634 <DrainJobQueue(JSContext*)+388>:	mov    (%rax),%rax
   0x44f637 <DrainJobQueue(JSContext*)+391>:	mov    0x10(%rax),%rsi

Fuzzing Team

Updated

•

8 years ago

Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Fuzzing Team

Comment 1

•

8 years ago

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d4cf63e47ae9
user:        Till Schneidereit
date:        Thu Jul 21 00:44:16 2016 +0200
summary:     Bug 911216 - Part 30: Enable SpiderMonkey Promise implementation. r=bz,efaust,bholley,Paolo,tromey,shu

This iteration took 0.851 seconds to run.

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

Updated

•

8 years ago

Flags: needinfo?(till)

ovidiu boca[:Ovidiu]

Updated

•

8 years ago

status-firefox48: --- → unaffected

status-firefox49: --- → unaffected

status-firefox51: --- → affected

Till Schneidereit [:till]

Assignee

Comment 2

•

8 years ago

Attached patch Ignore reentrant calls to the JS shell's drainJobQueue function — Details — Splinter Review

Yeah, this is a bit silly: drainJobQueue doesn't handle recursive calls. A real embedding wouldn't have those - what with run to completion semantics and all - so in theory we could just assert against it. In practice, that'd require marking drainJobQueue as non-fuzzing-safe. We do want fuzzers to call it, though, because otherwise they couldn't properly test promises. So instead, let's just ignore reentrant calls.

Assignee: nobody → till

Status: NEW → ASSIGNED

Flags: needinfo?(till)

Attachment #8780274 - Flags: review?(efaustbmo)

Eric Faust [:efaust]

Comment 3

•

8 years ago

Comment on attachment 8780274 [details] [diff] [review]
Ignore reentrant calls to the JS shell's drainJobQueue function

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

::: js/src/shell/js.cpp
@@ +658,5 @@
> +    // It doesn't make sense for job queue draining to be reentrant. At the
> +    // same time we don't want to assert against it, because that'd make
> +    // drainJobQueue unsafe for fuzzers. We do want fuzzers to test this, so
> +    // we simply ignore nested calls of drainJobQueue.
> +    sc->drainingJobQueue = true;

wish we had a way to assert we could only hit the reentrant case in the shell. :/

Attachment #8780274 - Flags: review?(efaustbmo) → review+

Christian Holler (:decoder)

Reporter

Comment 4

•

8 years ago

This is an automated crash issue comment:

Summary: Assertion failure: aIndex < mLength, at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/mozilla/Vector.h:459
Build version: mozilla-central revision f97a056ae623
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize
Runtime options: --fuzzing-safe --ion-offthread-compile=off

Testcase:

new Promise(() => rejset).catch(drainJobQueue)


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000045b319 in mozilla::Vector<JSObject*, 0ul, js::SystemAllocPolicy>::operator[] (aIndex=0, this=0x7ffff69800a8) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/mozilla/Vector.h:459
#0  0x000000000045b319 in mozilla::Vector<JSObject*, 0ul, js::SystemAllocPolicy>::operator[] (aIndex=0, this=0x7ffff69800a8) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/mozilla/Vector.h:459
#1  JS::GCVector<JSObject*, 0ul, js::SystemAllocPolicy>::operator[] (i=0, this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/GCVector.h:64
#2  js::MutableGCVectorOperations<JS::PersistentRooted<JS::GCVector<JSObject*, 0ul, js::SystemAllocPolicy> >, JSObject*, 0ul, js::SystemAllocPolicy>::operator[] (aIndex=0, this=0x7ffff6980088) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/GCVector.h:175
#3  DrainJobQueue (cx=cx@entry=0x7ffff695f000) at js/src/shell/js.cpp:722
#4  0x000000000045b395 in DrainJobQueue (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=0x7fffffdfec28) at js/src/shell/js.cpp:734
#5  0x0000000000ae60f9 in js::CallJSNative (cx=cx@entry=0x7ffff695f000, native=0x45b370 <DrainJobQueue(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#6  0x0000000000ad6323 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453
#7  0x0000000000ad6656 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:498
#8  0x0000000000ad67ae in js::Call (cx=cx@entry=0x7ffff695f000, fval=..., fval@entry=..., thisv=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:517
#9  0x0000000000c38f93 in js::PromiseReactionJob (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=0x7fffffdfee98) at js/src/builtin/Promise.cpp:545
#10 0x0000000000ae60f9 in js::CallJSNative (cx=cx@entry=0x7ffff695f000, native=0xc38ce0 <js::PromiseReactionJob(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
#11 0x0000000000ad6323 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff695f000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453
#12 0x0000000000ad6656 in InternalCall (cx=cx@entry=0x7ffff695f000, args=...) at js/src/vm/Interpreter.cpp:498
#13 0x0000000000ad67ae in js::Call (cx=cx@entry=0x7ffff695f000, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:517
#14 0x00000000008ec271 in JS::Call (cx=cx@entry=0x7ffff695f000, thisv=..., thisv@entry=..., fval=..., fval@entry=..., args=..., rval=..., rval@entry=...) at js/src/jsapi.cpp:2840
#15 0x000000000045b09c in JS::Call (rval=..., args=..., funObj=..., thisv=..., cx=0x7ffff695f000) at js/src/jsapi.h:3300
#16 DrainJobQueue (cx=cx@entry=0x7ffff695f000) at js/src/shell/js.cpp:720
#17 0x000000000045b395 in DrainJobQueue (cx=cx@entry=0x7ffff695f000, argc=<optimized out>, vp=0x7fffffdff2e8) at js/src/shell/js.cpp:734
#18 0x0000000000ae60f9 in js::CallJSNative (cx=cx@entry=0x7ffff695f000, native=0x45b370 <DrainJobQueue(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]


Is this the same issue? Also recursing around DrainJobQueue but with an assertion instead.

Till Schneidereit [:till]

Assignee

Comment 5

•

8 years ago

(In reply to Eric Faust [:efaust] from comment #3)
> wish we had a way to assert we could only hit the reentrant case in the
> shell. :/

That's not strictly true: Gecko has ways to spin a nested event loop draining the micro-task queue from script, too. That entirely different mechanism has its own ways to prevent this particular issue, though.

(In reply to Christian Holler (:decoder) from comment #4)
> Is this the same issue? Also recursing around DrainJobQueue but with an
> assertion instead.

It is, yes.

Pulsebot

Comment 6

•

8 years ago

Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/814753913f49
Ignore reentrant calls to the JS shell's drainJobQueue function. r=efaust

Ryan VanderMeulen [:RyanVM]

Comment 7

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/814753913f49

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox51: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla51

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Comment 8

•

8 years ago

Hi Till, I wasn't able to find any instances of this crash in the past week on Aurora50. However, since status-firefox50 is affected, should we consider uplifting this to Aurora?

Flags: needinfo?(till)

Till Schneidereit [:till]

Assignee

Comment 9

•

8 years ago

(In reply to Ritu Kothari (:ritu) from comment #8)
> Hi Till, I wasn't able to find any instances of this crash in the past week
> on Aurora50. However, since status-firefox50 is affected, should we consider
> uplifting this to Aurora?

I don't think we need to uplift this: it only affects the shell, not the browser, so can't have any impact on our users.

Flags: needinfo?(till)

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Comment 10

•

8 years ago

(In reply to Till Schneidereit [:till] from comment #9)
> (In reply to Ritu Kothari (:ritu) from comment #8)
> > Hi Till, I wasn't able to find any instances of this crash in the past week
> > on Aurora50. However, since status-firefox50 is affected, should we consider
> > uplifting this to Aurora?
> 
> I don't think we need to uplift this: it only affects the shell, not the
> browser, so can't have any impact on our users.

Got it. Thanks. This is now a wontfix for Fx50.

status-firefox50: affected → wontfix

Bugzilla

Quick Search

Crash [@ JS::Rooted<JSObject*>::set] with Promise

Categories

(Core :: JavaScript Engine, defect)

Tracking

()

People

(Reporter: decoder, Assigned: till)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Security

(public)

User Story

Attachments

(1 file)

Description

Updated

Comment 1

Updated

Updated

Comment 2

Comment 3

Comment 4

Comment 5

Comment 6

Comment 7

Comment 8

Comment 9

Comment 10

Attachment

General

Description

File Name

Content Type