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)
Core
DOM: Workers
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: valentin, Assigned: baku)
Details
Attachments
(1 file)
6.03 KB,
patch
|
valentin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8946231 -
Flags: review?(valentin.gosu)
Reporter | ||
Comment 2•6 years ago
|
||
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
Comment 4•6 years ago
|
||
bugherder |
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.
Description
•