Closed Bug 469758 Opened 11 years ago Closed 8 years ago

Incorrect error message with adjacent "let" blocks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: jruderman, Assigned: h4writer)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(2 files, 2 obsolete files)

let (UNUSED = 0) { } let (b = 1) { [][b][2]; }

Result:
  TypeError: [][UNUSED] is undefined
Expected:
  TypeError: [][b] is undefined
Duplicate of this bug: 522858
Duplicate of this bug: 528205
Duplicate of this bug: 622389
Blocks: jserror
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.
Attached patch Patch (obsolete) — Splinter Review
(In reply to Hannes Verschore from comment #4)
> ... everything I'm afraid to brake something...
*break
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 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.
Attachment #576868 - Flags: review?(dmandelin)
Comment on attachment 576868 [details] [diff] [review]
Reworked Hannes patch to remove goto

The fix looks OK. Could you please add a test case?
Attachment #576868 - Flags: review?(dmandelin)
Attachment #576830 - Attachment is obsolete: true
Attachment #576868 - Attachment is obsolete: true
Attachment #577538 - Flags: review?(dmandelin)
Just noticed I forgot to add the testcase to the jstest.list file.
Attachment #577608 - Flags: review?(dmandelin)
Comment on attachment 577538 [details] [diff] [review]
Andrew's reworked patch of mine + added testcase

Thanks!
Attachment #577538 - Flags: review?(dmandelin) → review+
Attachment #577608 - Flags: review?(dmandelin) → review+
Keywords: checkin-needed
Assignee: general → hv1989
http://hg.mozilla.org/integration/mozilla-inbound/rev/b4d21a257883
Flags: in-testsuite+
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/b4d21a257883
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.