Bug 196020 - Add UI process WebRTC runtime logging.
Summary: Add UI process WebRTC runtime logging.
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-20 11:41 PDT by Eric Carlson
Modified: 2019-03-21 14:15 PDT (History)
5 users (show)

See Also:


Attachments
Patch (47.34 KB, patch)
2019-03-20 12:07 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (47.17 KB, patch)
2019-03-20 12:21 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (46.99 KB, patch)
2019-03-20 13:18 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (47.24 KB, patch)
2019-03-20 14:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (47.25 KB, patch)
2019-03-20 15:16 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch (47.18 KB, patch)
2019-03-20 15:49 PDT, Eric Carlson
youennf: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.44 MB, application/zip)
2019-03-20 17:05 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (2.30 MB, application/zip)
2019-03-20 18:34 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.91 MB, application/zip)
2019-03-20 19:38 PDT, Build Bot
no flags Details
Patch for landing (50.07 KB, patch)
2019-03-21 11:13 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Patch for landing (50.09 KB, patch)
2019-03-21 11:54 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2019-03-20 11:41:45 PDT
Add UI process WebRTC runtime logging.
Comment 1 Radar WebKit Bug Importer 2019-03-20 11:42:05 PDT
<rdar://problem/49071443>
Comment 2 Eric Carlson 2019-03-20 12:07:52 PDT
Created attachment 365381 [details]
Patch
Comment 3 Eric Carlson 2019-03-20 12:21:24 PDT
Created attachment 365383 [details]
Patch
Comment 4 Build Bot 2019-03-20 12:22:59 PDT
Attachment 365383 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698:  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: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 youenn fablet 2019-03-20 12:27:15 PDT
Comment on attachment 365383 [details]
Patch

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

> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:48
> +    , m_inspectedPage(context.inspectedPage)

Are we sure context.inspectedPage stays valid for the whole life of PageConsoleAgent?

> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:59
> +void PageConsoleAgent::getLoggingChannels(ErrorString&, RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>& channels)

This routine is similar to WebConsoleAgent::getLoggingChannels.
Could it be shared?

Since we always overwrite channels, should we change it to return a RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>?
It is a virtual method though, so probably needs more refactoring.

> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:108
> +        level = WTFLogLevel::Error;

maybe rewrite to return early, like { { WTFLogChannelState::Off, WTFLogLevel::Error } };

> Source/WebKit/UIProcess/WebPageProxy.cpp:8949
> +        m_logger->setEnabled(this, isAlwaysOnLoggingAllowed());

What happens if the web process is crashing.
We might still be logging but the web process might think we are not logging.
Or WebPageProxy might be attached to a different process.
We may need to sync the logging somehow, or maybe it happens automatically by the web inspector?
Should we set m_logger to nullptr in resetState?

The other thing is when page is navigating cross origin.
In that case WebPageProxy will be attached to a different process with potentially different logging values.
Comment 6 Eric Carlson 2019-03-20 13:16:57 PDT
Comment on attachment 365383 [details]
Patch

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

>> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:48
>> +    , m_inspectedPage(context.inspectedPage)
> 
> Are we sure context.inspectedPage stays valid for the whole life of PageConsoleAgent?

Yes, Joe Peck suggested this approach.

>> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:59
>> +void PageConsoleAgent::getLoggingChannels(ErrorString&, RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>& channels)
> 
> This routine is similar to WebConsoleAgent::getLoggingChannels.
> Could it be shared?
> 
> Since we always overwrite channels, should we change it to return a RefPtr<JSON::ArrayOf<Inspector::Protocol::Console::Channel>>?
> It is a virtual method though, so probably needs more refactoring.

All of the logging code was removed from WebConsoleAgent

>> Source/WebCore/inspector/agents/page/PageConsoleAgent.cpp:108
>> +        level = WTFLogLevel::Error;
> 
> maybe rewrite to return early, like { { WTFLogChannelState::Off, WTFLogLevel::Error } };

Sure.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8949
>>  #undef MERGE_WHEEL_EVENTS
> 
> What happens if the web process is crashing.
> We might still be logging but the web process might think we are not logging.
> Or WebPageProxy might be attached to a different process.
> We may need to sync the logging somehow, or maybe it happens automatically by the web inspector?
> Should we set m_logger to nullptr in resetState?
> 
> The other thing is when page is navigating cross origin.
> In that case WebPageProxy will be attached to a different process with potentially different logging values.

