track enabled debugger domains for safe simultaneous use #1474

brendankenny · 2017-01-14T23:02:44Z

gatherRunner, driver, and gatherers enable and disable domains at will, and we currently have no system for tracking the state of them so that they don't interfere with each other. This has happened a few times in the past and it's difficult to track down why e.g. an event never fires because someone else disabled the domain you were listening to. Most recently this came up in #1421 where the styles and css-usage gatherers interfere when cleaning up after themselves.

This PR tracks enable and disable commands sent through the driver, eliminates redundant calls and prevents disabling by one user of the domain if others have also enabled it but have not yet disabled it.

The second commit is removing some of the places I've remembered us guarding against ourselves which is less necessary after this change.

brendankenny · 2017-01-14T23:07:52Z

There are still a few gotchas that we're living with that this doesn't solve. A big one is for domains where we listen to an event and then enable the domain. We rely on the protocol to have buffered all previous events and to replay them all once the domain is enabled. We use this in quite a few places (getSecurityState, getServiceWorkerVersions, etc). If some other gatherer has already enabled that domain then all previous events have already replayed and the next one won't receive them. We don't guard against that now. I added a driver.domainEnabled(domain) method so we can at least check if a domain is already enabled, something we couldn't do before, but that still relies on us being disciplined in interactions between gatherers.

This also doesn't check who is doing the enabling and disabling, so in theory gatherer2 could close the domain gatherer1 just opened and still cause an interaction bug. Tokens for enable/disable didn't really seem worth it, but I'm open to it.

I'm also thinking in a followup PR we could add a "clear domain state" step between passes in gatherRunner, with a suitable log.warn/log.error for any domains accidentally left open. We haven't always been strict about this except for the serious-overhead domains, but we probably should be.

brendankenny · 2017-01-14T23:09:29Z

lighthouse-core/gather/connections/connection.js

