641563 - "Assertion failure: tp->begin.lineno == tp->end.lineno,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Function("(x)\nfor(var b,x in")

asserts js debug shell on TM changeset bb2bd286ef22 without -m nor -j at Assertion failure: tp->begin.lineno == tp->end.lineno

Top few lines of gdb stack:

#0  0x001c48fd in JS_Assert (s=0x32868c "tp->begin.lineno == tp->end.lineno", file=0x3285b8 "/Users/fuzz4/Desktop/jsfunfuzz-dbg-32-tm-63284-bb2bd286ef22/compilePath/jsscan.cpp", ln=471) at /Users/fuzz4/Desktop/jsfunfuzz-dbg-32-tm-63284-bb2bd286ef22/compilePath/jsutil.cpp:80
#1  0x00183c29 in js::TokenStream::reportCompileErrorNumberVA (this=0xbfffdc0c, pn=0x8adb40, flags=0, errorNumber=117, ap=0xbfffd8c0 "\f###\002") at /Users/fuzz4/Desktop/jsfunfuzz-dbg-32-tm-63284-bb2bd286ef22/compilePath/jsscan.cpp:471
#2  0x0015cb2a in js::Parser::reportErrorNumber (this=0xbfffdbe0, pn=0x8adb40, flags=0, errorNumber=117) at jsparse.h:1205
#3  0x0014a6b1 in js::Parser::forStatement (this=0xbfffdbe0) at /Users/fuzz4/Desktop/jsfunfuzz-dbg-32-tm-63284-bb2bd286ef22/compilePath/jsparse.cpp:5327
#4  0x0014b497 in js::Parser::statement (this=0xbfffdbe0) at /Users/fuzz4/Desktop/jsfunfuzz-dbg-32-tm-63284-bb2bd286ef22/compilePath/jsparse.cpp:6073

Comment 1

[Nicholas Nethercote [inactive]]

Same assertion as bug 640075, I bet it's something similar, ie. tp->end.lineno needs to be set for some construct in the erroneous case.  I suspect we'll see quite a few of these in the immediate future.

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   63253:3035bb782013
user:        Nicholas Nethercote
date:        Tue Mar 08 16:10:51 2011 -0800
summary:     Bug 638034 - Make scanning safer.  r=brendan.

Comment 3

[Nicholas Nethercote [inactive]]

Yep, that's the patch that tightened the checking, thus exposing the latent bug.

Comment 4

[Nicholas Nethercote [inactive]]

This doesn't need to block.  The worst that can happen is that a line number can be wrong, possibly leading to an incorrect error message from the JS engine.  Plus it's not really a regression, it's just been noticed now because stronger checking is in place.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Still asserts on trunk.

Comment 6

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

> This doesn't need to block.  The worst that can happen is that a line number
> can be wrong, possibly leading to an incorrect error message from the JS
> engine.  Plus it's not really a regression, it's just been noticed now
> because stronger checking is in place.

Thus, is this also a bogus assert?

Comment 7

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

(outline of chat, thanks Luke for helping out)

17:27   gkw: luke: do you happen to know if bug 641563 is a bogus assert?
17:28   luke: gkw: we don't do crazy dominator analysis on token positions, so i'd tend to think the bug was mostly harmless, but i don't think the assert is bogus

17:29   gkw: luke: oh. but if it's harmless = unlikely to be fixed soon :(
17:30   luke: gkw: yes

17:30   luke: gkw: it is a question about whether the assert should be removed if we don't care about what it is asserting
17:30   luke: gkw: but that is a question for njn

17:31   gkw: luke: i see. yeah, if the assert is to be ignored, then why have it there
17:31   gkw: luke: thanks though. i'll comment in the bug about this, and hopefully poke njn when he's back


njn, would it make sense to have a patch that removes this assert if we don't care about what it is asserting, even though it isn't bogus?

(i.e. what's the best way to move forward on this bug?)

Comment 8

[Nicholas Nethercote [inactive]]

The assertion is legitimate, and we have very little testing of our line number data, I'd like to keep it there if possible.  If it's causing major problems then disabling it would be ok.

Comment 9

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

(In reply to Nicholas Nethercote [:njn] from comment #8)
> The assertion is legitimate, and we have very little testing of our line
> number data, I'd like to keep it there if possible.  If it's causing major
> problems then disabling it would be ok.

It isn't major, it's on our ignore list but still appearing occasionally in our fuzzing output.

Comment 10

[Nicholas Nethercote [inactive]]

I've worked out what's going wrong, but I'm not sure how to fix it.

Here's a simplified version of the problematic JS code:

  x
  for(var b,x in

Within Parser::variables() we call NewBindingNode() for each of |b| and |x|.
And then we set the end position of the PNK_VAR node like this:

    pn->pn_pos.end = pn->last()->pn_pos.end;

In this case, pn->last() is the node for |x|.  The PNK_VAR starts on line 2,
but |pn->last()->pn_pos.end.lineno| is 1.  So we get the assertion.

Why is the line number 1?  Because |x| was seen earlier, when NewBindingNode() 
is called for it, we see an existing lexdep and return the ParseNode we created
on line 1 for |x|.

NewBindingNode's reuse of ParseNodes in this manner seems totally bogus w.r.t.
line/column positions.  More generally:  do we have a parse tree or a parse 
DAG?

Comment 11

[Luke Wagner [:luke]]

> NewBindingNode's reuse of ParseNodes in this manner seems totally bogus
> w.r.t. line/column positions.  More generally:  do we have a parse tree or a parse 
> DAG?

We have a parse tree where pn_lexdef is a cross/back-edge.  The lexdeps hash table exclusively holds "placeholder" definitions which have been created, so some use could point to them, but are not, conceptually, yet part of the tree.

It seems to me that the fix would for NewBindingNode to update the pn_pos of any node it pulls out of lexdeps.

Comment 12

[Christian Holler (:decoder)]

JSBugMon is unable to track this because the test isolation algorithm isn't capable of working with tests that are meant to produce SyntaxError like in comment 0.

Comment 13

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

Likely fixed by bug 568142.

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   103053:a8601aeb1d1d
user:        Alex Crichton
date:        Wed Aug 08 11:32:21 2012 -0700
summary:     Bug 568142 - Part 2: Have ParseNode's pn_pos enclose children better. r=jorendorff

Comment 14

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

Test landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f4f1f67bec7

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f4f1f67bec7

Comment 16

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

-> VERIFIED because test has been landed in the testsuite.