Fixes #1282

[ebidel]

R: @brendankenny

You can test against https://geda.nkenspen.de/ to trigger the debugString path. DBW tester won't surface it partly because of #1286.

[ebidel]

one could argue this is ripe for a helper refactor, but we only have 2 atm. Don't think that warrants it just yet.

[wardpeet]

I like the idea of this fix. The try catch covers all our errors but might it be better to add this check inside the error capture as well? So we can mimic it as an eval but say extension?

[ebidel]

@wardpeet do you mean why isn't this inside of driver.js? One reason I kept it in the audits is because we have some captureFunctionCallSites audits that don't filter on page host and the debugStrings for every audit will be different. I don't think there will be much code saved atm.

[Janpot]

Looking through https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/driver.js#L740
For eval'ed code it seems there is getEvalOrigin. Any reason this isn't used instead?

while (callFrame.isEval()) {
  callFrame = callFrame.getEvalOrigin();
}

Seems like that would solve the eval special case.

regarding

lighthouse/lighthouse-core/gather/driver.js

Line 751 in 39cc6a7

url = stackTrace[0];

url will only be empty then in case of extension content scripts, which produce stacks like <anonymous>:1:1. I guess you want those filtered out anyway.

[wardpeet]

@ebidel sorry should have been a bit more clear.

Whenever we write more and more audits that rely on the captureJSCallUsage, it might be ok to also add a check for "extensions" like we do with eval.

we could add an extra parameter but for now just mimic eval.

__nativeError.prepareStackTrace = function(error, structStackTrace) {
      // First frame is the function we injected (the one that just threw).
      // Second, is the actual callsite of the funcRef we're after.
      const callFrame = structStackTrace[1];
      let url = callFrame.getFileName();
      const line = callFrame.getLineNumber();
      const col = callFrame.getColumnNumber();
      let isEval = callFrame.isEval();
      const stackTrace = structStackTrace.slice(1).map(callsite => callsite.toString());

      // If we don't have an URL, (e.g. eval'd code), use the last entry in the
      // stack trace to give some context: eval(<context>):<line>:<col>
      // See https://crbug.com/646849.
      if (!url) {
        url = stackTrace[0];
      }

      try {
         new URL(url);
      } catch(ex) {
         isEval  = true;
      }

      // TODO: add back when we want stack traces.
      // Stack traces were removed from the return object in
      // https://github.com/GoogleChrome/lighthouse/issues/957 so callsites
      // would be unique.
      return {url, args, line, col, isEval }; // return value is e.stack
    };

[wardpeet]

@Janpot indeed getEvalOrigin looks like a good fit.

#1331 is somewhat related, this happens when scripts are evalled.

[patrickhulce]

+1 to seeing if there's a way to identify scripts originating from extensions. Ideally we should ignore those usages entirely and treat them as if they were disabled if we can't actually disable them temporarily? (which seems like a bad idea for extensions to have permission to do anyway)

[ebidel]

PTAL. We're now catching and filtering out crx usage. I've verified this PR works against dbw_tester.html and the original page posted in comment 1 (https://geda.nkenspen.de/), with and without the redux crx enabled.

[Janpot]

@ebidel getEvalOrigin returns a new CallSite instance. I think your code should look like:

const callFrame = structStackTrace[1];
if (callFrame.isEval()) {
  callFrame = callFrame.getEvalOrigin();
}
let url = callFrame.getFileName();
const line = callFrame.getLineNumber();
const col = callFrame.getColumnNumber();
const stackTrace = structStackTrace.slice(1).map(callsite => callsite.toString());

After this you can drop the whole isEval logic completely. The special case will be extension scripts which you can't detect using the stacktrace API. (You can parse the stringified stacktrace though like what's currently happening for eval)

[ebidel]

@Janpot that may be, but the getEvalOrigin() stringifies as expected to the last callframe on the stack. That's the one want went.

I tried your snippet, but it misses some of the eval'd lines in dbw_tester.html

eval('Date.now()'); // FAIL
new Function('Date.now()')() // FAIL

The approach in this PR produces the correct results for all cases. Here's the artifact list sent to the audits:

[patrickhulce]

nit: move to inside catch block? feels a bit like if () { return cond } else { } return true atm

[ebidel]

done

[patrickhulce]

nice I like this improvement

[patrickhulce]

given our fail/no fail smoke tests right now, what benefit does this add? (seems like we'd be unable to catch eval not working here too actually)

[ebidel]

Not much, just wanted to check that Date.now() outside of an inner function was also producing results. Until smokehouse audits number of violations, this won't do much other than a manual verify.

[patrickhulce]

alright just checking, sg

[Janpot]

@ebidel You are right, I tried and it turns out getEvalOrigin just returns a string and not CallSite. The documentation is bogus. Maybe another option is to use some regex to get the url out of 'eval at dateNowTest (http://localhost:10200/dobetterweb/dbw_tester.html:13:3)'?

[Janpot]

Filed as https://bugs.chromium.org/p/chromium/issues/detail?id=677529

[ebidel]

@Janpot going to keep the existing behavior as is. We were returning the first entry of eval'd stack traces before this PR (as opposed to regex'ing out URL patterns). It's been my experience that DevTools doesn't always give us a well defined URL.

[Janpot]

sure, you could always fall back on the first entry if a regex parse doesn't return a valid url