@@ -100,7 +100,8 @@ class Connection {
      // handleRawError returns or throws synchronously; wrap to put into promise chain.
      return callback.resolve(Promise.resolve().then(_ => {
        if (object.error) {
-          return this.handleRawError(object.error, callback.method);
+          log.formatProtocol('method <= browser ERR', {method: callback.method}, 'error');
+          throw new Error(`Protocol error (${callback.method}): ${object.error.message}`);


moving this back is basically undoing #861 and #895. DOM.disable returns an error if DOM is already disabled (see details in #861), but with this change the disable command is never sent if it's already disabled, so we never get the error in the first place.

Am I right that some domains throw and error if you call disable twice and some do not? Might be worth bringing up with the DT times. That seems inconsistent at best.

definitely inconsistent. From #861 is this cs link, currently showing only DOM complains if you disable when already disabled: https://cs.chromium.org/search/?q=if.*!enabled+file:%5Esrc/third_party/WebKit/Source/core/inspector/+case:yes&sq=package:chromium&type=cs

@patrickhulce @paulirish if they want to pursue a debugger protocol bug :)

brendankenny · 2017-01-14T23:13:24Z

lighthouse-core/gather/driver.js

-      // Enable Page domain to wait for Page.loadEventFired
-      .then(_ => this.sendCommand('Page.enable'))
+    // Check any domains that could interfere with or add overhead to the trace.
+    if (this.domainEnabled('Debugger')) {


this moves these to just being a log error message. Disabling these domains after beforePass has executed if a trace is being taken means that e.g. if someone adds a DBW gatherer that needs a trace then any DBW gatherer that enabled Debugger, CSS, or DOM in beforePass will be broken, possibly fairly subtly. Better to warn on these...or even throw an Error?

the log error message will propagate all the way into the report right? If it were an error, the DBW audit/gatherer would surface a -1 in the report?

If we're no longer disabling these before a trace, won't that change perf results?

If we're no longer disabling these before a trace, won't that change perf results?

In the current config these domains aren't enabled when we take a trace, so there's no change currently. If we add a gatherer that does enable one of these domains before a trace, though, it could cause a difference, which is why I kept a warning message. Maybe that's not enough, though.

the log error message will propagate all the way into the report right? If it were an error, the DBW audit/gatherer would surface a -1 in the report?

log.error only logs it in red to the CLI/console, which hopefully someone catches :)

The problem with the approach on master is that it introduces errors but in the opposite way: it undercuts any gatherers that may unknowingly be running in the same pass as a trace being taken. The result may be subtle and not an error. e.g. a gather is listening for a CSS event that never comes because driver has disabled the CSS domain out from under it, but the lack of an event may just mean whatever it was gathering didn't happen on this particular page.

Maybe as suggested we do want to assert the domains are disabled and throw if they aren't, with a nice error message saying that the current config has gatherers interfering with a trace

I'm specifically thinking of CRX users who wouldn't see cli/console logs. But if the error surfaces up at the end (or as a failure during the CRX run), then it's all good.

assert the domains are disabled and throw if they aren't

Yea. That would be a good way for use to verify no one tries do so anything crazy during a trace in the future.

turned into thrown errors

brendankenny · 2017-01-14T23:14:05Z

lighthouse-core/gather/driver.js

@@ -530,9 +588,6 @@ class Driver {

  _readTraceFromStream(streamHandle) {
    return new Promise((resolve, reject) => {
-      // COMPAT: We've found `result` not retaining its value in this scenario when it's


obsolete comment

brendankenny · 2017-01-14T23:16:00Z

lighthouse-core/gather/gatherers/styles.js

-        // return driver.sendCommand('CSS.disable')
-        //   .then(_ => driver.sendCommand('DOM.disable'))
-        //   .then(_ => resolve(styleHeaders));
+


commented out in #1421. Verified that bringing this back without tracking domains failed the DBW smoke test (in the unused-css-rules audit), but with this change it now passes correctly

patrickhulce · 2017-01-17T17:25:41Z

lighthouse-core/gather/driver.js

+   * @param {string} domain
+   * @return {boolean}
+   */
+  domainEnabled(domain) {


just curious here, what are your thoughts in general on isX prefixing for all boolean values/functions?

I would be in favor of us "standardizing" on a convention. Been wrestling with this myself.

sometimes I find it redundant but in this case I think it helps clarity. done

patrickhulce

interesting route! what do you think about having gatherers declare the domains they use as an approach to solve both this and the still lingering buffered events issue?

patrickhulce · 2017-01-17T17:31:22Z

lighthouse-core/gather/driver.js

+   * @return {boolean}
+   * @private
+   */
+  _checkDomainToggle(domain, action) {


maybe something slightly more explicit for the name like _shouldSendDomainCommand or _shouldToggleDomain?

sounds good

brendankenny · 2017-01-17T19:37:52Z

interesting route! what do you think about having gatherers declare the domains they use as an approach to solve both this and the still lingering buffered events issue?

It seems the main problem with this approach is how you define a failure case. e.g. it's fine for the styles and css-usage gatherers to both enable the CSS domain simultaneously, and in fact they should each enable it, as they can't depend on being run in the same pass for all time.

Taking a different tack, there's not really a way to limit the domain declarations to gatherers who need to say "I'm going to enable this domain and it better be disabled when I enable it", because some other gatherer might need that domain but not need the events from it, so doesn't need that guarantee.

It really seems like manually checking that a domain is disabled when you need that behavior, and throwing an unrecoverable error otherwise, is the best way to catch this, but also the least automatically testable. I guess we could wrap that up for folks, driver.assertDomainDisabled('Security') or whatever, though not sure how helpful that really is vs them just doing the conditional throw themselves

patrickhulce · 2017-01-17T20:04:25Z

It really seems like manually checking that a domain is disabled when you need that behavior, and throwing an unrecoverable error otherwise, is the best way to catch this, but also the least automatically testable. I guess we could wrap that up for folks, driver.assertDomainDisabled('Security')

I like that. In cases where they care the gatherer can assert and flip on manually, but in cases where they don't need it to be disabled at start it can be just declared as needed.

ebidel · 2017-01-17T20:14:24Z

lighthouse-core/gather/driver.js

+   * @private
+   */
+  _checkDomainToggle(domain, action) {
+    const enable = action === 'enable';


should action just be a boolean instead of passing string around?

it's slightly more awkward but probably better. Done.

ebidel · 2017-01-17T20:18:01Z

lighthouse-core/gather/driver.js

+   * @param {string} domain
+   * @return {boolean}
+   */
+  domainEnabled(domain) {


I would be in favor of us "standardizing" on a convention. Been wrestling with this myself.

ebidel · 2017-01-17T20:37:13Z

lighthouse-core/gather/driver.js

-      // Enable Page domain to wait for Page.loadEventFired
-      .then(_ => this.sendCommand('Page.enable'))
+    // Check any domains that could interfere with or add overhead to the trace.
+    if (this.domainEnabled('Debugger')) {


the log error message will propagate all the way into the report right? If it were an error, the DBW audit/gatherer would surface a -1 in the report?

ebidel · 2017-01-17T20:40:06Z

lighthouse-core/gather/driver.js

-      // Enable Page domain to wait for Page.loadEventFired
-      .then(_ => this.sendCommand('Page.enable'))
+    // Check any domains that could interfere with or add overhead to the trace.
+    if (this.domainEnabled('Debugger')) {


If we're no longer disabling these before a trace, won't that change perf results?

brendankenny · 2017-01-17T23:41:44Z

updated. PTAL

brendankenny commented Jan 14, 2017

View reviewed changes

patrickhulce reviewed Jan 17, 2017

View reviewed changes

ebidel reviewed Jan 17, 2017

View reviewed changes

track enabled debugger domains for safe simultaneous use

2988a55

brendankenny force-pushed the domains branch from 66f5163 to 36aa9ad Compare January 17, 2017 21:32

ebidel added report and removed report labels Jan 17, 2017

remove workarounds for lack of domain state tracking

2cb1636

brendankenny force-pushed the domains branch from 36aa9ad to 2cb1636 Compare January 17, 2017 23:41

ebidel approved these changes Jan 18, 2017

View reviewed changes

brendankenny merged commit 986f4b5 into master Jan 19, 2017

brendankenny deleted the domains branch January 19, 2017 00:58

This was referenced Feb 11, 2017

Extension Error: this.handleRawError is not a function #1696

Closed

remove handleRawError call from extension connection #1697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

track enabled debugger domains for safe simultaneous use #1474

track enabled debugger domains for safe simultaneous use #1474

brendankenny commented Jan 14, 2017 •

edited

Loading

brendankenny commented Jan 14, 2017

brendankenny Jan 14, 2017 •

edited

Loading

ebidel Jan 17, 2017

brendankenny Jan 17, 2017

brendankenny Jan 14, 2017 •

edited

Loading

ebidel Jan 17, 2017

ebidel Jan 17, 2017

brendankenny Jan 17, 2017

ebidel Jan 17, 2017

brendankenny Jan 17, 2017

brendankenny Jan 14, 2017

brendankenny Jan 14, 2017

patrickhulce Jan 17, 2017

ebidel Jan 17, 2017

brendankenny Jan 17, 2017

patrickhulce left a comment

patrickhulce Jan 17, 2017

brendankenny Jan 17, 2017

brendankenny commented Jan 17, 2017

patrickhulce commented Jan 17, 2017

ebidel Jan 17, 2017

brendankenny Jan 17, 2017

ebidel Jan 17, 2017

ebidel Jan 17, 2017

ebidel Jan 17, 2017

brendankenny commented Jan 17, 2017

track enabled debugger domains for safe simultaneous use #1474

track enabled debugger domains for safe simultaneous use #1474

Conversation

brendankenny commented Jan 14, 2017 • edited Loading

brendankenny commented Jan 14, 2017

brendankenny Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Jan 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 17, 2017

patrickhulce commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jan 17, 2017

brendankenny commented Jan 14, 2017 •

edited

Loading

brendankenny Jan 14, 2017 •

edited

Loading

brendankenny Jan 14, 2017 •

edited

Loading