Closed Bug 1366927 Opened 3 years ago Closed 3 years ago

Assertion failure: ssc <= c, at js/src/jsapi.h:4172 or Assertion failure: begin + len <= length(), at jsscript.cpp:1691

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- fixed

People

(Reporter: decoder, Assigned: shu)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update])

Attachments

(1 file)

The following testcase crashes on mozilla-central revision 9851fcb0bf4d (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads):

evaluate(`
  var f = x => saveStack().column; 
  f( ... of  => () => x.toString("foo"))
`, { columnNumber: 1729 })



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000a96138 in JS::CompileOptions::setColumn (ssc=<optimized out>, c=<optimized out>, this=<optimized out>) at js/src/jsapi.h:4172
#0  0x0000000000a96138 in JS::CompileOptions::setColumn (ssc=<optimized out>, c=<optimized out>, this=<optimized out>) at js/src/jsapi.h:4172
#1  js::frontend::CompileLazyFunction (cx=cx@entry=0x7ffff6924000, lazy=lazy@entry=..., chars=0x7ffff5b314dc u"of  => () => x.toString(\"foo\"))\n", length=length@entry=30) at js/src/frontend/BytecodeCompiler.cpp:652
#2  0x00000000009a6046 in JSFunction::createScriptForLazilyInterpretedFunction (cx=cx@entry=0x7ffff6924000, fun=fun@entry=...) at js/src/jsfun.cpp:1532
#3  0x00000000004691ec in JSFunction::getOrCreateScript (cx=cx@entry=0x7ffff6924000, fun=..., fun@entry=...) at js/src/jsfun.h:429
#4  0x00000000009a5b70 in js::LazyScript::functionDelazifying (script=..., cx=0x7ffff6924000) at js/src/jsscriptinlines.h:82
#5  JSFunction::createScriptForLazilyInterpretedFunction (cx=0x7ffff6924000, fun=fun@entry=...) at js/src/jsfun.cpp:1484
#6  0x00000000004691ec in JSFunction::getOrCreateScript (cx=<optimized out>, fun=...) at js/src/jsfun.h:429
#7  0x00000000009aac48 in js::FunctionToString (cx=cx@entry=0x7ffff6924000, fun=..., prettyPrint=prettyPrint@entry=false) at js/src/jsfun.cpp:995
#8  0x00000000009ab140 in fun_toStringHelper (cx=cx@entry=0x7ffff6924000, obj=..., obj@entry=..., indent=indent@entry=32768) at js/src/jsfun.cpp:1132
#9  0x00000000009ca812 in fun_toSource (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/jsfun.cpp:1185
#10 0x000000000053e0af in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0x9ca680 <fun_toSource(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#11 0x0000000000532e83 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6924000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:470
#12 0x0000000000533298 in InternalCall (cx=cx@entry=0x7ffff6924000, args=...) at js/src/vm/Interpreter.cpp:515
#13 0x00000000005333cd in js::Call (cx=cx@entry=0x7ffff6924000, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:534
#14 0x0000000000a0d1ea in js::Call (rval=..., thisObj=<optimized out>, fval=..., cx=0x7ffff6924000) at js/src/vm/Interpreter.h:94
#15 js::ValueToSource (cx=cx@entry=0x7ffff6924000, v=..., v@entry=...) at js/src/jsstr.cpp:3543
#16 0x0000000000a11fa2 in js::DecompileValueGenerator (cx=cx@entry=0x7ffff6924000, spindex=<optimized out>, v=..., fallbackArg=..., skipStackHits=skipStackHits@entry=0) at js/src/jsopcode.cpp:2353
#17 0x000000000094ff88 in js::ReportValueErrorFlags (cx=0x7ffff6924000, flags=0, errorNumber=53, spindex=<optimized out>, v=..., fallback=..., arg1=0x0, arg2=0x0) at js/src/jscntxt.cpp:1028
#18 0x000000000052dde3 in Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:3025
[...]
#24 0x000000000045d97e in Evaluate (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/shell/js.cpp:1652
#25 0x000000000053e0af in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0x45cfb0 <Evaluate(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
[...]
#38 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8463
rax	0x0	0
rbx	0x7fffffffb510	140737488336144
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffb4b0	140737488336048
rsp	0x7fffffffaaf0	140737488333552
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x58	88
r11	0x7ffff6b9f750	140737332770640
r12	0x7ffff6924000	140737330167808
r13	0x7fffffffabe0	140737488333792
r14	0x6c1	1729
r15	0x9	9
rip	0xa96138 <js::frontend::CompileLazyFunction(JSContext*, JS::Handle<js::LazyScript*>, char16_t const*, unsigned long)+1864>
=> 0xa96138 <js::frontend::CompileLazyFunction(JSContext*, JS::Handle<js::LazyScript*>, char16_t const*, unsigned long)+1864>:	movl   $0x0,0x0
   0xa96143 <js::frontend::CompileLazyFunction(JSContext*, JS::Handle<js::LazyScript*>, char16_t const*, unsigned long)+1875>:	ud2
