Bug 12720 - Re-defining window.location.toString function keeps re-loading forever
Summary: Re-defining window.location.toString function keeps re-loading forever
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P2 Normal
Assignee: Feng Qian
URL:
Keywords: HasReduction
Depends on:
Blocks:
 
Reported: 2007-02-09 15:37 PST by Feng Qian
Modified: 2007-03-07 12:47 PST (History)
1 user (show)

See Also:


Attachments
Test case (83 bytes, text/html)
2007-02-09 15:58 PST, David Kilzer (:ddkilzer)
no flags Details
patch (1.53 KB, patch)
2007-02-27 18:41 PST, Feng Qian
no flags Details | Formatted Diff | Diff
a better test with expected output (1.86 KB, patch)
2007-02-27 18:45 PST, Feng Qian
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Feng Qian 2007-02-09 15:37:45 PST
Create a page with a statement:

window.location.toString = function() { return "www.cnn.com"; }

Open it in a new window, it keeps re-loading the page.
Comment 1 David Kilzer (:ddkilzer) 2007-02-09 15:57:34 PST
Behavior confirmed in shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.8 (8N1037) and a local debug build of WebKit r19537 with the same software.

Both Firefox 2.0.0.1 and Opera 9.10 don't appear to log any errors when the test case is loaded--they just silently refuse to change the built-in function.

Comment 2 David Kilzer (:ddkilzer) 2007-02-09 15:58:14 PST
Created attachment 13098 [details]
Test case
Comment 3 Feng Qian 2007-02-27 18:41:01 PST
Created attachment 13417 [details]
patch

The bug is caused by Location::put if the property name is a function name in the LocationTable. It does not change URL, but put() will think the URL was changed and reload the file again, and into an infinite loop. The fix just returns early when it founds the property name is in the LocationTable, but not changing URL.
Comment 4 Feng Qian 2007-02-27 18:45:16 PST
Created attachment 13418 [details]
a better test with expected output
Comment 5 Maciej Stachowiak 2007-02-27 18:50:23 PST
This looks ok, but it might be better to mark the functions that should not be replaceable as read-only.
Comment 6 Maciej Stachowiak 2007-02-27 22:53:35 PST
Comment on attachment 13418 [details]
a better test with expected output

After further consideration this fix seems ok.
Comment 7 Feng Qian 2007-03-01 12:04:51 PST
(In reply to comment #5)
> This looks ok, but it might be better to mark the functions that should not be
> replaceable as read-only.
> 

How does ReadOnly attribute work? Will the call be returned before calling Location::put ?
Comment 8 Mark Rowe (bdash) 2007-03-06 19:30:41 PST
Landed in r19995.

Thanks for the patch Ian.  Can you please ensure that you use spaces rather than tabs in your ChangeLog entries, and that they are consistent in formatting with other entries.  I'm specifically thinking of new lines between description and file list, and the presence of the "Reviewed by NOBODY" line.  The latter will ensure that the patch cannot be landed unless the reviewer is added.
Comment 9 Mark Rowe (bdash) 2007-03-06 19:44:32 PST
I also just noticed that you forgot to include a changelog for the LayoutTests portion of your patch.
Comment 10 Mark Rowe (bdash) 2007-03-06 20:30:07 PST
Not to keep going on... but the expected results you provided didn't match the test.  I updated the test in r20002 to make things pass.
Comment 11 Feng Qian 2007-03-07 12:47:44 PST
(In reply to comment #8)
> Landed in r19995.
> 
> Thanks for the patch Ian.  Can you please ensure that you use spaces rather
> than tabs in your ChangeLog entries, and that they are consistent in formatting
> with other entries.  I'm specifically thinking of new lines between description
> and file list, and the presence of the "Reviewed by NOBODY" line.  The latter
> will ensure that the patch cannot be landed unless the reviewer is added.

Thanks Mike. I will try my best to remember these style rules.

>