Bug 19844 - JavaScript Switch statement modifies "this"
Summary: JavaScript Switch statement modifies "this"
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Critical
Assignee: Cameron Zwarich (cpst)
Keywords: InRadar, Regression
Depends on:
Reported: 2008-07-01 07:23 PDT by Fabian Jakobs
Modified: 2008-07-02 01:19 PDT (History)
1 user (show)

See Also:

Test case for this bug (405 bytes, text/html)
2008-07-01 07:25 PDT, Fabian Jakobs
no flags Details
Codegen dump (1.18 KB, text/plain)
2008-07-01 16:19 PDT, Cameron Zwarich (cpst)
no flags Details
Proposed patch (without tests) (2.34 KB, patch)
2008-07-01 16:28 PDT, Cameron Zwarich (cpst)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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"

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

function switchFunction(val)
  switch (val)
    case this:

  return this;


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
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.