Bug 140478 - Web Inspector and regular console use different source code locations for messages
Summary: Web Inspector and regular console use different source code locations for mes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on: 140608
Blocks: 140508
  Show dependency treegraph
 
Reported: 2015-01-14 16:26 PST by Alexey Proskuryakov
Modified: 2015-01-19 01:19 PST (History)
6 users (show)

See Also:


Attachments
proposed patch (699.65 KB, patch)
2015-01-14 16:38 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (682.28 KB, patch)
2015-01-14 16:42 PST, Alexey Proskuryakov
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (524.84 KB, application/zip)
2015-01-14 18:03 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (948.54 KB, application/zip)
2015-01-14 19:08 PST, Build Bot
no flags Details
proposed patch (698.72 KB, patch)
2015-01-15 10:46 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (681.36 KB, patch)
2015-01-15 11:03 PST, Alexey Proskuryakov
burg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-01-14 16:26:42 PST
Web Inspector and console client calculate source code locations in entirely different ways. This doesn't make sense to me, seems like a bug.
Comment 1 Alexey Proskuryakov 2015-01-14 16:38:49 PST
Created attachment 244660 [details]
proposed patch
Comment 2 WebKit Commit Bot 2015-01-14 16:40:17 PST
Attachment 244660 [details] did not pass style-queue:


ERROR: Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WTF/wtf/Vector.h:510:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 279 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2015-01-14 16:42:12 PST
Created attachment 244661 [details]
proposed patch
Comment 4 Brian Burg 2015-01-14 17:20:55 PST
Comment on attachment 244661 [details]
proposed patch

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

Overall this is great!

Keep in mind that the inspector's console UI will change displayed line numbers based on source maps, pretty-printed code, etc. So it may not match, but the raw unformatted source location information is definitely the same now.

This does not seem to convert compute line numbers the old way where createCallStackForConsole is used. This includes the code path that prints to stdout based on WebCoreLogging prefs. It would be great to do that, but it could probably be a different bug.

Is this likely to require rebaselining for other ports test results? I'm not sure that can be avoided, but could be painful.

> Source/JavaScriptCore/ChangeLog:3
> +        Web Inspector and regular console show different source code locations for messages

I would change 'show' to 'use', for reasons in the main comment.

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.h:-74
> -    void addMessageToConsole(MessageSource, MessageType, MessageLevel, const String& message, JSC::ExecState*, PassRefPtr<ScriptArguments>, unsigned long requestIdentifier = 0);

This is much nicer.

> Source/WebCore/page/PageConsoleClient.cpp:153
> +    std::unique_ptr<Inspector::ConsoleMessage> message = std::make_unique<Inspector::ConsoleMessage>(MessageSource::ConsoleAPI, type, level, messageText, WTF::move(arguments), exec);

Nit: auto would be fine here.

> Source/WebCore/page/PageConsoleClient.cpp:167
> +    // FIXME: This doesn't work, we already moved out of arguments variable (regressed in <http://trac.webkit.org/changeset/178060>).

Could you address this by changing the moves to copyRefs?

> LayoutTests/http/tests/media/media-source/mediasource-remove-expected.txt:8
> +FAIL Test remove transitioning readyState from 'ended' to 'open'. InvalidStateError: DOM Exception 11(stack: remove@[native code]

Did you mean to include this result regression?
Comment 5 Build Bot 2015-01-14 18:03:46 PST
Comment on attachment 244661 [details]
proposed patch

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

New failing tests:
http/tests/security/xss-DENIED-iframe-src-alias.html
http/tests/security/drag-drop-local-file.html
Comment 6 Build Bot 2015-01-14 18:03:50 PST
Created attachment 244671 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5
Comment 7 Build Bot 2015-01-14 19:08:47 PST
Comment on attachment 244661 [details]
proposed patch

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

New failing tests:
http/tests/security/xss-DENIED-iframe-src-alias.html
Comment 8 Build Bot 2015-01-14 19:08:58 PST
Created attachment 244674 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 9 Alexey Proskuryakov 2015-01-14 21:50:21 PST
Thank you for the great feedback, I'm going to address is alongside test failures.

> This does not seem to convert compute line numbers the old way where createCallStackForConsole is used. This includes the code path that prints to stdout based on WebCoreLogging prefs. It would be great to do that, but it could probably be a different bug.

Could you please elaborate?

The chunk of code that I removed that used createScriptCallStackForConsole is actually similar to what a ConsoleMessage constructor does, with a few (presumably unintentional) differences. 

> Could you address this by changing the moves to copyRefs?

The right thing to do is to fix that in a separate patch, especially since fixing this one can take some time.
Comment 10 Brian Burg 2015-01-14 22:29:41 PST
(In reply to comment #9)
> Thank you for the great feedback, I'm going to address is alongside test
> failures.
> 
> > This does not seem to convert compute line numbers the old way where createCallStackForConsole is used. This includes the code path that prints to stdout based on WebCoreLogging prefs. It would be great to do that, but it could probably be a different bug.
> 
> Could you please elaborate?
> 
> The chunk of code that I removed that used createScriptCallStackForConsole
> is actually similar to what a ConsoleMessage constructor does, with a few
> (presumably unintentional) differences. 

I was trying to figure out the subtle differences between ScriptCallStackFactory methods. The ForConsole version skips the first (hopefully native) call frame. It seems that there is no reason to use createScriptCallStackForConsole() anywhere that feeds into ConsoleMessage since it knows to start from the first non-native call frame. I was confused by the fact that the system console logging code path uses the ForConsole logic instead of ConsoleMessage.

Another cleanup would be to delete createScriptCallStackForConsole() entirely, along with the various subtle ConsoleMessage constructors, in favor of an optional flag. But it's not important for this patch as long as there's not a regression. (In fact, complexity has been reduced quite a bit with this patch.)

> > Could you address this by changing the moves to copyRefs?
> 
> The right thing to do is to fix that in a separate patch, especially since
> fixing this one can take some time.

Oh, right, it's a big patch with the tests. Will fix.
Comment 11 Alexey Proskuryakov 2015-01-15 09:55:25 PST
> Is this likely to require rebaselining for other ports test results?

Not likely - this patch doesn't change any Mac results, so chances are that it only affects cross-platform results.
Comment 12 Alexey Proskuryakov 2015-01-15 10:46:07 PST
Created attachment 244700 [details]
proposed patch

> The ForConsole version skips the first (hopefully native) call frame. 

By the way, a lot of these line numbers are wrong indeed. For example, we erroneously skip the first frame on some subtests of http/tests/security/xss-DENIED-iframe-src-alias.html, but also a number of other results I skimmed over were similarly wrong.

This patch will make it easier to test future fixes in this area.
Comment 13 WebKit Commit Bot 2015-01-15 10:47:25 PST
Attachment 244700 [details] did not pass style-queue:


ERROR: Source/WTF/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WTF/wtf/Vector.h:510:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 2 in 280 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Alexey Proskuryakov 2015-01-15 11:03:15 PST
Created attachment 244702 [details]
proposed patch
Comment 15 Brian Burg 2015-01-15 13:24:11 PST
Comment on attachment 244702 [details]
proposed patch

r=me

Looks like test results have been corrected.
Comment 16 Alexey Proskuryakov 2015-01-15 13:52:56 PST
Committed <http://trac.webkit.org/r178527>.
Comment 17 Csaba Osztrogonác 2015-01-19 01:19:27 PST
(In reply to comment #16)
> Committed <http://trac.webkit.org/r178527>.

It broke the !INSPECTOR build - new bug report to fix this regression: bug140608