Last Comment Bug 469758 - Incorrect error message with adjacent "let" blocks
: Incorrect error message with adjacent "let" blocks
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Hannes Verschore [:h4writer]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 522858 528205 622389 (view as bug list)
Depends on:
Blocks: jserror
  Show dependency treegraph
 
Reported: 2008-12-15 16:55 PST by Jesse Ruderman
Modified: 2012-02-01 13:58 PST (History)
7 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.48 KB, patch)
2011-11-24 14:49 PST, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Reworked Hannes patch to remove goto (4.28 KB, patch)
2011-11-24 21:03 PST, Andrew Paprocki
no flags Details | Diff | Splinter Review
Andrew's reworked patch of mine + added testcase (4.80 KB, patch)
2011-11-29 03:01 PST, Hannes Verschore [:h4writer]
dmandelin: review+
Details | Diff | Splinter Review
Add testcase to jstests.list (585 bytes, patch)
2011-11-29 07:25 PST, Hannes Verschore [:h4writer]
dmandelin: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2008-12-15 16:55:30 PST
let (UNUSED = 0) { } let (b = 1) { [][b][2]; }

Result:
  TypeError: [][UNUSED] is undefined
Expected:
  TypeError: [][b] is undefined
Comment 1 Andrew Paprocki 2011-11-08 08:26:52 PST
*** Bug 522858 has been marked as a duplicate of this bug. ***
Comment 2 Andrew Paprocki 2011-11-08 08:27:26 PST
*** Bug 528205 has been marked as a duplicate of this bug. ***
Comment 3 Andrew Paprocki 2011-11-08 08:27:58 PST
*** Bug 622389 has been marked as a duplicate of this bug. ***
Comment 4 Hannes Verschore [:h4writer] 2011-11-24 14:48:43 PST
I found the culprit in the way how the function "GetLocal" in jsopcode.cpp works.
If the function is called from "js_DecompileValueGenerator" it (can) have a negative offset.
Then a search through "script->objects" takes place to find the name of the variable.
Now there is the fault, the code assumes the first object that goes through all tests is the right one. Here in this case we find the first object (containing "UNUSED"), but should find the second one (containing "b").

I adjusted the code. Instead of looping all objects to find the right object. I look at the JSOP_ENTERBLOCK definition (if available) to obtain the index to the right object. Upon fail it will run the old code (and do a loop).

The patch is a bit dirty. I used a goto and there is some duplicate code. But because I don't understand everything I'm afraid to brake something. So the code mimics the old code as good as possible.

I ran the jstests.py testset. I'm not sure if it would detect faults in error reporting?
I couldn't run it totally, because it kept running after xx minutes. So I aborted.
with patch: [3153|   9| 164]
Only timeouts or Date related regressions.
Comment 5 Hannes Verschore [:h4writer] 2011-11-24 14:49:48 PST
Created attachment 576830 [details] [diff] [review]
Patch
Comment 6 Hannes Verschore [:h4writer] 2011-11-24 14:52:22 PST
(In reply to Hannes Verschore from comment #4)
> ... everything I'm afraid to brake something...
*break
Comment 7 Andrew Paprocki 2011-11-24 21:03:20 PST
Created attachment 576868 [details] [diff] [review]
Reworked Hannes patch to remove goto

I applied the logic in my tree and reworked the patch to remove the goto:. I confirm the patch does fix the issue. Someone else needs to vet the logic to make sure it is sane (any additional JS_ASSERT()s needed?).
Comment 8 Andrew Paprocki 2011-11-24 21:06:38 PST
Comment on attachment 576868 [details] [diff] [review]
Reworked Hannes patch to remove goto

Asking dmandelin for review or to reassign to someone who could vet this logic.
Comment 9 David Mandelin [:dmandelin] 2011-11-28 11:50:04 PST
Comment on attachment 576868 [details] [diff] [review]
Reworked Hannes patch to remove goto

The fix looks OK. Could you please add a test case?
Comment 10 Hannes Verschore [:h4writer] 2011-11-29 03:01:34 PST
Created attachment 577538 [details] [diff] [review]
Andrew's reworked patch of mine + added testcase
Comment 11 Hannes Verschore [:h4writer] 2011-11-29 07:25:14 PST
Created attachment 577608 [details] [diff] [review]
Add testcase to jstests.list

Just noticed I forgot to add the testcase to the jstest.list file.
Comment 12 David Mandelin [:dmandelin] 2011-11-29 11:09:00 PST
Comment on attachment 577538 [details] [diff] [review]
Andrew's reworked patch of mine + added testcase

Thanks!
Comment 14 Ed Morley [:emorley] 2011-12-04 07:20:08 PST
https://hg.mozilla.org/mozilla-central/rev/b4d21a257883

Note You need to log in before you can comment on or make changes to this bug.