Closed Bug 1433723 Opened 6 years ago Closed 6 years ago

URLWorker::SetProtocol should always proxy to the main thread

Categories

(Core :: DOM: Workers, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: valentin, Assigned: baku)

Details

Attachments

(1 file)

Right now, if the scheme is "http" or "https", URLWorker::SetProtocol will set the protocol directly on mStdURL, which is incorrect, as the type of the URI object depends on having the appropriate scheme.
There is a comment about it in URLMainThread::SetProtocol.

I propose the following: if the scheme is already "http/s" and target is also "http/s" then use mStdURL. Otherwise proxy to the main thread.
Attached patch url.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8946231 - Flags: review?(valentin.gosu)
Comment on attachment 8946231 [details] [diff] [review]
url.patch

Review of attachment 8946231 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this!
r+ with a few nits and questions

::: dom/url/URLWorker.cpp
@@ +837,5 @@
>      if (NS_WARN_IF(NS_FAILED(rv))) {
>        return;
>      }
>  
> +    SetHrefInternal(NS_ConvertUTF8toUTF16(href), eUseProxyIfNeeded, aRv);

Is there a reason why we need to SetHref after calling SetScheme?
We definitely have mStdURL, and SetScheme should do the trick.

@@ +838,5 @@
>        return;
>      }
>  
> +    SetHrefInternal(NS_ConvertUTF8toUTF16(href), eUseProxyIfNeeded, aRv);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

this should check aRv, not rv.

@@ +867,5 @@
> +    MOZ_ASSERT(!mStdURL);
> +    MOZ_ASSERT(mURLProxy);
> +
> +    // Now we can restart setting the protocol.
> +    return SetProtocol(aProtocol, aRv);

Can't we just fall through to the next code?

::: dom/url/tests/mochitest.ini
@@ +21,5 @@
>  [test_worker_urlSearchParams.html]
>  [test_unknown_url_origin.html]
>  [test_bloburl_location.html]
> +[test_worker_protocol.html]
> +support-files = protocol_worker.js

Can't find this file in the tree/patch.

::: dom/url/tests/test_worker_protocol.html
@@ +19,5 @@
> +
> +  worker.onmessage = function(event) {
> +    is(event.target, worker, "Correct worker");
> +
> +    if (event.data.type == 'finish') {

nit: I think eslint requires we use double quotes for JS strings.
Attachment #8946231 - Flags: review?(valentin.gosu) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed4ebaacc8b7
URL in workers should use a proxy if the protocol is not http nor https, r=valentin
https://hg.mozilla.org/mozilla-central/rev/ed4ebaacc8b7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: