Bug 45260

Summary:

Web Inspector: upstream testExpandScope debugger test

Product:

WebKit

Reporter:

Yury Semikhatsky <yurys>

Component:

Web Inspector (Deprecated)

Assignee:

Yury Semikhatsky <yurys>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, bweinstein, commit-queue, eric, joepeck, keishi, loislo, pfeldman, pmuellr, podivilov, rik, timothy, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
[IMAGE] Safari and Chrome side by side on test case	none
[TEST] The Test Case Used in the Picture Above	none

Yury Semikhatsky

Reported 2010-09-06 05:27:35 PDT

Web Inspector: upstream testExpandScope debugger test.

Attachments
Patch (9.77 KB, patch) 2010-09-06 05:30 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (17.65 KB, patch) 2010-09-07 06:42 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
Patch (17.65 KB, patch) 2010-09-10 11:04 PDT, Yury Semikhatsky	no flags	Details Formatted Diff Diff
[IMAGE] Safari and Chrome side by side on test case (224.24 KB, image/png) 2010-09-10 11:58 PDT, Joseph Pecoraro	no flags	Details
[TEST] The Test Case Used in the Picture Above (263 bytes, text/html) 2010-09-10 11:58 PDT, Joseph Pecoraro	no flags	Details
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Yury Semikhatsky

Comment 1 2010-09-06 05:30:27 PDT

Created attachment 66631 [details] Patch

Ilya Tikhonovsky

Comment 2 2010-09-06 05:49:53 PDT

