Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950303004:
[DevTools] Add additional accessor check in InjectedScriptSource.js (Closed)
|
Created:
4 years, 7 months ago by kozy Modified:
4 years, 7 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, kinuko+watch, kozyatinskiy+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Add additional accessor check in InjectedScriptSource.js
Let's check that getter doesn't have user defined source code additionally.
BUG=613777
R=lushnikov@chromium.org,pfeldman@chromium.org
Committed: https://crrev.com/720dc263ae351cb79b2de6ba4be59e091718bd93
Cr-Commit-Position: refs/heads/master@{#395367}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Messages
Total messages: 22 (7 generated)
please take a look!
https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/sources/debugger/properties-special-expected.txt (left): https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/sources/debugger/properties-special-expected.txt:19: caller: (...) how come this got removed? https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:465: var hasGetterFunctionSource = isGetterFunction ? InjectedScriptHost.hasFunctionSource(descriptor.get) : false; var canFormatAccessorsAsProperty = isGetterFunction ? InjectedScriptHost.hasFunctionSource(descriptor.get) : true;
all done. please take a look. https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector/sources/debugger/properties-special-expected.txt (left): https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector/sources/debugger/properties-special-expected.txt:19: caller: (...) On 2016/05/06 04:28:11, lushnikov wrote: > how come this got removed? Bound function object contains function ThrowTypeError() as getter for arguments property. It's internal V8 function and hasn't source code. We get TypeError excption while call and ignore this property in catch block because accessorPropertiesOnly flag equals true. I think that it's good change because this property doesn't add any value for users. https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950303004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:465: var hasGetterFunctionSource = isGetterFunction ? InjectedScriptHost.hasFunctionSource(descriptor.get) : false; On 2016/05/06 04:28:11, lushnikov wrote: > var canFormatAccessorsAsProperty = isGetterFunction ? > InjectedScriptHost.hasFunctionSource(descriptor.get) : true; Done.
Thanks, lgtm https://codereview.chromium.org/1950303004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950303004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:466: var canFormatAccessorsAsProperty = !(isGetterFunction ? InjectedScriptHost.hasFunctionSource(descriptor.get) : true); can we inline "!"
All done! Pavel, please take a look! https://codereview.chromium.org/1950303004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js (right): https://codereview.chromium.org/1950303004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/InjectedScriptSource.js:466: var canFormatAccessorsAsProperty = !(isGetterFunction ? InjectedScriptHost.hasFunctionSource(descriptor.get) : true); On 2016/05/06 17:58:56, lushnikov wrote: > can we inline "!" Done.
Description was changed from ========== [DevTools] Change checking binding accessors in InjectedScriptSource.js Instead of checking object for instance of binding object, we can simple check does function has user defined JS or not. It's improved our behavior by two ways: - we don't execute user defined JS when user redefines some binding object, - we support new implementation for window getters (show value instead getter/setter). BUG=595206 R=lushnikov@chromium.org,pfeldman@chromium.org ========== to ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=595206 R=lushnikov@chromium.org,pfeldman@chromium.org ==========
Add new check instead replace old. Please take a look!
ping? :)
kozyatinskiy@chromium.org changed reviewers: + dgozman@chromium.org
Dmitry, please take a look.
The bug number is wrong. Let's have more description. https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-getProperties.html (right): https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-getProperties.html:186: var expression = "var e = document.createEvent(\"Event\"); Object.defineProperty(e, \"eventPhase\", { get: function() { return 239; } })"; How does this expression return |e| to ask properties on it later?
Description was changed from ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=595206 R=lushnikov@chromium.org,pfeldman@chromium.org ========== to ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=613777 R=lushnikov@chromium.org,pfeldman@chromium.org ==========
Added another issue number. https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-getProperties.html (right): https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/inspector-protocol/runtime/runtime-getProperties.html:186: var expression = "var e = document.createEvent(\"Event\"); Object.defineProperty(e, \"eventPhase\", { get: function() { return 239; } })"; On 2016/05/21 01:22:37, dgozman wrote: > How does this expression return |e| to ask properties on it later? Object.defineProperty returns first argument.
lgtm https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp (right): https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:74: if (!info[1]->IsFunction()) How this could be possible? Why do we return |false| and not |true| then? https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:76: if (info[1].As<v8::Function>()->ScriptId() != v8::UnboundScript::kNoScriptId) Let's comment that this means user-defined code.
All done. https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp (right): https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:74: if (!info[1]->IsFunction()) On 2016/05/23 15:53:03, dgozman wrote: > How this could be possible? Why do we return |false| and not |true| then? It could be undefined and we don't want to format this kind of accessor as property. "get A function which serves as a getter for the property, or undefined if there is no getter (accessor descriptors only)." [1] [1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/1950303004/diff/70001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/v8_inspector/V8InjectedScriptHost.cpp:76: if (info[1].As<v8::Function>()->ScriptId() != v8::UnboundScript::kNoScriptId) On 2016/05/23 15:53:03, dgozman wrote: > Let's comment that this means user-defined code. Done.
The CQ bit was checked by kozyatinskiy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org, dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1950303004/#ps90001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950303004/90001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950303004/90001
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=613777 R=lushnikov@chromium.org,pfeldman@chromium.org ========== to ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=613777 R=lushnikov@chromium.org,pfeldman@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:90001)
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=613777 R=lushnikov@chromium.org,pfeldman@chromium.org ========== to ========== [DevTools] Add additional accessor check in InjectedScriptSource.js Let's check that getter doesn't have user defined source code additionally. BUG=613777 R=lushnikov@chromium.org,pfeldman@chromium.org Committed: https://crrev.com/720dc263ae351cb79b2de6ba4be59e091718bd93 Cr-Commit-Position: refs/heads/master@{#395367} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/720dc263ae351cb79b2de6ba4be59e091718bd93 Cr-Commit-Position: refs/heads/master@{#395367} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
