|
|
Created:
3 years, 8 months ago by adithyas Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, blink-reviews-dom_chromium.org, blink-reviews-frames_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, Yoav Weiss Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove context checks in CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld
The checks appear to be unnecessary as isolate->GetCurrentContext() should return a valid context object if isolate->InContext() returns true, and the script state field is set on v8::Context objects immediately after they are created, so the context should be properly initialized.
BUG=682322
Review-Url: https://codereview.chromium.org/2827763002
Cr-Commit-Position: refs/heads/master@{#466004}
Committed: https://chromium.googlesource.com/chromium/src/+/bd0add4325a532ca0a0d9c23eaa55f0084317b6f
Patch Set 1 #Patch Set 2 : Remove toLocalDOMWindow checks #Messages
Total messages: 25 (14 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
adithyas@chromium.org changed reviewers: + haraken@chromium.org, jbroman@chromium.org
I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my guess is that it checks if the global object has been set in the context (and has not been detached). If that's the case, maybe I could just change the check to see if context->Global() returns an empty handle (and then I don't need to move these functions)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/04/18 19:34:40, adithyas wrote: > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my guess > is that it checks if the global object has been set in the context (and has not > been detached). If that's the case, maybe I could just change the check to see > if context->Global() returns an empty handle (and then I don't need to move > these functions)? Yeah, I don't know either. Can you quickly check with a git blame history? In the first place, ActivityLogger is a best-effort service to detect a Chrome extension that seems to be doing something weird. So the implementation is ad-hoc in many ways and not intending to be accurate. So, I think it's fine to simply remove the ToLocalDOMWindow() check.
On 2017/04/18 20:25:25, haraken wrote: > On 2017/04/18 19:34:40, adithyas wrote: > > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my > guess > > is that it checks if the global object has been set in the context (and has > not > > been detached). If that's the case, maybe I could just change the check to see > > if context->Global() returns an empty handle (and then I don't need to move > > these functions)? > > Yeah, I don't know either. Can you quickly check with a git blame history? > > In the first place, ActivityLogger is a best-effort service to detect a Chrome > extension that seems to be doing something weird. So the implementation is > ad-hoc in many ways and not intending to be accurate. So, I think it's fine to > simply remove the ToLocalDOMWindow() check. I didn't check anything for myself, but at a glace I think that it's because ActivityLogger (and almost everything) doesn't work on a remote window / remote context.
On 2017/04/19 09:45:35, Yuki wrote: > On 2017/04/18 20:25:25, haraken wrote: > > On 2017/04/18 19:34:40, adithyas wrote: > > > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my > > guess > > > is that it checks if the global object has been set in the context (and has > > not > > > been detached). If that's the case, maybe I could just change the check to > see > > > if context->Global() returns an empty handle (and then I don't need to move > > > these functions)? > > > > Yeah, I don't know either. Can you quickly check with a git blame history? > > > > In the first place, ActivityLogger is a best-effort service to detect a Chrome > > extension that seems to be doing something weird. So the implementation is > > ad-hoc in many ways and not intending to be accurate. So, I think it's fine to > > simply remove the ToLocalDOMWindow() check. > > I didn't check anything for myself, but at a glace I think that it's because > ActivityLogger (and almost everything) doesn't work on a remote window / remote > context. By the way, if !context.IsEmpty(), then context->Global() never returns an empty handle, even for a remote context, I believe.
On 2017/04/19 at 09:46:45, yukishiino wrote: > On 2017/04/19 09:45:35, Yuki wrote: > > On 2017/04/18 20:25:25, haraken wrote: > > > On 2017/04/18 19:34:40, adithyas wrote: > > > > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my > > > guess > > > > is that it checks if the global object has been set in the context (and has > > > not > > > > been detached). If that's the case, maybe I could just change the check to > > see > > > > if context->Global() returns an empty handle (and then I don't need to move > > > > these functions)? > > > > > > Yeah, I don't know either. Can you quickly check with a git blame history? > > > > > > In the first place, ActivityLogger is a best-effort service to detect a Chrome > > > extension that seems to be doing something weird. So the implementation is > > > ad-hoc in many ways and not intending to be accurate. So, I think it's fine to > > > simply remove the ToLocalDOMWindow() check. > > > > I didn't check anything for myself, but at a glace I think that it's because > > ActivityLogger (and almost everything) doesn't work on a remote window / remote > > context. > > By the way, if !context.IsEmpty(), then context->Global() never returns an empty handle, even for a remote context, I believe. I found a CL (https://codereview.chromium.org/357873003) that added in a toDOMWindow() check along with !context.IsEmpty(), to check if the context is "properly initialized". This was only recently changed to toLocalDOMWindow() in (http://crrev.com/2723973002). I don't think the check was initially intended to protect against the remote window case. So, it sounds like !context.IsEmpty() should suffice as a check to see if the context was properly initialized?
On 2017/04/19 at 17:05:51, adithyas wrote: > On 2017/04/19 at 09:46:45, yukishiino wrote: > > On 2017/04/19 09:45:35, Yuki wrote: > > > On 2017/04/18 20:25:25, haraken wrote: > > > > On 2017/04/18 19:34:40, adithyas wrote: > > > > > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my > > > > guess > > > > > is that it checks if the global object has been set in the context (and has > > > > not > > > > > been detached). If that's the case, maybe I could just change the check to > > > see > > > > > if context->Global() returns an empty handle (and then I don't need to move > > > > > these functions)? > > > > > > > > Yeah, I don't know either. Can you quickly check with a git blame history? > > > > > > > > In the first place, ActivityLogger is a best-effort service to detect a Chrome > > > > extension that seems to be doing something weird. So the implementation is > > > > ad-hoc in many ways and not intending to be accurate. So, I think it's fine to > > > > simply remove the ToLocalDOMWindow() check. > > > > > > I didn't check anything for myself, but at a glace I think that it's because > > > ActivityLogger (and almost everything) doesn't work on a remote window / remote > > > context. > > > > By the way, if !context.IsEmpty(), then context->Global() never returns an empty handle, even for a remote context, I believe. > > I found a CL (https://codereview.chromium.org/357873003) that added in a toDOMWindow() check along with !context.IsEmpty(), to check if the context is "properly initialized". This was only recently changed to toLocalDOMWindow() in (http://crrev.com/2723973002). I don't think the check was initially intended to protect against the remote window case. So, it sounds like !context.IsEmpty() should suffice as a check to see if the context was properly initialized? Both of those checks seem suspect to me. 1. How can context be empty? If v8::Isolate::InContext returns true, we're definitely in a context. And that context should have a native context (AFAIK V8 never clears it). So v8::Isolate::GetCurrentContext should return a context. 2. We should be setting the ScriptState internal field on the v8::Context immediately after creating it, and AFAICT we do so at all call sites. The only thing I can see this guarding against is a worker context (where the global object won't be a window), but that shouldn't be a problem (there just isn't a DOM activity logger in those contexts). Maybe this check is no longer required, and we could try removing it.
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/19 at 20:35:25, jbroman wrote: > On 2017/04/19 at 17:05:51, adithyas wrote: > > On 2017/04/19 at 09:46:45, yukishiino wrote: > > > On 2017/04/19 09:45:35, Yuki wrote: > > > > On 2017/04/18 20:25:25, haraken wrote: > > > > > On 2017/04/18 19:34:40, adithyas wrote: > > > > > > I'm not exactly sure what purpose the !ToLocalDOMWindow() check serves, my > > > > > guess > > > > > > is that it checks if the global object has been set in the context (and has > > > > > not > > > > > > been detached). If that's the case, maybe I could just change the check to > > > > see > > > > > > if context->Global() returns an empty handle (and then I don't need to move > > > > > > these functions)? > > > > > > > > > > Yeah, I don't know either. Can you quickly check with a git blame history? > > > > > > > > > > In the first place, ActivityLogger is a best-effort service to detect a Chrome > > > > > extension that seems to be doing something weird. So the implementation is > > > > > ad-hoc in many ways and not intending to be accurate. So, I think it's fine to > > > > > simply remove the ToLocalDOMWindow() check. > > > > > > > > I didn't check anything for myself, but at a glace I think that it's because > > > > ActivityLogger (and almost everything) doesn't work on a remote window / remote > > > > context. > > > > > > By the way, if !context.IsEmpty(), then context->Global() never returns an empty handle, even for a remote context, I believe. > > > > I found a CL (https://codereview.chromium.org/357873003) that added in a toDOMWindow() check along with !context.IsEmpty(), to check if the context is "properly initialized". This was only recently changed to toLocalDOMWindow() in (http://crrev.com/2723973002). I don't think the check was initially intended to protect against the remote window case. So, it sounds like !context.IsEmpty() should suffice as a check to see if the context was properly initialized? > > Both of those checks seem suspect to me. > > 1. How can context be empty? If v8::Isolate::InContext returns true, we're definitely in a context. And that context should have a native context (AFAIK V8 never clears it). So v8::Isolate::GetCurrentContext should return a context. > 2. We should be setting the ScriptState internal field on the v8::Context immediately after creating it, and AFAICT we do so at all call sites. > > The only thing I can see this guarding against is a worker context (where the global object won't be a window), but that shouldn't be a problem (there just isn't a DOM activity logger in those contexts). > > Maybe this check is no longer required, and we could try removing it. That makes sense to me, I've updated the CL. I'm not sure exactly what caused the failure in http://crbug.com/389118 though, and since there is no regression test for this, I can't confirm that nothing breaks by removing the checks.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
LGTM. Could you update the CL subject and description?
Description was changed from ========== Move CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld to V8Binding Both CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld use ToLocalDOMWindow() to check if the global object is attached. V8DOMActivityLogger will be moving to platform/binding and cannot host these methods. Since these methods aren't tied to V8DOMActivityLogger in any way (except for the fact that they return an instance of V8DOMActivityLogger), this CL moves these 2 functions into V8Binding. BUG=682322 ========== to ========== Remove context checks in CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld The checks appear to be unnecessary as isolate->GetCurrentContext() should return a valid context object if isolate->InContext() returns true, and the script state field is set on v8::Context objects immediately after they are created, so the context should be properly initialized. BUG=682322 ==========
The CQ bit was checked by adithyas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492698945934090, "parent_rev": "f7de9f05260ea183c85c8f360de32d62ddf55a0a", "commit_rev": "bd0add4325a532ca0a0d9c23eaa55f0084317b6f"}
Message was sent while issue was closed.
Description was changed from ========== Remove context checks in CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld The checks appear to be unnecessary as isolate->GetCurrentContext() should return a valid context object if isolate->InContext() returns true, and the script state field is set on v8::Context objects immediately after they are created, so the context should be properly initialized. BUG=682322 ========== to ========== Remove context checks in CurrentActivityLogger and CurrentActivityLoggerIfIsolatedWorld The checks appear to be unnecessary as isolate->GetCurrentContext() should return a valid context object if isolate->InContext() returns true, and the script state field is set on v8::Context objects immediately after they are created, so the context should be properly initialized. BUG=682322 Review-Url: https://codereview.chromium.org/2827763002 Cr-Commit-Position: refs/heads/master@{#466004} Committed: https://chromium.googlesource.com/chromium/src/+/bd0add4325a532ca0a0d9c23eaa5... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/bd0add4325a532ca0a0d9c23eaa5... |