NI for :shu per IRC discussion :)
Flags: needinfo?(shu)
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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/52459cfd1576
user:        Jan de Mooij
date:        Fri Sep 23 12:55:14 2016 +0200
summary:     Bug 1304390 - Compress/decompress script sources in chunks. r=luke

This iteration took 229.124 seconds to run.
Summary: Assertion failure: ssc <= c, at js/src/jsapi.h:4172 → Assertion failure: ssc <= c, at js/src/jsapi.h:4172 or Assertion failure: begin + len <= length(), at jsscript.cpp:1691
The idea is this:

Offsets, like token positions and scripts' begin/end and
toString{Start,End}, are now always offset from the beginning of the
"root" ScriptSource buffer. That is, when delazifying, if the buffer we
are parsing is an interior pointer into some parent ScriptSource buffer,
the token offsets for that parsing session are still offset from that
parent buffer instead of the interior pointer.

The initial column number in CompileOptions will be added to the column
number when an offset maps to the initial line of the ScriptSource.
Columns are computed from offsets, but have no bearing on how to index
into the ScriptSource buffers.
Attachment #8872855 - Flags: review?(jimb)
Flags: needinfo?(shu)
Assignee: nobody → shu
I've started looking at this. It's pretty big, and I have some other urgent stuff at the moment, so I'm aiming to have this done by Wed 6-14, next week.
Hey Jim, had a chance to look at this yet?
Flags: needinfo?(jimb)
Sorry, I bounced off this and got distracted by other things. Reviewing this patch is my primary goal now.
Flags: needinfo?(jimb)
Reduced test case:

evaluate("\n(y => 1)()", { columnNumber: 1729 })
Comment on attachment 8872855 [details] [diff] [review]
Rework column handling in frontend by separating column from offset from root ScriptSource buffer.

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

Okay, I think I've got my head around the change this is making. If some of the comments sound clueless, please point that out to me.

Please include the reduced test case in the prior comment in the revised patch.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +10703,5 @@
>          // correct toString output.
>          //
>          // Token positions are already offset from the start column. Since
>          // toString offsets are absolute offsets into the ScriptSource,
>          // de-offset from the starting column.

Does this comment need to come out?

@@ +10705,5 @@
>          // Token positions are already offset from the start column. Since
>          // toString offsets are absolute offsets into the ScriptSource,
>          // de-offset from the starting column.
> +        ptrdiff_t classStart = ptrdiff_t(pn->pn_pos.begin);
> +        ptrdiff_t classEnd = ptrdiff_t(pn->pn_pos.end);

So great.

