Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1202453002:
ServiceWorker: Implement navigate() method in WindowClient (chromium side). (Closed)
|
Created:
5 years, 6 months ago by zino Modified:
5 years, 4 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, mlamouri+watch-content_chromium.org, tzik, serviceworker-reviews, jam, darin-cc_chromium.org, horo+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, kinuko+serviceworker, kinuko+watch Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionServiceWorker: Implement navigate() method in WindowClient (chromium side).
Implement navigate() method in WindowClient (chromium side).
Spec:
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-navigate
Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/KETlHSSc878
Blink side:
https://codereview.chromium.org/1196193003/
Layout test:
https://codereview.chromium.org/1211253007/
BUG=500911
Committed: https://crrev.com/a564acb41ce9fe3243643546258fb1a04783d77b
Cr-Commit-Position: refs/heads/master@{#340635}
Patch Set 1 #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Total comments: 3
Patch Set 7 : nit #
Total comments: 10
Patch Set 8 : addressed comment #
Total comments: 4
Patch Set 9 : #
Messages
Total messages: 48 (12 generated)
jinho.bang@samsung.com changed reviewers: + mkwst@chromium.org, nhiroki@chromium.org
Dear reviewers, I uploaded the patch to implement the navigate() method in ServiceWorkerWindowClient. I'm not familiar with service worker part and this is my first patch about it. (But I'm interested in the part). So, please review the code and correct me. Thank you.
jinho.bang@samsung.com changed reviewers: + falken@chromium.org
Partially commented. I'll take another look later. https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1642: } We need to verify that provider_host's active worker is equal to this version as per the spec: "If the context object's associated service worker client's active worker is not the incumbent settings object's global object's service worker, return a promise rejected with a TypeError." FYI: I think this should be "provider_host's controller". Commented on the spec issue (https://github.com/slightlyoff/ServiceWorker/issues/681#issuecomment-114423686) https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1643: |url| passed from a renderer should be verified before using because a renderer could be compromised (See ServiceWorkerDispatcherHost::OnRegisterServiceWorker for how to verify. Maybe we should check "url.is_valid()" and its origin?) https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1681: } nit: you don't have to wrap a single line with {}
On 2015/06/23 at 10:14:37, nhiroki wrote: > Partially commented. I'll take another look later. > > https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... > File content/browser/service_worker/service_worker_version.cc (right): > > https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:1642: } > We need to verify that provider_host's active worker is equal to this version as per the spec: > > "If the context object's associated service worker client's active worker is not the incumbent settings object's global object's service worker, return a promise rejected with a TypeError." > > FYI: I think this should be "provider_host's controller". Commented on the spec issue (https://github.com/slightlyoff/ServiceWorker/issues/681#issuecomment-114423686) > > https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:1643: > |url| passed from a renderer should be verified before using because a renderer could be compromised (See ServiceWorkerDispatcherHost::OnRegisterServiceWorker for how to verify. Maybe we should check "url.is_valid()" and its origin?) > > https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... > content/browser/service_worker/service_worker_version.cc:1681: } > nit: you don't have to wrap a single line with {} I'll defer to nhiroki@, et al on the implementation. The message change looks reasonable, but I'll hold off on stamping it until after the implementation is clear.
Thank you for review. I'm sorry for delay. I'll upload a new patch set soon.
@nhiroki, Please take a look this as well. Thank you. https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1642: } Maybe done but I'm not sure. Please correct me. https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1643: On 2015/06/23 10:14:37, nhiroki wrote: > |url| passed from a renderer should be verified before using because a renderer > could be compromised (See ServiceWorkerDispatcherHost::OnRegisterServiceWorker > for how to verify. Maybe we should check "url.is_valid()" and its origin?) Done. https://codereview.chromium.org/1202453002/diff/1/content/browser/service_wor... content/browser/service_worker/service_worker_version.cc:1681: } On 2015/06/23 10:14:37, nhiroki wrote: > nit: you don't have to wrap a single line with {} Done.
https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); Please add a comment after |false| like this: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false /* is_renderer_initiated */); https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); Why is this false? This API call is issued by SW on renderer process, so I think this would be |true|. https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:1752: if (!client.IsEmpty()) Why didn't you copy the comment from OnOpenWindowFinished?
https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); This transition type could be PAGE_TRANSITION_AUTO_TOPLEVEL (I'm not really sure about the transition mechanism though)
I addressed your comments. Could you review this again? Thank you. https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); On 2015/07/10 06:15:35, nhiroki wrote: > This transition type could be PAGE_TRANSITION_AUTO_TOPLEVEL (I'm not really sure > about the transition mechanism though) Done. https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); On 2015/07/10 06:11:19, nhiroki wrote: > Please add a comment after |false| like this: > > CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false /* is_renderer_initiated */); > Done. https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:255: CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false); On 2015/07/10 06:11:19, nhiroki wrote: > Why is this false? This API call is issued by SW on renderer process, so I think > this would be |true|. Done. https://codereview.chromium.org/1202453002/diff/40001/content/browser/service... content/browser/service_worker/service_worker_version.cc:1752: if (!client.IsEmpty()) On 2015/07/10 06:11:19, nhiroki wrote: > Why didn't you copy the comment from OnOpenWindowFinished? Done.
REG: Patch Set 4, apparently you failed to upload service_worker_version.cc. Could you re-upload the entire patch set?
On 2015/07/14 01:44:08, nhiroki wrote: > REG: Patch Set 4, apparently you failed to upload service_worker_version.cc. > Could you re-upload the entire patch set? Yep.. I've just re-uploaded. Thank you!
+ kinuko@ as content's owner.
jinho.bang@samsung.com changed reviewers: + kinuko@chromium.org
+ kinuko@ as content's owner.
The CQ bit was checked by jinho.bang@samsung.com to run a CQ dry run
The CQ bit was unchecked by jinho.bang@samsung.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202453002/80001
kinuko@, nhiroki@, Could you review this CL? Thank you.
LGTM https://codereview.chromium.org/1202453002/diff/80001/content/browser/service... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/80001/content/browser/service... content/browser/service_worker/service_worker_version.cc:252: if (!render_frame_host || !web_contents) Could you run |callback| with invalid parameters so that the caller can detect this error?
On 2015/07/14 03:05:52, zino wrote:
> + kinuko@ as content's owner.
JFYI: You don't have to wait for kinuko@'s reviews because I'm an owner of
content/{browser,renderer}/service_worker/*. Of course, you can ask her to
review if you want :)
On 2015/07/15 05:44:49, nhiroki wrote:
> On 2015/07/14 03:05:52, zino wrote:
> > + kinuko@ as content's owner.
>
> JFYI: You don't have to wait for kinuko@'s reviews because I'm an owner of
> content/{browser,renderer}/service_worker/*. Of course, you can ask her to
> review if you want :)
>
Oh, I didn't know.
I think it's enough due to your high quality reviews.
Thank you for your helping and reviews.
https://codereview.chromium.org/1202453002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):
https://codereview.chromium.org/1202453002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:253:
BrowserThread::PostTask(
Is this correct? If it's incorrect, please let me know :)
Still LGTM https://codereview.chromium.org/1202453002/diff/100001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/100001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:253: BrowserThread::PostTask( On 2015/07/15 06:10:18, zino wrote: > Is this correct? If it's incorrect, please let me know :) Looks good. Thanks! https://codereview.chromium.org/1202453002/diff/100001/content/common/service... File content/common/service_worker/service_worker_messages.h (right): https://codereview.chromium.org/1202453002/diff/100001/content/common/service... content/common/service_worker/service_worker_messages.h:502: std::string /* message */ ) nit: There is an extra whitespace after "/* message */".
Patchset #7 (id:120001) has been deleted
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1202453002/#ps140001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202453002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2015/07/15 07:53:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) nasko@, Could you please review this CL? nhiroki@ is owner for content/common/service_worker/* but the |*_messages*.h| file is |set noparent|. Mike is also owner for the file but he seems to be OOO. Thank you.
jinho.bang@samsung.com changed reviewers: + nasko@chromium.org, palmer@chromium.org
On 2015/07/15 07:53:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) nasko@ or palmer@, Could you please review this CL? nhiroki@ is owner for content/common/service_worker/* but the |*_messages*.h| file is |set noparent|. Mike is also owner for the file but he seems to be OOO. Thank you.
https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; I think you should validate that the UUID has the lexical structure that you expect of a UUID (e.g. exactly 32 hex digits, or whatever). This string has just crossed upward over a privilege boundary, and so requires extra scrutiny. https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:919: const std::string& message) { What is the possible range of values for |message|? Any particular size restrictions, lexical or syntactic structure? Are they arbitrary, or are there only N possible messages (and hence can they be more efficiently represented as an enum)?
Thank you for review. https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; On 2015/07/15 17:07:54, palmer wrote: > I think you should validate that the UUID has the lexical structure that you > expect of a UUID (e.g. exactly 32 hex digits, or whatever). This string has just > crossed upward over a privilege boundary, and so requires extra scrutiny. Thank you for review. If you're saying that we have to check the validation of the strings in IPC communication, The client_uuid has been assigned newly from provider host (from not renderer). So, I don't think we need to check here. Please correct me if I'm wrong :)
https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; It looks like the provider host gets the UUID directly from the renderer in |OnNavigateClient|, without first checking that the UUID makes sense. (Line 1735.) Recall that the UUID comes to |OnNavigateClient| from an untrustworthy renderer: 281 // Ask the browser to navigate a client (renderer->browser). 282 IPC_MESSAGE_ROUTED3(ServiceWorkerHostMsg_NavigateClient, 283 int /* request_id */, 284 std::string /* uuid */, 285 GURL /* url */)
Thank you for answer. https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; Yes, the uuid is passed from the renderer but it is used in only |OnNavigateClient| in order to get provider host. The GetProviderHostByClientID() uses internally map data structure to find it. So, if the string is invalid then |provider_host| will be null, then browser sends NavigateErorr message to renderer. I think it is the same with validation check. In this case, do we need to check additionally? FYI, the client_uuid in |OnNavigateClientFinished| is different from the client_uuid in |OnNavigateClient|. https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:919: const std::string& message) { On 2015/07/15 17:07:54, palmer wrote: > What is the possible range of values for |message|? Any particular size > restrictions, lexical or syntactic structure? Are they arbitrary, or are there > only N possible messages (and hence can they be more efficiently represented as > an enum)? It's just error message. It uses the same way with |OpenWindowError| or |ClaimClientsError|. The way seems to be already used in many cases. The error message can be dynamic. (e.g. The requested URL is included in the message)
palmer@ and IPC owners. Could you please reply? Thank you.
https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; > Yes, the uuid is passed from the renderer but it is used in only > |OnNavigateClient| in order to get provider host. > The GetProviderHostByClientID() uses internally map data structure to find it. > So, if the string is invalid then |provider_host| will be null, then browser > sends NavigateErorr message to renderer. > I think it is the same with validation check. > In this case, do we need to check additionally? Yes, for a few reasons. 1. It's best to fail as soon as possible. 2. It's best to fail for a reason that is maximally related to the underlying problem. For example, this code is good: 1715 if (!url.is_valid()) { 1716 DVLOG(1) << "Received unexpected invalid URL from renderer process."; 3. It's easy to write a function to validate UUIDs. 4. We don't want to let untrustworthy processes (in this case, renderers) fill a privileged processes' memory with arbitrary strings of arbitrary size, when all we needed was a string with a very tight lexical definition. > FYI, the client_uuid in |OnNavigateClientFinished| is different from the > client_uuid in |OnNavigateClient|. Different how? Where did it come from? We should validate all UUIDs at each point that they cross a privilege boundary. (Then, and only then, other areas of the code can trust that the value is valid.) https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:919: const std::string& message) { > The error message can be dynamic. (e.g. The requested URL is included in the > message) How about passing a reason code (an enum) and the offending URL as a GURL? enum NavigateClientErrorReason { URL_INVALID, URL_NOT_REACHABLE, URL_..., // ... }; And then: void ServiceWorkerContextClient::OnNavigateClientError( int request_id, NavigateClientErrorReason reason, const GURL& url) { // ... }
Thank you for review. Could you please review again? https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1799: client.client_uuid = client_uuid; Done. I added the validation check to |OnNavigateClient|. (using base::IsValidGUID function) On 2015/07/20 23:15:32, palmer wrote: > Different how? Where did it come from? We should validate all UUIDs at each > point that they cross a privilege boundary. (Then, and only then, other areas of > the code can trust that the value is valid.) There is a difference between client_uuid[1] in |OnNavigateClient| and client_uuid[2] in |OnNavigateClientFinished|. [1] The first uuid comes from renderer in order to find a provider host. [2] The client_uuid is created by base::GenerateGUID() when creating provider host on browser process. It's always exists in browser side. Also the provider host and the uuid are created newly after navigating. I mean the uuid didn't come from renderer. (It comes from browser.) https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/140001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:919: const std::string& message) { Done. (without using enum, because the message isn't various.)
Ping palmer@ Please review again :) Thank you.
IPC LGTM, with some questions about error handling. https://codereview.chromium.org/1202453002/diff/150001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/150001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1750: if (!base::IsValidGUID(client_uuid)) { I almost wonder if it's more appropriate to send NavigateClientError for invalid URLs (line 1730), and/or to send RESULT_CODE_KILLED_BAD_MESSAGE for invalid UUIDs. A bad URL might be a regular mistake, but a bad UUID indicates corruption or attack? In any case, you're checking the validity of the UUIDs, so that's good. Just something to think about. https://codereview.chromium.org/1202453002/diff/150001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/150001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:921: std::string message; I might simplify this error message, and also disambiguate it (as written, the "cannot navigate" might be hard to distinguish from the URL itself): std::string message = "Cannot navigate to URL: " + url.spec();
palmer@, Thank you for review. I addressed your comments :) Could you review again? https://codereview.chromium.org/1202453002/diff/150001/content/browser/servic... File content/browser/service_worker/service_worker_version.cc (right): https://codereview.chromium.org/1202453002/diff/150001/content/browser/servic... content/browser/service_worker/service_worker_version.cc:1750: if (!base::IsValidGUID(client_uuid)) { On 2015/07/27 21:55:58, palmer wrote: > I almost wonder if it's more appropriate to send NavigateClientError for invalid > URLs (line 1730), and/or to send RESULT_CODE_KILLED_BAD_MESSAGE for invalid > UUIDs. A bad URL might be a regular mistake, but a bad UUID indicates corruption > or attack? > > In any case, you're checking the validity of the UUIDs, so that's good. Just > something to think about. Done. https://codereview.chromium.org/1202453002/diff/150001/content/renderer/servi... File content/renderer/service_worker/service_worker_context_client.cc (right): https://codereview.chromium.org/1202453002/diff/150001/content/renderer/servi... content/renderer/service_worker/service_worker_context_client.cc:921: std::string message; On 2015/07/27 21:55:58, palmer wrote: > I might simplify this error message, and also disambiguate it (as written, the > "cannot navigate" might be hard to distinguish from the URL itself): > > std::string message = "Cannot navigate to URL: " + url.spec(); Done.
Still LGTM. Thanks!
The CQ bit was checked by jinho.bang@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1202453002/#ps170001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1202453002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1202453002/170001
Message was sent while issue was closed.
Committed patchset #9 (id:170001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/a564acb41ce9fe3243643546258fb1a04783d77b Cr-Commit-Position: refs/heads/master@{#340635} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
