Bug 118328 – Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Curtis]

Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary

[Chris Curtis]

Created attachment 205950 [details]
Patch

[Geoffrey Garen]

Comment on attachment 205950 [details]
Patch

r=me

Achievement unlocked: first WebKit patch.

[WebKit Commit Bot]

Comment on attachment 205950 [details]
Patch

Rejecting attachment 205950 [details] from commit-queue.

New failing tests:
fast/events/remove-target-with-shadow-in-drag.html
inspector/console/console-exception-stack-traces.html
Full output: http://webkit-queues.appspot.com/results/1016477

[WebKit Commit Bot]

Created attachment 205957 [details]
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205950 [details]
Patch

Attachment 205950 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1015568

New failing tests:
fast/events/remove-target-with-shadow-in-drag.html
inspector/console/console-exception-stack-traces.html

[Build Bot]

Created attachment 205958 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205950 [details]
Patch

Attachment 205950 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1014677

New failing tests:
inspector/console/console-exception-stack-traces.html

[Build Bot]

Created attachment 205960 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Chris Curtis]

Created attachment 206268 [details]
Fixed the Errors in the previous patch

Corrected patch. Modified a test case to match the functionality in Firefox, Chrome, and now Safari. Fixed two places that the topCallFrame was not being set correctly before a thrown error.

[WebKit Commit Bot]

Attachment 206268 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/events/remove-target-with-shadow-in-drag-expected.txt', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/interpreter/Interpreter.cpp', u'Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp']" exit_code: 1
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:118:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp:119:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/interpreter/Interpreter.cpp:153:  Missing space before {  [whitespace/braces] [5]
Source/JavaScriptCore/interpreter/Interpreter.cpp:154:  Missing space after ,  [whitespace/comma] [3]
Source/JavaScriptCore/interpreter/Interpreter.cpp:659:  Extra space after ( in if  [whitespace/parens] [5]
Source/JavaScriptCore/interpreter/Interpreter.cpp:671:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Curtis]

Created attachment 206269 [details]
Fixed the styling errors of new patch.

[Michael Saboff]

Comment on attachment 206269 [details]
Fixed the styling errors of new patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=206269&action=review

> Source/JavaScriptCore/interpreter/Interpreter.cpp:685
> +

Looks like there is an extra blank line here.  If so remove it.

[Chris Curtis]

Created attachment 206279 [details]
removed extra line

[Geoffrey Garen]

Comment on attachment 206279 [details]
removed extra line

View in context: https://bugs.webkit.org/attachment.cgi?id=206279&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        I didn't add a layout test, but modified LayoutTests/fast/events/remove-target-
> +        with-shadow-in-drag-expected.txt to match with expected results in both Firefox 

remove-target-with-shadow-in-drag-expected.txt doesn't seem to call eval() or decode(). Does it really cover those cases?

I don't understand why decode() wants to roll back the callee frame. Can you explain that?

> Source/JavaScriptCore/ChangeLog:11
> +        and Chrome and now safari.

"Safari" should be capitalized.

> Source/JavaScriptCore/ChangeLog:17
> +        one frame off of topCallFrame. Error wasn't observable before becasue the stack was

Typo: "becasue" should be "because".

> Source/JavaScriptCore/ChangeLog:18
> +         being overwritten the next time addStackIfNecessary was called.

Extra space at the beginning of the line.

[Chris Curtis]

Created attachment 206285 [details]
Corrected and added info to the change log.

@Geoff, The remove-target-with-shadow-in-drag-expected.txt change was a trivial line number change. LayoutTests/inspector/console/console-exception-stack-traces.html is the test case that will fail without the changes to eval and decode. To answer your question, callee does not want to roll back its call frame unless it is the error frame itself(which it is in this case). I roll back the topCallFrame right before the throw. After it returns, the ~TopCallFrameSetter is invoked, which does the same action but it is needed before the vm is deallocated. Directly after the callee's vm is deallocated, add error information retrieves the information which is why the stack trace was correct the second time around and the error was never found.

[Geoffrey Garen]

Comment on attachment 206285 [details]
Corrected and added info to the change log.

View in context: https://bugs.webkit.org/attachment.cgi?id=206285&action=review

> Source/JavaScriptCore/ChangeLog:15
> +        Changes to decode and eval come from LayoutTests/inspector/console/console-exception-stack-traces.html.

A little clearer to say "are tested by" instead of "come from".

> Source/JavaScriptCore/ChangeLog:23
> +        (JSC::Interpreter::eval): The current frame was the errored frame. This is popping

"errored frame" is a misleading phrase. The top frame is the frame we allocated for running eval code. There's nothing wrong with it. 

We need to roll back .topCallFrame because we set it before we fully committed to running the eval code, and now, late in the game, we've discovered that we aren't going to run the eval code. It would probably also work to delay setting .topCallFrame until we knew for sure we were going to run the code. (I'm not asking you to do this necessarily, only trying to illustrate the logic here.)

Please try to clarify this comment.

> Source/JavaScriptCore/ChangeLog:28
> +        * runtime/JSGlobalObjectFunctions.cpp:
> +        (JSC::runtime::decode): See above ^^

I'm still not clear about why you think an exception thrown from decodeURI() should behave the same way as a syntax error when trying to run eval code.

If f() calls decodeURI(), and decodeURI() throws a URI error, shouldn't the backtrace show f()->decodeURI()? In general, if f() calls any host function, which throws an error, shouldn't the backtrace always be f()->host()? Otherwise, how do I know that host() is the function that threw the error?

eval() is special because it's a keyword that invokes a program "as if" it were a part of the text of the calling function. decodeURI() is not special in this way.

[Chris Curtis]

For both cases, the frame that is being removed is the allocated memory that has not been fully built because there was the error thrown. For decode and eval at the time to creation, the stack trace is still pointing to the unfinished frame. The result is an added frame at the top that looks like decodeURI@[native code] and eval@[native code] with the rest of the stack matching the expected results from Safari, FireFox, and Chrome.

[Chris Curtis]

My previous knowledge was mistaken. I was assuming eval@[NativeCode] and decode@[NativeCode] for the the top of the call frame was incorrect because Chome and Firefox did not display them.

[Chris Curtis]

Created attachment 206553 [details]
patch

[Geoffrey Garen]

Comment on attachment 206553 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206553&action=review

> LayoutTests/ChangeLog:9
> +        These results also match Chrome and Firfox's resutls on this test.

Typo: resutls => results.

> LayoutTests/inspector/console/console-exception-stack-traces.html:101
> +                if (errorStackTrace.length - traceStackTrace.length == 1 && (errorStackTrace[0].functionName == "decodeURI" || errorStackTrace[0].functionName == "eval"))
> +                    ;
> +                else
> +                    hadStackTraceDifference = true;

Empty statements can be hard to read. You should reverse the direction of this branch to avoid an empty statement:

if (errorStackTrace.length - traceStackTrace.length != 1 || (errorStackTrace[0].functionName != "decodeURI" && errorStackTrace[0].functionName != "eval")
    hadStackTraceDifference = true;

[Chris Curtis]

Created attachment 206568 [details]
patch2

[EFL EWS Bot]

Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/962431

[Early Warning System Bot]

Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1062269

[Early Warning System Bot]

Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/1054400

[EFL EWS Bot]

Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/931868

[Chris Curtis]

Created attachment 206571 [details]
patch 3

[Geoffrey Garen]

Comment on attachment 206571 [details]
patch 3

r=me

[WebKit Commit Bot]

Comment on attachment 206571 [details]
patch 3

Clearing flags on attachment: 206571

Committed r152606: <http://trac.webkit.org/changeset/152606>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.