::: js/src/frontend/Parser.cpp
@@ +3016,5 @@
>          // are parenFreeArrow, we will set this below, after consuming the NAME.
>          funbox->setStart(tokenStream);
> +    } else {
> +        // When delazifying, we may not have a current token and pos() is
> +        // garbage. In that case, substitute the first token's position.

This is a little worrisome, because it suggests that ParserBase can be in a state where calling `pos` returns garbage. If this spot change is necessary, it makes me worry there could be others we don't know about. Is there any way to assert in pos that it won't return garbage?

(I recognize this is probably a pre-existing condition.)

@@ +3248,5 @@
>          funbox->setIsExprBody();
>  
>      PropagateTransitiveParseFlags(lazy, pc->sc());
>  
> +    if (!tokenStream.advance(fun->lazyScript()->end()))

So great.

::: js/src/frontend/SharedContext.h
@@ -577,5 @@
>  
>      void setEnd(const TokenStream& tokenStream) {
> -        // For all functions except class constructors, the buffer and
> -        // toString ending positions are the same. Class constructors override
> -        // the toString ending position with the end of the class definition.

Does this comment really no longer apply? It seems like you haven't changed the meanings of these positions.

::: js/src/frontend/TokenStream.cpp
@@ +260,5 @@
>      // which fixes the GCC/clang error, but causes bustage on Windows.  Sigh.
>      //
>      uint32_t maxPtr = MAX_PTR;
>  
> +    // The first line begins at buffer offset |initialLineOffset|.  MAX_PTR is

There is a comment in TokenStream.h still claiming it's always zero, which should be changed as well.

Would it also make sense to have SourceCoords::fill assert that other and this have the same value for lineStartOffsets_[0]?

::: js/src/frontend/TokenStream.h
@@ +510,5 @@
>          uint32_t lineIndexToNum(uint32_t lineIndex) const { return lineIndex + initialLineNum_; }
>          uint32_t lineNumToIndex(uint32_t lineNum)   const { return lineNum   - initialLineNum_; }
> +        uint32_t lineIndexAndOffsetToColumn(uint32_t lineIndex, uint32_t offset) const {
> +            uint32_t lineStartOffset = lineStartOffsets_[lineIndex];
> +            MOZ_ASSERT(offset >= lineStartOffset);

When we could assume the first line offset was zero, then we knew that lineIndexOf would always return a line index whose offset was <= the given offset, no matter what wild offset we passed in. But now that we can specify other first line offsets, passing in offsets before lineStartOffsets_[0] in non-DEBUG builds will cause this function to return negative (or rather, gigantic positive) values in response to wild offsets. For that reason, I'd recommend making this a release assert.

Passing such offsets to lineIndexOf should just return zero, so there's no risk there.

@@ +514,5 @@
> +            MOZ_ASSERT(offset >= lineStartOffset);
> +            uint32_t column = offset - lineStartOffset;
> +            if (lineIndex == 0)
> +                return column + initialColumn_;
> +            return column;

Nice to see this all factored out into one place.

::: js/src/jsapi.h
@@ +4002,5 @@
> +    // decompression. That is, for a ScriptSource buffer |src|, we delazify
> +    // LazyScripts with a buffer |src + lazy->begin()|. We record
> +    // |lazy->begin()| here to compute absolute offsets into the root
> +    // source buffer.
> +    unsigned rootSourceBufferOffset;

This is a wonderful clarification of the meaning of this field.

I think calling this the "root source buffer" is misleading, since SourceObjects don't form trees. Suggest 'scriptSourceOffset', with corresponding changes to setters, related fields, etc.

Suggest comment:

// The offset within the ScriptSource's full uncompressed text of the first
// character we're presenting for compilation with this CompileOptions.
//
// When we compile a LazyScript, we pass the compiler only the substring of the
// source the lazy function occupies. With chunked decompression, we may not
// even have the complete uncompresed source present in memory. But parse node
// positions are offsets within the ScriptSource's full text, and LazyScripts
// indicate their substring of the full source by its starting and ending
// offsets within the full text. This scriptSourceOffset field lets the front
// end convert between these offsets and offsets within the substring presented
// for compilation.
Attachment #8872855 - Flags: review?(jimb) → review+
ni? shu for jim's questions (and landing)
Flags: needinfo?(shu)
(Thanks, Mike!)
(In reply to Jim Blandy :jimb from comment #8)
> Comment on attachment 8872855 [details] [diff] [review]
> Rework column handling in frontend by separating column from offset from
> root ScriptSource buffer.
> 
> Review of attachment 8872855 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Okay, I think I've got my head around the change this is making. If some of
> the comments sound clueless, please point that out to me.
> 
> Please include the reduced test case in the prior comment in the revised
> patch.
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ +10703,5 @@
> >          // correct toString output.
> >          //
> >          // Token positions are already offset from the start column. Since
> >          // toString offsets are absolute offsets into the ScriptSource,
> >          // de-offset from the starting column.
> 
> Does this comment need to come out?
> 

Yes, good catch. Will remove.

> @@ +10705,5 @@
> >          // Token positions are already offset from the start column. Since
> >          // toString offsets are absolute offsets into the ScriptSource,
> >          // de-offset from the starting column.
> > +        ptrdiff_t classStart = ptrdiff_t(pn->pn_pos.begin);
> > +        ptrdiff_t classEnd = ptrdiff_t(pn->pn_pos.end);
> 
> So great.
> 
> ::: js/src/frontend/Parser.cpp
> @@ +3016,5 @@
> >          // are parenFreeArrow, we will set this below, after consuming the NAME.
> >          funbox->setStart(tokenStream);
> > +    } else {
> > +        // When delazifying, we may not have a current token and pos() is
> > +        // garbage. In that case, substitute the first token's position.
> 
> This is a little worrisome, because it suggests that ParserBase can be in a
> state where calling `pos` returns garbage. If this spot change is necessary,
> it makes me worry there could be others we don't know about. Is there any
> way to assert in pos that it won't return garbage?
> 
> (I recognize this is probably a pre-existing condition.)
> 

Hm, I don't know to do so easily right now without some more judicious rewriting of TokenStream. The current pos is the pos of the current token, and the tokens are kept in a circular C array. To check that there is a valid first token is more trouble than it's worth for this patch imo.

> @@ +3248,5 @@
> >          funbox->setIsExprBody();
> >  
> >      PropagateTransitiveParseFlags(lazy, pc->sc());
> >  
> > +    if (!tokenStream.advance(fun->lazyScript()->end()))
> 
> So great.
> 
> ::: js/src/frontend/SharedContext.h
> @@ -577,5 @@
> >  
> >      void setEnd(const TokenStream& tokenStream) {
> > -        // For all functions except class constructors, the buffer and
> > -        // toString ending positions are the same. Class constructors override
> > -        // the toString ending position with the end of the class definition.
> 
> Does this comment really no longer apply? It seems like you haven't changed
> the meanings of these positions.

Oops, you're right. Will add the comment back.

> 
> ::: js/src/frontend/TokenStream.cpp
> @@ +260,5 @@
> >      // which fixes the GCC/clang error, but causes bustage on Windows.  Sigh.
> >      //
> >      uint32_t maxPtr = MAX_PTR;
> >  
> > +    // The first line begins at buffer offset |initialLineOffset|.  MAX_PTR is
> 
> There is a comment in TokenStream.h still claiming it's always zero, which
> should be changed as well.
> 
> Would it also make sense to have SourceCoords::fill assert that other and
> this have the same value for lineStartOffsets_[0]?

Good catches, will fix comment and assert.

> 
> ::: js/src/frontend/TokenStream.h
> @@ +510,5 @@
> >          uint32_t lineIndexToNum(uint32_t lineIndex) const { return lineIndex + initialLineNum_; }
> >          uint32_t lineNumToIndex(uint32_t lineNum)   const { return lineNum   - initialLineNum_; }
> > +        uint32_t lineIndexAndOffsetToColumn(uint32_t lineIndex, uint32_t offset) const {
> > +            uint32_t lineStartOffset = lineStartOffsets_[lineIndex];
> > +            MOZ_ASSERT(offset >= lineStartOffset);
> 
> When we could assume the first line offset was zero, then we knew that
> lineIndexOf would always return a line index whose offset was <= the given
> offset, no matter what wild offset we passed in. But now that we can specify
> other first line offsets, passing in offsets before lineStartOffsets_[0] in
> non-DEBUG builds will cause this function to return negative (or rather,
> gigantic positive) values in response to wild offsets. For that reason, I'd
> recommend making this a release assert.

Sure, made a release assert.

> 
> Passing such offsets to lineIndexOf should just return zero, so there's no
> risk there.
> 
> @@ +514,5 @@
> > +            MOZ_ASSERT(offset >= lineStartOffset);
> > +            uint32_t column = offset - lineStartOffset;
> > +            if (lineIndex == 0)
> > +                return column + initialColumn_;
> > +            return column;
> 
> Nice to see this all factored out into one place.
> 
> ::: js/src/jsapi.h
> @@ +4002,5 @@
> > +    // decompression. That is, for a ScriptSource buffer |src|, we delazify
> > +    // LazyScripts with a buffer |src + lazy->begin()|. We record
> > +    // |lazy->begin()| here to compute absolute offsets into the root
> > +    // source buffer.
> > +    unsigned rootSourceBufferOffset;
> 
> This is a wonderful clarification of the meaning of this field.
> 
> I think calling this the "root source buffer" is misleading, since
> SourceObjects don't form trees. Suggest 'scriptSourceOffset', with
> corresponding changes to setters, related fields, etc.
> 
> Suggest comment:
> 
> // The offset within the ScriptSource's full uncompressed text of the first
> // character we're presenting for compilation with this CompileOptions.
> //
> // When we compile a LazyScript, we pass the compiler only the substring of
> the
> // source the lazy function occupies. With chunked decompression, we may not
> // even have the complete uncompresed source present in memory. But parse
> node
> // positions are offsets within the ScriptSource's full text, and LazyScripts
> // indicate their substring of the full source by its starting and ending
> // offsets within the full text. This scriptSourceOffset field lets the front
> // end convert between these offsets and offsets within the substring
> presented
> // for compilation.

Will take your comment.
Flags: needinfo?(shu)
Pushed by shu@rfrn.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19a82f6ac49e
Rework column handling in frontend by separating column from offset from root ScriptSource buffer. (r=jimb)
https://hg.mozilla.org/mozilla-central/rev/19a82f6ac49e
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
I assume we want to let this ride the trains.
That seems reasonable to me.
Duplicate of this bug: 1370887
Duplicate of this bug: 1370882
Duplicate of this bug: 1370886
You need to log in before you can comment on or make changes to this bug.