Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1875903002:
[DevTools] Better heuristic for detecting object literals (Closed)
|
Created:
4 years, 8 months ago by Zirak Modified:
4 years, 5 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, pfeldman, sergeyv+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] Better heuristic for detecting object literals
Improved heuristic for detecting object literals. Previously valid
blocks were detected as object literals and wrapped in parentheses.
BUG=499864, 603890
Patch Set 1 #Patch Set 2 : Add tests #
Total comments: 1
Patch Set 3 : Ensure detection doesn't incur side effects #
Total comments: 4
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 3
Patch Set 6 : #
Messages
Total messages: 31 (10 generated)
Description was changed from ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG= 499864 ========== to ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG= 499864 ==========
zirakertan@gmail.com changed reviewers: + paulirish@chromium.com, pfeldman@chromium.com
kozyatinskiy@chromium.org changed reviewers: + kozyatinskiy@chromium.org
https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:229: Function("return " + code + ";"); We can't execute user code in frontend. It can produce different problems like DevTools freeze on expression like { while(true) {} } or ReferencedError if user execute var a = 42 and then { b: a }. And we don't to execute code twice it can produce hard detectable issues, e.g. { let a = 42; ++window.i; eval("}"); This expression will produce SyntaxError with and without parentheses but window.i will be incremented twice.
On 2016/04/11 19:17:32, kozyatinskiy wrote: > https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:229: > Function("return " + code + ";"); > We can't execute user code in frontend. It can produce different problems like > DevTools freeze on expression like { while(true) {} } or ReferencedError if user > execute var a = 42 and then { b: a }. > And we don't to execute code twice it can produce hard detectable issues, e.g. > { let a = 42; ++window.i; eval("}"); > This expression will produce SyntaxError with and without parentheses but > window.i will be incremented twice. `Function` doesn't execute the code though, it only creates a function containing the specified code. For example: var foo = Function('alert(1)'); // no alert foo(); // alert Function('while (1) {}') // no freezing console.log('done!'); // gets logged like a boss Did I miss something?
On 2016/04/11 19:17:32, kozyatinskiy wrote: > https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:229: > Function("return " + code + ";"); > We can't execute user code in frontend. It can produce different problems like > DevTools freeze on expression like { while(true) {} } or ReferencedError if user > execute var a = 42 and then { b: a }. > And we don't to execute code twice it can produce hard detectable issues, e.g. > { let a = 42; ++window.i; eval("}"); > This expression will produce SyntaxError with and without parentheses but > window.i will be incremented twice. To be absolutely sure, I added a test which checks that we're running the user code only once.
kozyatinskiy@chromium.org changed reviewers: + dgozman@chromium.org
Thank you! lgtm % nits Please wait for Pavel or Dmitry review. https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:223: if (!(/^\s*\{/.test(code) && /\}\s*$/.test(code))) { Remove brackets. https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:231: catch (e) { Put catch in previous line. https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:237: Function("(" + code + ")"); Move both check to one try block. https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: catch (e) { ditto
On 2016/04/11 21:00:12, kozyatinskiy wrote: > Thank you! > lgtm % nits > > Please wait for Pavel or Dmitry review. > > https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:223: if > (!(/^\s*\{/.test(code) && /\}\s*$/.test(code))) { > Remove brackets. > > https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:231: catch (e) > { > Put catch in previous line. > > https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:237: > Function("(" + code + ")"); > Move both check to one try block. > > https://codereview.chromium.org/1875903002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: catch (e) > { > ditto Thanks! Should be fixed. Should I run any trybots? I hit the "Choose trybots" button but got scared by all the options.
https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: WebInspector.console.warn("Evaluating input as an object literal."); Let's not spam user since we do pretty good detection here. Do you expect a lot of false positives?
On 2016/04/11 21:47:07, dgozman wrote: > https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: > WebInspector.console.warn("Evaluating input as an object literal."); > Let's not spam user since we do pretty good detection here. Do you expect a lot > of false positives? Paul Irish suggested it, and after some thought I realised it makes sense. It surprised quite a few people as evident here: http://stackoverflow.com/q/36438034/617762 Maybe we can expand that further and add a link to a page on the devtools site (sort of like the Microsoft Knowledge Base...), or even to a StackOverflow question.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:231: Function("(" + code + ")"); when do we need this? Function has no surprises as far as parsing is concerned? We already do a best effort here since target and client VMs may differ, I hope we don't allow scripting devtools that way?
On 2016/04/12 18:45:30, pfeldman_ooo wrote: > https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:231: > Function("(" + code + ")"); > when do we need this? That just checks that introducing parentheses doesn't cause a syntax error. Now that you mention it, ensuring that the code can be a return's RHS could be enough. I'll play around with it to be sure and update. > Function has no surprises as far as parsing is concerned? We already do a best > effort here since target and client VMs may differ, I hope we don't allow > scripting devtools that way? From what I know `Function` doesn't pack any surprises, simply creates a function with the specified code as its body. What do you mean by "scripting devtools"? As in running the code in the devtools' context? As replied above to kozyatinskiy, since it's a `Function` call we don't run any code, and to be extra sure I've added a test in the 3rd patch set.
With the help of some friends (notably Jan Dvorak[1]) I found out that the logic
isn't good enough:
> { a: 4 }, { a: 7 }
> { a: 4 }; { a: 7 }
> { a: 4 } + { a: 7 }
I've managed to handle the logic for the first two cases, the third one is much
more difficult. I'll meditate on this.
It may not be possible without parsing the input but I'm hopeful.
[1] : http://stackoverflow.com/users/499214/jan-dvorak
On 2016/04/12 19:47:35, Zirak wrote:
> With the help of some friends (notably Jan Dvorak[1]) I found out that the
logic
> isn't good enough:
>
> > { a: 4 }, { a: 7 }
> > { a: 4 }; { a: 7 }
> > { a: 4 } + { a: 7 }
>
> I've managed to handle the logic for the first two cases, the third one is
much
> more difficult. I'll meditate on this.
> It may not be possible without parsing the input but I'm hopeful.
>
> [1] : http://stackoverflow.com/users/499214/jan-dvorak
Actually, I'm wrong: We *want* those to be treated as expressions, all the
confusing arises from them *not* being treated as expressions. Changing the
wording of the warning to "Treating input as an expression" should do those
cases justice.
Ping?
Sorry, this somehow slipped through reviews. I like the new logic! A couple of nits, and we can land this. https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:218: * @suppress {uselessCode} What is this? https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:221: function looksLikeAnObjectLiteral(code) { style: { on next line https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: WebInspector.console.warn("Evaluating input as an expression."); I still don't agree with this. Let's extract this into a separate patch, where we can discuss it. Improving the heuristic is worth on its own, so landing this patch without warning would be really good.
On 2016/04/26 23:50:28, dgozman wrote: > Sorry, this somehow slipped through reviews. I like the new logic! A couple of > nits, and we can land this. > > https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js (right): > > https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:218: * > @suppress {uselessCode} > What is this? We call `Function` purely for its side effect of throwing an error, so Closure Compiler complains at us that it's useless code. This tells it that we're cool. > https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:221: function > looksLikeAnObjectLiteral(code) { > style: { on next line Fixed. > https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: > WebInspector.console.warn("Evaluating input as an expression."); > I still don't agree with this. Let's extract this into a separate patch, where > we can discuss it. Improving the heuristic is worth on its own, so landing this > patch without warning would be really good. Fair enough, removed. Should I open that up now (maybe just as an issue?) or wait for this patch to land first?
> https://codereview.chromium.org/1875903002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/devtools/front_end/sdk/ConsoleModel.js:240: > > WebInspector.console.warn("Evaluating input as an expression."); > > I still don't agree with this. Let's extract this into a separate patch, where > > we can discuss it. Improving the heuristic is worth on its own, so landing > this > > patch without warning would be really good. > > Fair enough, removed. Should I open that up now (maybe just as an issue?) or > wait for this patch to land first? I think we can reuse the same bug, but make a separate patch. lgtm, please go ahead with landing.
The CQ bit was checked by paulirish@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kozyatinskiy@chromium.org Link to the patchset: https://codereview.chromium.org/1875903002/#ps100001 (title: " ")
Description was changed from ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG= 499864 ========== to ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG=499864, 603890 ==========
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875903002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875903002/100001
Thanks for the contribution! Appreciate it
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Merge conflict against master. Can you rebase your branch and upload a new patchset? You can use `git rebase origin/master` against your branch.
I'd love to help with this patch and rebase it but we'd lose you as the commit author. :( Whenever you're ready, we can get this rebased.
Friendly ping! What is status of this?
Description was changed from ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG=499864, 603890 ========== to ========== [DevTools] Better heuristic for detecting object literals Improved heuristic for detecting object literals. Previously valid blocks were detected as object literals and wrapped in parentheses. BUG=499864, 603890 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
