Bug 19844

Summary:

JavaScript Switch statement modifies "this"

Product:

WebKit

Reporter:

Fabian Jakobs <fabian.jakobs>

Component:

JavaScriptCore

Assignee:

Cameron Zwarich (cpst) <zwarich>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

zwarich

Priority:

Keywords:

InRadar, Regression

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Test case for this bug	none
Codegen dump	none
Proposed patch (without tests)	darin: review+

Description Fabian Jakobs 2008-07-01 07:23:29 PDT

I have encountered a strange error with the latest nightlys (rev. 34824).

Special switch statements seem to modifiy the function context (aka this) and set if to the value "false". This can be observed in this simple example.


<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN"
   "http://www.w3.org/TR/html4/strict.dtd">

<html lang="en">
<body>
<script type="text/javascript" charset="utf-8">
    
function test() {
    alert(this == switchFunction.call(this, 12));
}

function switchFunction(val)
{
  switch (val)
  {
    case this:
      break;

    default:
      break;
  }
  return this;
}
test();

</script>

</body>
</html>

Comment 1 Fabian Jakobs 2008-07-01 07:25:14 PDT

Created attachment 22028 [details]
Test case for this bug

Example which shows this problem. Other browser including Safari 3.1.1 return "true" while the current nightly returns "false"

Comment 2 Mark Rowe (bdash) 2008-07-01 15:44:51 PDT

<rdar://problem/6048093>

Comment 3 Cameron Zwarich (cpst) 2008-07-01 16:19:32 PDT

Created attachment 22033 [details]
Codegen dump

The problem is that it is writing the result of stricteq to lr2, which is 'this'. I will fix this after dinner.

Comment 4 Cameron Zwarich (cpst) 2008-07-01 16:28:48 PDT

Created attachment 22034 [details]
Proposed patch (without tests)

Here's a patch. I will add tests to fast/js/codegen-temporaries for both the 'this' and local variable cases.

Comment 5 Darin Adler 2008-07-01 16:32:15 PDT

Comment on attachment 22034 [details]
Proposed patch (without tests)

Patch looks good. r=me

But why not include the tests with the patch next time?

Comment 6 Cameron Zwarich (cpst) 2008-07-01 16:32:46 PDT

(In reply to comment #5)
> (From update of attachment 22034 [details] [edit])
> Patch looks good. r=me
> 
> But why not include the tests with the patch next time?

Going out for a Canada Day dinner and didn't have the time.

Comment 7 Cameron Zwarich (cpst) 2008-07-01 18:09:11 PDT

Landed in r34940.

Comment 8 Fabian Jakobs 2008-07-02 01:19:50 PDT

Awesome. This was really quick!

Works for me.