As we discussed, lets leave this as is for now and revisit it later.
Comment 7 Eric Carlson 2019-03-20 13:18:15 PDT
Created attachment 365386 [details]
Patch
Comment 8 Build Bot 2019-03-20 13:21:40 PDT
Attachment 365386 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698:  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: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Eric Carlson 2019-03-20 14:24:38 PDT
Created attachment 365400 [details]
Patch
Comment 10 Build Bot 2019-03-20 14:29:09 PDT
Attachment 365400 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698:  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: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Eric Carlson 2019-03-20 15:16:01 PDT
Created attachment 365410 [details]
Patch
Comment 12 Build Bot 2019-03-20 15:18:19 PDT
Attachment 365410 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698:  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: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Eric Carlson 2019-03-20 15:49:16 PDT
Created attachment 365418 [details]
Patch
Comment 14 Build Bot 2019-03-20 15:52:08 PDT
Attachment 365418 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:698:  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: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 youenn fablet 2019-03-20 16:07:28 PDT
Comment on attachment 365418 [details]
Patch

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

> Source/WebCore/inspector/agents/page/PageConsoleAgent.h:46
>      virtual ~PageConsoleAgent() = default;

This default destructor is probably not needed.

> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:89
> +    const Logger& logger() const final { return m_logger.get(); }

Could be implemented as m_page.logger();
We could then remove m_logger and we would be sure to be in sync with whatever page logger is.

> Source/WebKit/UIProcess/WebPageProxy.cpp:8931
> +        m_logger->setEnabled(this, isAlwaysOnLoggingAllowed());

When we are changing of WebPageProxy::m_websiteDataStore, we might change of isAlwaysOnLoggingAllowed() return value.
Maybe logger should be updated accordingly at that time.
Comment 16 Eric Carlson 2019-03-20 16:34:15 PDT
Comment on attachment 365418 [details]
Patch

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

>> Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.h:89
>> +    const Logger& logger() const final { return m_logger.get(); }
> 
> Could be implemented as m_page.logger();
> We could then remove m_logger and we would be sure to be in sync with whatever page logger is.

Good idea, changed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:8931
>> +        m_logger->setEnabled(this, isAlwaysOnLoggingAllowed());
> 
> When we are changing of WebPageProxy::m_websiteDataStore, we might change of isAlwaysOnLoggingAllowed() return value.
> Maybe logger should be updated accordingly at that time.

Yes, changed.
Comment 17 Build Bot 2019-03-20 17:05:48 PDT
Comment on attachment 365418 [details]
Patch

Attachment 365418 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11587341

New failing tests:
inspector/console/webcore-logging.html
Comment 18 Build Bot 2019-03-20 17:05:50 PDT
Created attachment 365442 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 19 Build Bot 2019-03-20 18:34:28 PDT
Comment on attachment 365418 [details]
Patch

Attachment 365418 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11588233

New failing tests:
inspector/console/webcore-logging.html
Comment 20 Build Bot 2019-03-20 18:34:30 PDT
Created attachment 365458 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 21 Build Bot 2019-03-20 19:38:20 PDT
Comment on attachment 365418 [details]
Patch

Attachment 365418 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11589767

New failing tests:
inspector/console/webcore-logging.html
Comment 22 Build Bot 2019-03-20 19:38:22 PDT
Created attachment 365464 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 23 Eric Carlson 2019-03-21 11:13:44 PDT
Created attachment 365572 [details]
Patch for landing
Comment 24 Eric Carlson 2019-03-21 11:54:48 PDT
Created attachment 365581 [details]
Patch for landing
Comment 25 Build Bot 2019-03-21 11:58:08 PDT
Attachment 365581 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestProxy.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/page/ChromeClient.h:501:  Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line.  [build/webcore_export] [4]
ERROR: Source/WebKit/UIProcess/UserMediaPermissionRequestManagerProxy.cpp:703:  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: 3 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 26 WebKit Commit Bot 2019-03-21 14:15:35 PDT
Comment on attachment 365581 [details]
Patch for landing

Clearing flags on attachment: 365581

Committed r243328: <https://trac.webkit.org/changeset/243328>