Bug 10323 – REGRESSION: javascript: URL containing '\\' gets passed as '//'

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Sander Rijken]

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)

[Sander Rijken]

Created attachment 9957 [details]
Reproduction page

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 9962 [details]
Test case v2

[Darin Adler]

This is specific to javascript: URLs, and is an issue in URL handling, not the JavaScript interpreter.

[Vladimir Olexa (vladinecko)]

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.

[David Kilzer (:ddkilzer)]

(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?

[Darin Adler]

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.

[Vladimir Olexa (vladinecko)]

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.

[Darin Adler]

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.

[Vladimir Olexa (vladinecko)]

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.

[Darin Adler]

Comment on attachment 10493 [details]
Patch - Revised Comment

r=me

[Darin Adler]

Committed revision 16300.