Bug 10323 - REGRESSION: javascript: URL containing '\\' gets passed as '//'
Summary: REGRESSION: javascript: URL containing '\\' gets passed as '//'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2006-08-09 08:29 PDT by Sander Rijken
Modified: 2006-09-11 10:31 PDT (History)
5 users (show)

See Also:


Attachments
Reproduction page (1.02 KB, text/html)
2006-08-09 08:30 PDT, Sander Rijken
no flags Details
Test case v2 (508 bytes, text/html)
2006-08-09 13:29 PDT, David Kilzer (:ddkilzer)
no flags Details
Patch - First attempt (4.97 KB, patch)
2006-09-06 21:25 PDT, Vladimir Olexa (vladinecko)
darin: review-
Details | Formatted Diff | Diff
Patch - Revised (5.36 KB, patch)
2006-09-09 12:08 PDT, Vladimir Olexa (vladinecko)
darin: review-
Details | Formatted Diff | Diff
Patch - Revised Comment (5.45 KB, patch)
2006-09-10 15:43 PDT, Vladimir Olexa (vladinecko)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sander Rijken 2006-08-09 08:29:44 PDT
A function called with an argument where the string contains a single \ e.g. 'myvalue\\something' is interpreted by the function as 'myvalue//something'

in safari (419.3) this works the way it should be, in 420 versions this appears to be broken (latest version I tried was r15815)
Comment 1 Sander Rijken 2006-08-09 08:30:28 PDT
Created attachment 9957 [details]
Reproduction page
Comment 2 David Kilzer (:ddkilzer) 2006-08-09 13:28:15 PDT
Confirmed on locally-built debug build of WebKit r15822.

Regression from production Safari 2.0.4 (419.3) on Mac OS X 10.4.7 (8J135/PowerPC).
Comment 3 David Kilzer (:ddkilzer) 2006-08-09 13:29:24 PDT
Created attachment 9962 [details]
Test case v2
Comment 4 Darin Adler 2006-08-12 17:57:57 PDT
This is specific to javascript: URLs, and is an issue in URL handling, not the JavaScript interpreter.
Comment 5 Vladimir Olexa (vladinecko) 2006-09-06 21:25:35 PDT
Created attachment 10429 [details]
Patch - First attempt

Proposed patch limiting backslash conversions to only non-javascript schemes. Changelog says no new test cases were added, but they are included in this patch.
Comment 6 David Kilzer (:ddkilzer) 2006-09-07 05:02:44 PDT
(In reply to comment #5)
> Proposed patch limiting backslash conversions to only non-javascript schemes.
> Changelog says no new test cases were added, but they are included in this
> patch. 

How did you run prepare-ChangeLog?  What directory were you in, and what arguments did you pass to the script?
Comment 7 Darin Adler 2006-09-07 07:48:53 PDT
Comment on attachment 10429 [details]
Patch - First attempt

Thanks for tackling this!

The patch has an object lifetime problem. The result of substituteBackslashes, with this code change, will only live until the end of the expression that initializes rel, leaving rel referring to a deleted object. There's a reason that the subsitutedRelative variable was declared outside the if statement in the old code, and this new code is broken.

I am surprised to see an explicit check for the javascript scheme here. I thought we were going to do this based on hierarchical vs. non-hierarchical URLs. And I'd really like the backslash hack to be an exception rather than the rule -- it would be better to have limited cases where it *does* happen rather than limited cases where it does not.

That having been said, I think this approach is OK, but not great.

There's also a missing space before "false" in the function call.

review- just for the object lifetime problem; otherwise this is fine.
Comment 8 Vladimir Olexa (vladinecko) 2006-09-09 12:08:25 PDT
Created attachment 10478 [details]
Patch - Revised

As discussed on IRC, this a quick-n-dirty patch for this bug that does its job. Also fixed is the memory issue from the previous version of the patch. Later, a more elegant solution checking for hierarchical URLs will be implemented.
Comment 9 Darin Adler 2006-09-10 12:40:56 PDT
Comment on attachment 10478 [details]
Patch - Revised

The code change is good, but I think this comment change is a bad idea:

-    // for compatibility with Win IE, we must treat backslashes as if they were slashes
+    // temporary fix for proper backslash substitution

You've replaced a comment that mentions the Win IE compatibility issue with one that is considerably more vague. The comment says this is "temporary" without explaining what's temporary about it or when the temporary time would be over. The comment says "proper backslash substitution" but there's no explanation for why backslashes need to be substituted at all or what would be proper vs. improper.

Patch is great otherwise, but review- for the comment.
Comment 10 Vladimir Olexa (vladinecko) 2006-09-10 15:43:20 PDT
Created attachment 10493 [details]
Patch - Revised Comment

Yes, makes sense! 

I changed the comment slightly from the original one only pointing out backslashes are not to be replaced by slashes when the javascript: schema is used.
Comment 11 Darin Adler 2006-09-10 19:42:19 PDT
Comment on attachment 10493 [details]
Patch - Revised Comment

r=me
Comment 12 Darin Adler 2006-09-11 10:31:31 PDT
Committed revision 16300.