CRX: disable extensions before running lighthouse #1492

ebidel · 2017-01-18T19:37:34Z

R: all

brendankenny

this one will be interesting, but could be a really good thing for the extension :)

brendankenny · 2017-01-20T00:38:55Z

lighthouse-extension/app/src/lighthouse-background.js

+  return new Promise(resolve => {
+    let remaining = installedExtensions.length;
+    installedExtensions.forEach(info => {
+      chrome.management.setEnabled(info.id, enable, _ => {


since the extension is running this after a button click, I assume this gets around the docs warning about "In most cases this function must be called in the context of a user gesture"?

Yea. Still falls within that click.

brendankenny · 2017-01-20T00:39:44Z

lighthouse-extension/app/src/lighthouse-background.js

-      updateBadgeUI();
-      throw err;
-    });
+  return enableOtherChromeExtensions(false)


needs to be in runLighthouseInExtension as devtools also runs runLighthouseForConnection (through runLighthouseInWorker).

(updateBadgeUI probably should be just in runLighthouseInExtension too. It gates itself internally by returning if no chrome.runtime, but it could quickly get confusing if we had a bunch of noop functions in the devtools path)

brendankenny · 2017-01-20T00:44:17Z

lighthouse-extension/app/src/lighthouse-background.js

+
+        // Deliver results even if enabling extensions fails.
+        enableOtherChromeExtensions(true).catch(err => {
+          log.warn('Chrome', `Could not disable some extensions. ${err.message}`);


disable -> enable? This seems like a big problem if it ever happens...worth bubbling up to the "Report Error" level?

Done. For the other, I don't think it's a good UX if LH fails because an extension fails to enable. That was my intention at least. I'm not sure how enabling could fail, but users can still get results if they do.

brendankenny · 2017-01-20T00:46:28Z

lighthouse-extension/app/src/lighthouse-background.js

+        updateBadgeUI();
+
+        enableOtherChromeExtensions(true).catch(err => {
+          log.warn('Chrome', `Could not enable/disable some extensions. ${err.message}`);


feels like we should track what state we weren't able to achieve (mostly in case we can't reenable extensions)? Would probably need a separate variable from lighthouseIsRunning to track that though

What are you thinking? And how would it surface to the user?

good question. I don't actually know a great way, but I'm concerned there will be some corner case of permissions or something where we end up silently disabling some people's extensions and we won't know about it until someone notices (or we get error reporting integrated)

It would be nice to know if/when the enabling/disabling can fail, but it may be risky to make that fatal. I really wish we had analytics :\

As part of your error refactoring, do you want to think about ways to surface top-level errors like this (e.g. non-gatherer errors) to the report so people can file bugs on them?

#941 is really only about errors in the gatherer->artifact->audit->report pipeline. @patrickhulce will have to figure this out to get error reporting working, though.

We could make a master bug, though

brendankenny · 2017-01-20T00:47:10Z

lighthouse-extension/app/src/lighthouse-background.js

@@ -277,6 +311,17 @@ window.isRunning = function() {
  return lighthouseIsRunning;
 };

+// Get list of installed extensions that are enabled and can be disabled.
+// Extensions are not allowed to be disabled if they are under an admin policy.
+chrome.management.getAll(installs => {


also needs to not run in devtools. Can just nest in the if statement below in this case.

brendankenny · 2017-01-20T00:52:22Z

lighthouse-extension/app/src/lighthouse-background.js

+function enableOtherChromeExtensions(enable) {
+  return new Promise(resolve => {
+    let remaining = installedExtensions.length;
+    installedExtensions.forEach(info => {


could also do

return Promise.all(installedExtensions.map(info => { return new Promise(resolve => { chrome.management.setEnabled(info.id, enable, resolve); }); })

brendankenny · 2017-01-20T00:56:24Z

lighthouse-extension/app/src/lighthouse-background.js

+ * @param {!Promise}
+ */
+function enableOtherChromeExtensions(enable) {
+  return new Promise(resolve => {


will you add a log.log or log.verbose about enabling or disabling extensions? Worth including installedExtensions.length in the log message?

brendankenny · 2017-01-20T00:58:09Z

how does the permission prompt look for management?

ebidel · 2017-01-20T23:22:15Z

The permission prompt looks like this:

ebidel · 2017-01-21T00:46:03Z

PTAL

brendankenny · 2017-01-24T00:59:13Z

lighthouse-extension/app/src/lighthouse-background.js

-      updateBadgeUI();
-      throw err;
-    });
+  return Runner.run(connection, runOptions) // Run Lighthouse.


can revert this block of changes?

It's just a proper 4-space indent. No code changes.

https://github.com/GoogleChrome/lighthouse/pull/1492/files?w=1

only two spaces for chaining though, just to keep everyone on their toes :)

(I think this is one place we diverge from Google style, but it's so common in open source JS that we've pretty much unquestioningly done it throughout LH)

We're not consistent, but I'll revert to be consistent with the file.

If we want to enforce something other than Goog style, there really should be an eslint rule.

brendankenny · 2017-01-24T01:03:20Z

lighthouse-extension/app/src/lighthouse-background.js

    .then(url => window.runLighthouseForConnection(connection, url, options, requestedAggregations))
    .then(results => {
+      // Deliver results even if enabling extensions fails.
+      enableOtherChromeExtensions(true).catch(err => {
+        log.warn('Chrome', `Could not enable some extensions. ${err.message}`);


should upgrade to log.error, but will enableOtherChromeExtensions ever actually reject? The docs mention that it could fail but don't mention how it will fail. Presumably it won't throw but will do the callback with an error or set lastError or something? We should hook that up to the reject callback

I think I remember it throwing, but will check. If not, we can monitor lastError.

looks like it does throw, but the error is from not checking lastError :) "Unchecked runtime.lastError while running management.setEnabled"

done. this is now in the promise flow.

brendankenny · 2017-01-24T01:04:18Z

lighthouse-extension/app/src/lighthouse-background.js

      const blobURL = window.createReportPageAsBlob(results, 'extension');
      chrome.tabs.create({url: blobURL});
+    }).catch(err => {
+      enableOtherChromeExtensions(true).catch(err => {
+        log.warn('Chrome', `Could not enable/disable some extensions. ${err.message}`);


log.error

ebidel · 2017-01-24T05:08:29Z

I've changed is so if an extension cannot be disabled, the chain rejects and we'll know about it via the extension bug report button. We should monitor this pretty closely on the next release. If there are errors coming in, we'll want to adjust.

ebidel · 2017-01-24T05:08:33Z

PTAL

brendankenny · 2017-01-24T23:50:50Z

lighthouse-extension/app/src/lighthouse-background.js

    .then(url => window.runLighthouseForConnection(connection, url, options, requestedAggregations))
    .then(results => {
+      enableOtherChromeExtensions(true);


these have to be returned for the rejection to get into the promise chain.

.then(results => { return enableOtherChromeExtensions(true).then(_ => { const blobURL = window.createReportPageAsBlob(results, 'extension'); chrome.tabs.create({url: blobURL}); }); }).catch(err => { return enableOtherChromeExtensions(true).then(_ => { throw err; }); });

maybe?

ebidel · 2017-01-25T00:16:43Z

PTAL

brendankenny

LGTM

ebidel force-pushed the crxdisable branch from f08a4a8 to 4f83315 Compare January 18, 2017 19:38

brendankenny requested changes Jan 20, 2017

View reviewed changes

ebidel mentioned this pull request Jan 23, 2017

Way to surface non-critical errors in the report #1512

Closed

brendankenny requested changes Jan 24, 2017

View reviewed changes

ebidel added 5 commits January 24, 2017 15:56

CRX: disable extensions before running lighthouse

e1e4b98

feedback

54088a1

use promise

a4052df

Put enable/disable extension in normal promise flow

701ecce

chain it up

77ad6ad

ebidel force-pushed the crxdisable branch from 138a3df to 77ad6ad Compare January 24, 2017 23:57

brendankenny approved these changes Jan 25, 2017

View reviewed changes

brendankenny merged commit 59581c7 into master Jan 25, 2017

brendankenny deleted the crxdisable branch January 25, 2017 00:32

CRX: disable extensions before running lighthouse #1492

CRX: disable extensions before running lighthouse #1492

Conversation

ebidel commented Jan 18, 2017

brendankenny 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 Jan 20, 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

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 20, 2017

ebidel commented Jan 20, 2017

ebidel commented Jan 21, 2017

Choose a reason for hiding this comment

ebidel Jan 24, 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

ebidel commented Jan 24, 2017 • edited Loading

ebidel commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebidel commented Jan 25, 2017

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny Jan 20, 2017 •

edited

Loading

ebidel Jan 24, 2017 •

edited

Loading

ebidel commented Jan 24, 2017 •

edited

Loading