Bug 101331

Hrm. JSC isn't behaving the way I expected it to. The patch I'll attach in a moment is a WIP that passes inspector tests in chromium-linux, but fails miserably on mac. I'm not sure why, however. It looks like it should work. :)

Created attachment 172570 [details] WIP: V8 works.

Created attachment 172756 [details] WIP: Rebased for bots.

Comment on attachment 172756 [details] WIP: Rebased for bots. Attachment 172756 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14747727 New failing tests: http/tests/inspector/network/cached-resource-destroyed-too-big-discarded.html editing/pasteboard/paste-double-nested-blockquote-before-blockquote.html http/tests/inspector/change-iframe-src.html editing/execCommand/outdent-blockquote-test1.html editing/pasteboard/copy-paste-float.html http/tests/inspector/network/har-content.html editing/inserting/insert-br-quoted-007.html http/tests/inspector-enabled/dedicated-workers-list.html http/tests/inspector-enabled/injected-script-discard.html editing/execCommand/outdent-blockquote-test3.html http/tests/cookies/multiple-cookies.html editing/execCommand/indent-pre.html http/tests/inspector/network/async-xhr-json-mime-type.html http/tests/inspector/appcache/appcache-swap.html editing/pasteboard/paste-blockquote-before-blockquote.html http/tests/inspector-enabled/console-clear-arguments-on-frame-navigation.html editing/execCommand/outdent-blockquote-test4.html http/tests/cookies/single-quoted-value.html editing/execCommand/outdent-blockquote-test2.html http/tests/inspector/network/cached-resource-destroyed-moved-to-storage.html http/tests/inspector/extensions-ignore-cache.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector-enabled/console-clear-arguments-on-frame-remove.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html accessibility/img-alt-tag-only-whitespace.html http/tests/inspector-enabled/document-write.html http/tests/history/back-during-onload-triggered-by-back.html http/tests/appcache/interrupted-update.html

Comment on attachment 172756 [details] WIP: Rebased for bots. View in context: https://bugs.webkit.org/attachment.cgi?id=172756&action=review > Source/WebCore/inspector/ConsoleMessage.cpp:78 > + ASSERT_NOT_REACHED(); I vaguely recall that there were some cases when the stack was empty but can't remember exact details. > Source/WebCore/page/Console.cpp:309 > + RefPtr<ScriptCallStack> callStack(createScriptCallStackForConsole(state)); It looks like we can capture only top frame of the call stack here and in Console::profile.

