Bug 80265 – The path of URL has redundant slashes should not be cached again in offline web applications

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[huangxueqing]

Created attachment 130093 [details]
test case

INTRODUCTION:
It's commom that user input more than one slashes in path of url due to careless, such as "http://127.0.0.1:8000//appcache//test.html" actually specify "http://127.0.0.1:8000/appcache/test.html".

REPOS STEPS:
1. Unzip attachment and move "appcache" folder into %APACHE_ROOT%;
2. Navigate http://127.0.0.1/appcache/test.html;
2. Naviagte http://127.0.0.1/////appcache/////test.html;

ACTUAL RESULTS:
There ware two record in cacheGroup table identified by "http://127.0.0.1/appcache/test.manifest" and "http://127.0.0.1/////appcache/////test.manifest" respectively.

EXPECTED RESULTS:
Only one record in cacheGroup identified by "http://127.0.0.1/appcache/test.manifest".



REPOS STEPS:
1. Navigate http://127.0.0.1/appcache/test.html;
2. Shutdown apache;
3. Navigate http://127.0.0.1/////appcache/test.html;

ACTUAL RESULTS:
Failed to load test.html from local cache.

EXPECTED RESULTS:
Load test.html from local cache correctly.

[WebKit Review Bot]

Attachment 130093 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[huangxueqing]

Created attachment 130094 [details]
test case

[huangxueqing]

Created attachment 130099 [details]
patch

[WebKit Review Bot]

Comment on attachment 130099 [details]
patch

Attachment 130099 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11803984

New failing tests:
http/tests/appcache/fallback-multiply-slashes.html
http/tests/appcache/appcache-multiply-slashes.html

[huangxueqing]

Created attachment 130112 [details]
skip layout test cases in chromium platform

[Build Bot]

Comment on attachment 130112 [details]
skip layout test cases in chromium platform

Attachment 130112 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11808564

[Build Bot]

Comment on attachment 130099 [details]
patch

Attachment 130099 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11809382

[huangxueqing]

Created attachment 130123 [details]
patch

[Build Bot]

Comment on attachment 130123 [details]
patch

Attachment 130123 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11809409

[huangxueqing]

Created attachment 130131 [details]
patch

[huangxueqing]

Created attachment 130134 [details]
patch

[Adam Barth]

Comment on attachment 130134 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130134&action=review

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:59
> +String stripRedundantSlashes(const String& str)

I'm sorry that I will not be able to review your patch fully, but this function clearly does not belong in this file.

[Alexey Proskuryakov]

I don't think that there is a concept of normalizing a URL by removing duplicate slashes, is there?

Thus, this looks like correct behavior to me.

[Adam Barth]

> I don't think that there is a concept of normalizing a URL by removing duplicate slashes, is there?

Correct.  I'm not away of any precedent for normalizing URIs by removing redundant slashes from the path.  There is some precedent for removing redundant slashes between the scheme and the authority, but even that is controversial (e.g., in the IETF's IRI working group).

[Alexey Proskuryakov]

Please feel free to reopen if there is evidence that this is a bug.

[huangxueqing]

Thanks for review. It's not well to put this function into KURL.h since KURL never call it. Any suggestions?  

(In reply to comment #12)
> (From update of attachment 130134 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130134&action=review
> 
> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:59
> > +String stripRedundantSlashes(const String& str)
> 
> I'm sorry that I will not be able to review your patch fully, but this function clearly does not belong in this file.

[huangxueqing]

No, but in my view, "appcache" as a "nightly web server" in local since it provide  resource just like a web server, and web server always handle redundant slashes in path correctly such as apache, thus "appcache" should do that also.
In addition, it's unnecessary that store a manifest twice.

(In reply to comment #13)
> I don't think that there is a concept of normalizing a URL by removing duplicate slashes, is there?
> 
> Thus, this looks like correct behavior to me.

[Alexey Proskuryakov]

> No, but in my view, "appcache" as a "nightly web server"

It's perfectly appropriate for a Web server to provide different responses to requests with different number of slashes.

[huangxueqing]

(In reply to comment #18)
> > No, but in my view, "appcache" as a "nightly web server"
> 
> It's perfectly appropriate for a Web server to provide different responses to requests with different number of slashes.

All right, thanks for review. Maybe I will investigate how web server handle it.