Bug 140478 – Web Inspector and regular console use different source code locations for messages

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Web Inspector and console client calculate source code locations in entirely different ways. This doesn't make sense to me, seems like a bug.

[Alexey Proskuryakov]

Created attachment 244660 [details]
proposed patch

[WebKit Commit Bot]

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.

[Alexey Proskuryakov]

Created attachment 244661 [details]
proposed patch

[Brian Burg]

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?

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

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.

[Brian Burg]

(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.

[Alexey Proskuryakov]

> 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.

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

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.

[Alexey Proskuryakov]

Created attachment 244702 [details]
proposed patch

[Brian Burg]

Comment on attachment 244702 [details]
proposed patch

r=me

Looks like test results have been corrected.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r178527>.

[Csaba Osztrogonác]

(In reply to comment #16)
> Committed <http://trac.webkit.org/r178527>.

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