Bug 118328

Summary: Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary
Product: WebKit Reporter: Chris Curtis <chris_curtis>
Component: New BugsAssignee: Chris Curtis <chris_curtis>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eflews.bot, gyuyoung.kim, rniwa, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ggaren: review+, commit-queue: commit-queue-
Archive of layout-test-results from webkit-cq-01 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Fixed the Errors in the previous patch
none
Fixed the styling errors of new patch.
msaboff: review+
removed extra line
ggaren: commit-queue-
Corrected and added info to the change log.
ggaren: review-
patch
ggaren: review-, ggaren: commit-queue-
patch2
eflews.bot: commit-queue-
patch 3 none

Description Chris Curtis 2013-07-02 15:28:15 PDT
Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary
Comment 1 Chris Curtis 2013-07-02 15:40:19 PDT
Created attachment 205950 [details]
Patch
Comment 2 Geoffrey Garen 2013-07-02 15:50:58 PDT
Comment on attachment 205950 [details]
Patch

r=me

Achievement unlocked: first WebKit patch.
Comment 3 WebKit Commit Bot 2013-07-02 16:46:20 PDT
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
Comment 4 WebKit Commit Bot 2013-07-02 16:46:22 PDT
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
Comment 5 Build Bot 2013-07-02 17:12:14 PDT
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
Comment 6 Build Bot 2013-07-02 17:12:16 PDT
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
Comment 7 Build Bot 2013-07-02 18:20:51 PDT
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
Comment 8 Build Bot 2013-07-02 18:20:52 PDT
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
Comment 9 Chris Curtis 2013-07-08 14:31:04 PDT
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.
Comment 10 WebKit Commit Bot 2013-07-08 14:34:04 PDT
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.
Comment 11 Chris Curtis 2013-07-08 14:39:33 PDT
Created attachment 206269 [details]
Fixed the styling errors of new patch.
Comment 12 Michael Saboff 2013-07-08 15:05:41 PDT
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.
Comment 13 Chris Curtis 2013-07-08 16:05:12 PDT
Created attachment 206279 [details]
removed extra line
Comment 14 Geoffrey Garen 2013-07-08 16:51:11 PDT
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.
Comment 15 Chris Curtis 2013-07-08 17:36:00 PDT
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.
Comment 16 Geoffrey Garen 2013-07-08 20:25:43 PDT
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.
Comment 17 Chris Curtis 2013-07-09 10:40:55 PDT
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.
Comment 18 Chris Curtis 2013-07-09 17:24:57 PDT
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.
Comment 19 Chris Curtis 2013-07-12 10:35:21 PDT
Created attachment 206553 [details]
patch
Comment 20 Geoffrey Garen 2013-07-12 11:53:14 PDT
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;
Comment 21 Chris Curtis 2013-07-12 13:09:00 PDT
Created attachment 206568 [details]
patch2
Comment 22 EFL EWS Bot 2013-07-12 13:12:22 PDT
Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/962431
Comment 23 Early Warning System Bot 2013-07-12 13:14:18 PDT
Comment on attachment 206568 [details]
patch2

Attachment 206568 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1062269
Comment 24 Early Warning System Bot 2013-07-12 13:15:05 PDT
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
Comment 25 EFL EWS Bot 2013-07-12 13:15:16 PDT
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
Comment 26 Chris Curtis 2013-07-12 14:23:14 PDT
Created attachment 206571 [details]
patch 3
Comment 27 Geoffrey Garen 2013-07-12 14:32:04 PDT
Comment on attachment 206571 [details]
patch 3

r=me
Comment 28 WebKit Commit Bot 2013-07-12 15:38:50 PDT
Comment on attachment 206571 [details]
patch 3

Clearing flags on attachment: 206571

Committed r152606: <http://trac.webkit.org/changeset/152606>
Comment 29 WebKit Commit Bot 2013-07-12 15:38:54 PDT
All reviewed patches have been landed.  Closing bug.