Comment on attachment 66631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66631&action=prettypatch > LayoutTests/inspector/debugger-expand-scope-expected.txt:8 > + arguments: Arguments[1] it would be better to show arguments in chromium too. > LayoutTests/inspector/debugger-expand-scope-expected.txt:12 > +- DOMWindowWith Block ditto > LayoutTests/inspector/debugger-expand-scope-expected.txt:13 > + innerFunction: function innerFunction(x) { ditto > LayoutTests/inspector/debugger-expand-scope-expected.txt:15 > + arguments: Arguments[1] ditto > LayoutTests/inspector/debugger-expand-scope-expected.txt:18 > +- DOMWindowGlobal In chromium we have ObjectGlobal here. Looks like we have some problems in Chromium.

Yury Semikhatsky

Comment 3 2010-09-07 06:42:55 PDT

Created attachment 66716 [details] Patch

Yury Semikhatsky

Comment 4 2010-09-07 06:48:36 PDT

(In reply to comment #2) > (From update of attachment 66631 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=66631&action=prettypatch > > > LayoutTests/inspector/debugger-expand-scope-expected.txt:8 > > + arguments: Arguments[1] > it would be better to show arguments in chromium too. > arguments object is created lazily in v8 while JSC always exposes it. Moreover arguments is automatically saved in function closure in JSC and v8 closure keeps only those variables which can be later accessed. Any attempt to access arguments array from the closure will lead to access to the arguments of the function itself. So I'd prefer to provide custom expectations in this case. > > LayoutTests/inspector/debugger-expand-scope-expected.txt:18 > > +- DOMWindowGlobal > In chromium we have ObjectGlobal here. > > Looks like we have some problems in Chromium. Fixed.

Joseph Pecoraro

Comment 5 2010-09-09 13:50:04 PDT

Comment on attachment 66716 [details] Patch > diff --git LayoutTests/inspector/debugger-expand-scope-expected.txt > + Test that sections representing scopes of the current call frame are expandable and contain correct data. Note the extra space before "Test". I think that is because the test doesn't wrap the text in a <p> like we usually do. Please add a <p>. This applies to both tests. > +<body onload="runTest()"> > +<input type='button' onclick='testFunction()' value='Test'/> > +Test that sections representing scopes of the current call frame are expandable > +and contain correct data. > +</body> > +- DOMWindowWith Block > + innerFunction: function innerFunction(x) { Any idea why JSC and v8 are so different here? > diff --git a/WebCore/ChangeLog > + * inspector/front-end/InjectedScript.js: return "Arguments" as class name for arguments variable in v8. This doesn't show up in the test results for chromium. Is that expected? > WebKit/chromium/src/js/DebuggerScript.js:246 > + continue; // Skipe internal variables like .arguments Typo: "Skipe" => "Skip" Typo: ".arguments" => "arguments."

Yury Semikhatsky

Comment 6 2010-09-10 11:04:05 PDT

Created attachment 67205 [details] Patch

Yury Semikhatsky

Comment 7 2010-09-10 11:09:24 PDT

(In reply to comment #5) > (From update of attachment 66716 [details]) > > diff --git LayoutTests/inspector/debugger-expand-scope-expected.txt > > + Test that sections representing scopes of the current call frame are expandable and contain correct data. > > Note the extra space before "Test". I think that is because the test doesn't > wrap the text in a <p> like we usually do. Please add a <p>. This applies > to both tests. Fixed. > > > +<body onload="runTest()"> > > +<input type='button' onclick='testFunction()' value='Test'/> > > +Test that sections representing scopes of the current call frame are expandable > > +and contain correct data. > > +</body> > > > > +- DOMWindowWith Block > > + innerFunction: function innerFunction(x) { > > Any idea why JSC and v8 are so different here? > The difference lies in the bindings implementation I guess. > > diff --git a/WebCore/ChangeLog > > + * inspector/front-end/InjectedScript.js: return "Arguments" as class name for arguments variable in v8. > > This doesn't show up in the test results for chromium. Is that expected? > Yes, it is. In v8 Arguments object is created only when it's accessed in the function. In this particular case we don't touch arguments so there is no such object. > > > WebKit/chromium/src/js/DebuggerScript.js:246 > > + continue; // Skipe internal variables like .arguments > > Typo: "Skipe" => "Skip" Fixed. > Typo: ".arguments" => "arguments." This is not a typo. It's a name of v8 internal variable.

Joseph Pecoraro

Comment 8 2010-09-10 11:57:37 PDT

> > Any idea why JSC and v8 are so different here? > > > The difference lies in the bindings implementation I guess. This interests me. I thought it was a difference with `with` blocks but it looks like both showed `with` blocks but both had very different scope chains. On Chrome, I couldn't access the "closureObj" which I'm pretty sure I could. See the attached picture and test case. It sounds like this is something that this test should be covering. > > > + * inspector/front-end/InjectedScript.js: return "Arguments" as class name for arguments variable in v8. > > > > This doesn't show up in the test results for chromium. Is that expected? > > > Yes, it is. In v8 Arguments object is created only when it's accessed in the > function. In this particular case we don't touch arguments so there is no such object. Thanks for that explanation. In that case I think it would be worth testing an example with a function that does access "argument" for v8 to have this show up in a test's expected results. Let me know if that sounds overkill. > > Typo: ".arguments" => "arguments." > This is not a typo. It's a name of v8 internal variable. Okay, good to know. Thanks.

Joseph Pecoraro

Comment 9 2010-09-10 11:58:00 PDT

Created attachment 67216 [details] [IMAGE] Safari and Chrome side by side on test case

Joseph Pecoraro

Comment 10 2010-09-10 11:58:30 PDT

Created attachment 67217 [details] [TEST] The Test Case Used in the Picture Above

Yury Semikhatsky

Comment 11 2010-09-13 05:10:45 PDT

(In reply to comment #8) > > > Any idea why JSC and v8 are so different here? > > > > > The difference lies in the bindings implementation I guess. > > This interests me. I thought it was a difference with `with` blocks but > it looks like both showed `with` blocks but both had very different scope > chains. On Chrome, I couldn't access the "closureObj" which I'm pretty > sure I could. See the attached picture and test case. It sounds like > this is something that this test should be covering. > The problem is that V8 places in a closure only variables that are accessed from the inner function. In your example function a() doesn't use any variables from the scope of outer function and that is why there is no closure scope at all. JSC places pointers to all objects from the outer scopes no matter will they be accessed from function a() or not and that is why you see the closure object in Safari. > > > > + * inspector/front-end/InjectedScript.js: return "Arguments" as class name for arguments variable in v8. > > > > > > This doesn't show up in the test results for chromium. Is that expected? > > > > > Yes, it is. In v8 Arguments object is created only when it's accessed in the > > function. In this particular case we don't touch arguments so there is no such object. > > Thanks for that explanation. In that case I think it would be worth testing > an example with a function that does access "argument" for v8 to have > this show up in a test's expected results. Let me know if that sounds overkill. > > This sounds good but it's a different functionality that deserves its own test.

WebKit Commit Bot

Comment 12 2010-09-13 06:25:59 PDT

Comment on attachment 67205 [details] Patch Rejecting patch 67205 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20978 test cases. inspector/extensions-events.html -> failed Exiting early after 1 failures. 17063 tests run. 316.70s total testing time 17062 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 26 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3921433

Yury Semikhatsky

Comment 13 2010-09-13 06:40:22 PDT

Comment on attachment 67205 [details] Patch Clearing flags on attachment: 67205 Committed r67385: <http://trac.webkit.org/changeset/67385>

Yury Semikhatsky

Comment 14 2010-09-13 06:40:36 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 15 2010-09-13 07:09:48 PDT

http://trac.webkit.org/changeset/67385 might have broken Qt Linux Release

Joseph Pecoraro

Comment 16 2010-09-13 10:02:30 PDT

> The problem is that V8 places in a closure only variables that are accessed from the > inner function. In your example function a() doesn't use any variables from the scope > of outer function and that is why there is no closure scope at all. JSC places pointers > to all objects from the outer scopes no matter will they be accessed from function > a() or not and that is why you see the closure object in Safari. Okay, so looking at my example above, v8 has an issue for the inspector. Since that variable is in scope. Is it not accessible because it was optimized out by v8 because it wasn't used by the code? I would still want it to show up in the console on evaluation.

Yury Semikhatsky

Comment 17 2010-09-14 00:50:29 PDT

(In reply to comment #16) > > The problem is that V8 places in a closure only variables that are accessed from the > > inner function. In your example function a() doesn't use any variables from the scope > > of outer function and that is why there is no closure scope at all. JSC places pointers > > to all objects from the outer scopes no matter will they be accessed from function > > a() or not and that is why you see the closure object in Safari. > > Okay, so looking at my example above, v8 has an issue for the inspector. Since that variable > is in scope. Is it not accessible because it was optimized out by v8 because it wasn't used by > the code? I would still want it to show up in the console on evaluation. It was optimized out from the closure of function a but it's still present in the activation record of the outer anonymous function and you can see it in the local scope of the bottom call frame. You can't use it in evaluation on the top frame though as it's not in the closure.

Note You need to log in before you can comment on or make changes to this bug.