Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2058133002:
Fix DevTools support of worklets (Closed)
|
Created:
4 years, 6 months ago by Gleb Lanbin Modified:
4 years, 5 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, sergeyv+blink_chromium.org, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL.
BUG=567358
TEST=None
Committed: https://crrev.com/4d2c605c2b105b2b52d6090d2f8610eaaf839746
Cr-Commit-Position: refs/heads/master@{#402873}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix #
Total comments: 3
Patch Set 3 : worklet can access itself #Patch Set 4 : get domWindow from worklet's scope #Patch Set 5 : fallback to toDOMWindow(calling) if callingWorkletScope->frame() is null #
Total comments: 5
Patch Set 6 : move callingContextCanAccessContext to BindingSecurity #
Total comments: 2
Patch Set 7 : move frame extraction logic to BindingSecurity #
Total comments: 2
Patch Set 8 : moved MainThreadDebugger::callingContextCanAccessContext's logic to BindingSecurity #
Total comments: 6
Patch Set 9 : fix comments #
Messages
Total messages: 43 (16 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Fix DevTools support of worklets. BUG=567358 TEST=None ========== to ========== Fix DevTools support of worklets. This changes MainThreadDebugger::callingContextCanAccessContext to return true if we're in worklet's execution scope. This is similar to the approach used in Workers(WorkerThreadDebugger::callingContextCanAccessContext). The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ==========
glebl@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2058133002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:191: // Similar to WorkerThreadDebugger::callingContextCanAccessContext(http://crrev.com/1638563002). We can return |true| in workers just because we know that it's isolate contains only a single worker, so any access is allowed. Here in main thread isolate, we may have a lot of different pages (possibly with worklets), so access check is required. I think, we should retrieve a LocalDOMWindow for both calling and target regardless of whether they are worklets or not, and then ask BindingSecurity.
https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:200: if (globalScope) { This code only runs if |target| is a worklet. You should move it outside of if statement.
https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:200: if (globalScope) { On 2016/06/24 22:03:15, dgozman wrote: > This code only runs if |target| is a worklet. You should move it outside of if > statement. not sure if it helps. In my case when I stop inside of the worklet |calling| is the context of my worklet function that doesn't have DOM window associated with it and I'm not sure what is the |target| in my case. I think |target| is my main CSS paint function defined in CSSPaintDefinition.cpp. It doesn't have DOM window as well. Maybe I should just verify that |target| and |calling| are running in MainThreadWorkletGlobalScope execution context and let it go? Something like if (toExecutionContext(target).isMainThreadWorkletGlobalScope() && toExecutionContext(calling).isMainThreadWorkletGlobalScope()) return true; what do you think?
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Fix DevTools support of worklets. This changes MainThreadDebugger::callingContextCanAccessContext to return true if we're in worklet's execution scope. This is similar to the approach used in Workers(WorkerThreadDebugger::callingContextCanAccessContext). The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ========== to ========== Fix DevTools support of worklets. Change callingContextCanAccessContext so it lets the calling context(worklet's injected script) to access the worklet execution context. The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ==========
Patchset #3 (id:80001) has been deleted
Description was changed from ========== Fix DevTools support of worklets. Change callingContextCanAccessContext so it lets the calling context(worklet's injected script) to access the worklet execution context. The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ========== to ========== Fix DevTools support of worklets. Change callingContextCanAccessContext so it lets worklet context to access itself. The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ==========
glebl@chromium.org changed reviewers: + ikilpatrick@chromium.org
https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:200: if (globalScope) { On 2016/06/25 00:17:26, glebl wrote: > On 2016/06/24 22:03:15, dgozman wrote: > > This code only runs if |target| is a worklet. You should move it outside of if > > statement. > > not sure if it helps. In my case when I stop inside of the worklet |calling| is > the context of my worklet function that doesn't have DOM window associated with > it and I'm not sure what is the |target| in my case. I think |target| is my main > CSS paint function defined in CSSPaintDefinition.cpp. It doesn't have DOM window > as well. Take a look at how BindingSecurity::shouldAllowAccessTo retrieves a frame and then DOM window from a worklet. > > Maybe I should just verify that |target| and |calling| are running in > MainThreadWorkletGlobalScope execution context and let it go? > Something like > > if (toExecutionContext(target).isMainThreadWorkletGlobalScope() && > toExecutionContext(calling).isMainThreadWorkletGlobalScope()) return true; > what do you think? No, in this case we should check that worklets are from the frames which can script each other. We cannot skip security checks.
PTAL
Patchset #5 (id:140001) has been deleted
dgozman@chromium.org changed reviewers: + yukishiino@chromium.org
@yukishiino: could you please take a look? Should we move this code to BindingSecurity? https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:195: if (executionContext->isMainThreadWorkletGlobalScope()) { I think this code is too complicated now, and should be moved to BindingSecurity instead. Maybe the whole method taking two contexts can live there? It's bindings after all. Let's ask owners.
https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:199: if (callingExecutionContext->isMainThreadWorkletGlobalScope()) { This looks pretty good. However, adding the reverse check would be even better.
https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:195: if (executionContext->isMainThreadWorkletGlobalScope()) { On 2016/06/27 23:12:33, dgozman wrote: > I think this code is too complicated now, and should be moved to BindingSecurity > instead. Maybe the whole method taking two contexts can live there? It's > bindings after all. Let's ask owners. Yes, please move this code into BindingSecurity. BindingSecurity should be responsible about the origin check, and we want it to be one-stop.
https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:195: if (executionContext->isMainThreadWorkletGlobalScope()) { On 2016/06/28 04:38:53, Yuki wrote: > On 2016/06/27 23:12:33, dgozman wrote: > > I think this code is too complicated now, and should be moved to > BindingSecurity > > instead. Maybe the whole method taking two contexts can live there? It's > > bindings after all. Let's ask owners. > > Yes, please move this code into BindingSecurity. BindingSecurity should be > responsible about the origin check, and we want it to be one-stop. Done. https://codereview.chromium.org/2058133002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:199: if (callingExecutionContext->isMainThreadWorkletGlobalScope()) { On 2016/06/27 23:13:38, dgozman wrote: > This looks pretty good. However, adding the reverse check would be even better. Done.
Description was changed from ========== Fix DevTools support of worklets. Change callingContextCanAccessContext so it lets worklet context to access itself. The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ========== to ========== The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ==========
https://codereview.chromium.org/2058133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h (left): https://codereview.chromium.org/2058133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h:28: virtual bool callingContextCanAccessContext(v8::Local<v8::Context> calling, v8::Local<v8::Context> target) = 0; Sorry, I wasn't clear here. We should live this method and it's implementations in MainThreadDebugger and WorkerThreadDebugger. But from MainThreadDebugger's one we should just call BindingSecurity's method, which will extract dom windows, frames, worklet global scopes itself and perform the security check. So, changes only go to MainThreadDebugger and BidningSecurity files.
https://codereview.chromium.org/2058133002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h (left): https://codereview.chromium.org/2058133002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/v8_inspector/public/V8DebuggerClient.h:28: virtual bool callingContextCanAccessContext(v8::Local<v8::Context> calling, v8::Local<v8::Context> target) = 0; On 2016/06/28 18:31:14, dgozman wrote: > Sorry, I wasn't clear here. We should live this method and it's implementations > in MainThreadDebugger and WorkerThreadDebugger. But from MainThreadDebugger's > one we should just call BindingSecurity's method, which will extract dom > windows, frames, worklet global scopes itself and perform the security check. > So, changes only go to MainThreadDebugger and BidningSecurity files. Done.
@yukishiino: could you please take a look? https://codereview.chromium.org/2058133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:198: if (targetExecutionContext->isMainThreadWorkletGlobalScope()) I'd even let BindingSecurity do these if checks for us, and introduce BindingSecurity::shouldAllowAccessTo(v8::Isolate*, ExecutionContext* calling, ExecutionContext* target)
https://codereview.chromium.org/2058133002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:198: if (targetExecutionContext->isMainThreadWorkletGlobalScope()) On 2016/06/28 19:21:00, dgozman wrote: > I'd even let BindingSecurity do these if checks for us, and introduce > BindingSecurity::shouldAllowAccessTo(v8::Isolate*, ExecutionContext* calling, > ExecutionContext* target) discussed offline. moved the entire logic to BindingSecurity as suggested by dgozman@.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:190: bool MainThreadDebugger::callingContextCanAccessContext(v8::Local<v8::Context> calling, v8::Local<v8::Context> target) Not directly related to the CL, but is there any reason you have to use a calling context here? We're deprecating a calling context (because it's broken in many ways) and replacing it with a current context. Would it be possible to use a current context instead?
LGTM on my side with nits. https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:35: #include "core/dom/ExecutionContext.h" Could you use a forward declaration instead? class ExecutionContext; should be enough. https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:76: static bool shouldAllowAccessTo(v8::Isolate*, const v8::Local<v8::Context>&, const ExecutionContext*, const MainThreadWorkletGlobalScope* target, SecurityReportingOption); I think const v8::Local<>& doesn't make much sense. Just use v8::Local<v8::Context> without a const reference.
This CL LGTM, though I want to address the calling context issue in a follow up.
On 2016/06/29 00:23:36, haraken wrote: > https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): > > https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:190: bool > MainThreadDebugger::callingContextCanAccessContext(v8::Local<v8::Context> > calling, v8::Local<v8::Context> target) > > Not directly related to the CL, but is there any reason you have to use a > calling context here? We're deprecating a calling context (because it's broken > in many ways) and replacing it with a current context. Would it be possible to > use a current context instead? See https://codereview.chromium.org/2040053003/ for more discussion on this topic.
inspector lgtm
thanks for the review. https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right): https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:35: #include "core/dom/ExecutionContext.h" On 2016/06/29 00:30:37, Yuki wrote: > Could you use a forward declaration instead? > > class ExecutionContext; > > should be enough. Done. https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:76: static bool shouldAllowAccessTo(v8::Isolate*, const v8::Local<v8::Context>&, const ExecutionContext*, const MainThreadWorkletGlobalScope* target, SecurityReportingOption); On 2016/06/29 00:30:37, Yuki wrote: > I think > const v8::Local<>& > doesn't make much sense. > > Just use v8::Local<v8::Context> without a const reference. Done. https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2058133002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:190: bool MainThreadDebugger::callingContextCanAccessContext(v8::Local<v8::Context> calling, v8::Local<v8::Context> target) On 2016/06/29 00:23:35, haraken wrote: > > Not directly related to the CL, but is there any reason you have to use a > calling context here? We're deprecating a calling context (because it's broken > in many ways) and replacing it with a current context. Would it be possible to > use a current context instead? please see the reply from dgozman@ above in this thread
The CQ bit was checked by glebl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, dgozman@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2058133002/#ps240001 (title: "fix comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ========== to ========== The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None ========== to ========== The testing is done by verifying the inspector behavior in content_shell/Chrome. Dedicated worklet's inspector tests will be provided in another CL. BUG=567358 TEST=None Committed: https://crrev.com/4d2c605c2b105b2b52d6090d2f8610eaaf839746 Cr-Commit-Position: refs/heads/master@{#402873} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4d2c605c2b105b2b52d6090d2f8610eaaf839746 Cr-Commit-Position: refs/heads/master@{#402873}
Message was sent while issue was closed.
Friendly sheriff passing by this cl in the middle of a bisect:
for next CLs please repeat the CL title ("Fix DevTools support of worklets" in
this case) as the first line of the commit message.
This CL shows in git log as:
"The testing is done by verifying the inspector behavior ...", which doesn't
help too much :)
See
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
for more details
Message was sent while issue was closed.
On 2016/07/01 08:58:26, Primiano Tucci wrote:
> Friendly sheriff passing by this cl in the middle of a bisect:
> for next CLs please repeat the CL title ("Fix DevTools support of worklets" in
> this case) as the first line of the commit message.
> This CL shows in git log as:
> "The testing is done by verifying the inspector behavior ...", which doesn't
> help too much :)
>
> See
>
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
> for more details
ack. Unfortunately a bit more detailed CL description was lost somewhere in
between multiple revisions of this CL. Also I was under impression that subject
would be included in git log message.
Thanks for the link.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
