|Summary:||Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary|
|Product:||WebKit||Reporter:||Chris Curtis <chris_curtis>|
|Component:||New Bugs||Assignee:||Chris Curtis <chris_curtis>|
|Severity:||Normal||CC:||buildbot, commit-queue, eflews.bot, gyuyoung.kim, rniwa, webkit-ews|
|Version:||528+ (Nightly build)|
Description Chris Curtis 2013-07-02 15:28:15 PDT
Optimize addStrackTraceIfNecessary to be faster in the case when it's not necessary
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
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 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 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 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 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.functionName == "decodeURI" || errorStackTrace.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.functionName != "decodeURI" && errorStackTrace.functionName != "eval") hadStackTraceDifference = true;
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 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.