[webkit-reviews] review granted: [Bug 26196] Fix the problem that worker's importScripts fails if the script URL is redirected from different origin. : [Attachment 30957] Proposed Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 8 18:03:19 PDT 2009


David Levin <levin at chromium.org> has granted Jian Li <jianli at chromium.org>'s
request for review:
Bug 26196: Fix the problem that worker's importScripts fails if the script URL
is redirected from different origin.
https://bugs.webkit.org/show_bug.cgi?id=26196

Attachment 30957: Proposed Patch
https://bugs.webkit.org/attachment.cgi?id=30957&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Just a few things to fix up on landing.

> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 40d853a..6b451d0 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,13 @@
> +2009-06-04  Jian Li	<jianli at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +

I think the current style is to add the bug title here.
> +	   https://bugs.webkit.org/show_bug.cgi?id=26196
> +	   Add a test case in the importScripts layout test to cover the
scenario that the import script URL can come from different redirect origin.

Interestingly, the changelog text is typically wrapped.

Perhaps 
  "can come from different origin through a redirect."
would be slightly clearer.

> +
> +	   * http/tests/workers/resources/worker-importScripts.js:
> +	   * http/tests/workers/worker-importScripts-expected.txt:
> +
>  2009-06-02  Alexey Proskuryakov  <ap at webkit.org>
>  
>	   Land correct results for a test I just added (forgot to regenerate
them after adding new
> diff --git a/LayoutTests/http/tests/workers/resources/worker-importScripts.js
b/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> index ddf479c..364c502 100644
> --- a/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> +++ b/LayoutTests/http/tests/workers/resources/worker-importScripts.js
> @@ -7,6 +7,7 @@ postMessage("PASS: importScripts(), exists, is a function,
and doesn't throw whe
>  var source1 = "worker-importScripts-source1.js";
>  var source2 = "worker-importScripts-source2.js";
>  var differentOrigin =
"http://localhost:8000/workers/resources/worker-importScripts-differentOrigin.j
s";
> +var differentRedirectOrigin =
"/resources/redirect.php?url=http://localhost:8000/workers/resources/worker-imp
ortScripts-differentOrigin.js";
>  var syntaxErrorSource = "worker-importScripts-syntaxError.js";
>  var fakeSource = "nonexistant";
>  var loadedSource1 = false;
> @@ -29,6 +30,16 @@ if (differentOriginLoaded)
>  
>  resetLoadFlags();
>  
> +try {
> +    importScripts(differentRedirectOrigin)
> +} catch(e) {
> +    postMessage("FAIL: Threw " + e + " when attempting cross origin load on
redirection");
Add "."

> +}
> +if (differentOriginLoaded)
> +    postMessage("PASS: executed script from different redirect origin");
I think this could be re-worded slight (similar to the ChangeLog and add a
".").

Add an else to print a FAIL message so that one can tell that it failed just by
running it in a browser.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index b119fa6..bf8ebc3 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,34 @@

Same fix up in this changelog as suggested above.

>  
> -DocumentThreadableLoader::DocumentThreadableLoader(Document* document,
ThreadableLoaderClient* client, const ResourceRequest& request, LoadCallbacks
callbacksSetting, ContentSniff contentSniff, StoredCredentials
storedCredentials)
> +DocumentThreadableLoader::DocumentThreadableLoader(Document* document,
ThreadableLoaderClient* client, const ResourceRequest& request, LoadCallbacks
callbacksSetting, ContentSniff contentSniff, StoredCredentials
storedCredentials, RedirectOriginCheck redirectOriginCheck)
>      : m_client(client)
>      , m_document(document)
>      , m_allowStoredCredentials(storedCredentials == AllowStoredCredentials)
>      ,
m_sameOriginRequest(document->securityOrigin()->canRequest(request.url()))
> +    , m_checkRedirectOrigin(redirectOriginCheck ==
RequireSameRedirectOrigin)
>  {

It would be nice to assert that redirectOriginCheck has one of the two allowed
values (and feel free to add it for storedCredentials which I should have
done).


More information about the webkit-reviews mailing list