(In reply to comment #1) > Hrm. JSC isn't behaving the way I expected it to. The patch I'll attach in a moment is a WIP that passes inspector tests in chromium-linux, but fails miserably on mac. I'm not sure why, however. It looks like it should work. :) What is the text diff for the failing tests? At first glance there is nothing suspicious about JSC changes. I'm wondering why non-http tests are not failing.

(In reply to comment #6) > (In reply to comment #1) > > Hrm. JSC isn't behaving the way I expected it to. The patch I'll attach in a moment is a WIP that passes inspector tests in chromium-linux, but fails miserably on mac. I'm not sure why, however. It looks like it should work. :) > > What is the text diff for the failing tests? At first glance there is nothing suspicious about JSC changes. I'm wondering why non-http tests are not failing. The actual results are the expected results minus line numbers. That is, instead of "CONSOLE MESSAGE: line 8: [Message goes here]", we get "CONSOLE MESSAGE: [Message goes here]". I'll generate a new result set and upload it in a moment.

Created attachment 172954 [details] 'inspector/console' results. This is what I'm seeing, Yury.

Created attachment 172955 [details] 'http/tests/inspector' results. More of the same, but in HTTP land. :)

Comment on attachment 172756 [details] WIP: Rebased for bots. View in context: https://bugs.webkit.org/attachment.cgi?id=172756&action=review > Source/WebCore/page/Console.cpp:174 > + RefPtr<ScriptCallStack> callStack(createScriptCallStackForConsole(arguments->globalState())); I guess I figured out what makes the test fail in JSC. You now take globalState() from the arguments which retrieves it from its field of type ScriptStateProtectedPtr. ScriptStateProtectedPtr always returns pointer to the global ExecState (see http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/ScriptState.cpp#L55) while here we need current ExecState to collect the stack trace.

(In reply to comment #10) > (From update of attachment 172756 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172756&action=review > > > Source/WebCore/page/Console.cpp:174 > > + RefPtr<ScriptCallStack> callStack(createScriptCallStackForConsole(arguments->globalState())); > > I guess I figured out what makes the test fail in JSC. You now take globalState() from the arguments which retrieves it from its field of type ScriptStateProtectedPtr. ScriptStateProtectedPtr always returns pointer to the global ExecState (see http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/ScriptState.cpp#L55) while here we need current ExecState to collect the stack trace. Huh. Ok. That's not at all what I expected it to do. I could store the actual script state on the ScriptArguments object and create a 'currentState()' method. Would that be a bad idea for some reason?

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 172756 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172756&action=review > > > > > Source/WebCore/page/Console.cpp:174 > > > + RefPtr<ScriptCallStack> callStack(createScriptCallStackForConsole(arguments->globalState())); > > > > I guess I figured out what makes the test fail in JSC. You now take globalState() from the arguments which retrieves it from its field of type ScriptStateProtectedPtr. ScriptStateProtectedPtr always returns pointer to the global ExecState (see http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/ScriptState.cpp#L55) while here we need current ExecState to collect the stack trace. > > Huh. Ok. That's not at all what I expected it to do. > > I could store the actual script state on the ScriptArguments object and create a 'currentState()' method. Would that be a bad idea for some reason? Or store the current state on the protected pointer and create the method there. Would that be better?

(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 172756 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=172756&action=review > > > > > Source/WebCore/page/Console.cpp:174 > > > + RefPtr<ScriptCallStack> callStack(createScriptCallStackForConsole(arguments->globalState())); > > > > I guess I figured out what makes the test fail in JSC. You now take globalState() from the arguments which retrieves it from its field of type ScriptStateProtectedPtr. ScriptStateProtectedPtr always returns pointer to the global ExecState (see http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/js/ScriptState.cpp#L55) while here we need current ExecState to collect the stack trace. > > Huh. Ok. That's not at all what I expected it to do. > > I could store the actual script state on the ScriptArguments object and create a 'currentState()' method. Would that be a bad idea for some reason? ExecState is a kind of current call frame so storing it in ScriptArguments is not a good idea in general as it will become invalid once you leave current JS call frame. You should probably pass it as a parameter to the console.

Created attachment 173017 [details] Patch

(In reply to comment #13) > ExecState is a kind of current call frame so storing it in ScriptArguments is not a good idea in general as it will become invalid once you leave current JS call frame. You should probably pass it as a parameter to the console. Yury clarified this for me outside this bug. The original plan of simplifying the interface at the same time we moved the stacktrace generation isn't going to work. :( It's still worthwhile to move the stacktraces out, as this starts us down the road of only generating traces inside Inspector, but it's not as pretty as I'd like. :( WDYT, Yury, Pavel?

Created attachment 173028 [details] Rebased.

Comment on attachment 173028 [details] Rebased. Attachment 173028 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14763714

Comment on attachment 173028 [details] Rebased. View in context: https://bugs.webkit.org/attachment.cgi?id=173028&action=review > Source/WebCore/bindings/v8/ScriptCallStackFactory.cpp:124 > +PassRefPtr<ScriptCallStack> createScriptCallStack(ScriptState* /* state */, size_t maxStackSize) Remove commented code.

Created attachment 174393 [details] Patch for landing.

Comment on attachment 174393 [details] Patch for landing. Attachment 174393 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/14845556

Comment on attachment 174393 [details] Patch for landing. View in context: https://bugs.webkit.org/attachment.cgi?id=174393&action=review > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:225 > + $functionName ne "webkit_dom_console_time_end" && Rather than here, skip the timeEnd function by checking for the name in the signature (plenty of examples below this code).

Created attachment 174573 [details] GTK.

(In reply to comment #21) > (From update of attachment 174393 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=174393&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:225 > > + $functionName ne "webkit_dom_console_time_end" && > > Rather than here, skip the timeEnd function by checking for the name in the signature (plenty of examples below this code). Thank you, that worked brilliantly. :)

Comment on attachment 174573 [details] GTK. Attachment 174573 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14848613 New failing tests: platform/chromium/virtual/threaded/compositing/webgl/webgl-background-color.html

Comment on attachment 174573 [details] GTK. Failure looks clearly unrelated. Trying again. :)

Comment on attachment 174573 [details] GTK. Clearing flags on attachment: 174573 Committed r134931: <http://trac.webkit.org/changeset/134931>

All reviewed patches have been landed. Closing bug.