make sure all "previous navigation event" test results vanish (deleted, async don't arrive late)

[franzenzenhofer]

sometimes on complex pages we have the effect that there is a lot of jumping of the rules panel, and suddenly on the next page previous executed rules start to show up .

sometimes it just seems an old executed rule result is still in the browser database and shows up somehow.

make sure that the table were the results are stored gets cleaned up on regularly, at sensemaking points, but also so that it just happens regularly.

[franzenzenhofer]

there should be a failsave that the table the panel uses to display stuff is "garbadge" collected from time to time (it's hard to explain under which circumstances garbadge starts pilling up in the panel, but it's definitly after a longer time without recompiling/readding the extension)

[franzenzenhofer]

hi moritz

from where and when does the cleanup event get called

https://github.com/franzenzenhofer/f19n-livetest-chrome-extension/blob/6bbf55519388cbd3c91a10121251984ba5070aef/src/javascripts/background.js#L45

[neuling]

ok, i will try go dig into how you implemented async rules. we have to potential points that could cause that effect of changing results.

1) network events that fire after the page has loaded and/or we navigated to another page 2) async rules

we probably need to get rid of not yet executed async rules when we navigate to a different page. i need to find a reproducible case to test and eventually fix this problem.

can you tell ma site where you recently experienced this error?

[franzenzenhofer]

http://gebruederstitch.at/

just browser around, scroll and then go to other pages, after some time you will see a lot of moving around in the panel. also sometimes (which is hard to replicate) some old results from 2 navigations back (sometimes some redirects) just stick.

please also a short note about the cleanup functionality as I just can't find where it is called from.

[neuling]

sorry, forgot about the cleanup method.

this method makes sure that every saved result or hidden-panel information gets cleaned after a tab is closed. trying to avoid leftovers.

its called here: https://github.com/franzenzenhofer/f19n-livetest-chrome-extension/blob/6bbf55519388cbd3c91a10121251984ba5070aef/src/javascripts/background.js#L141

[neuling]

i am slowly getting there. i think the "problem" is the way you implemented all the async stuff or at least the soft404 and robotstxt thing. not judging just trying to track down the error.

in document_start.js you start some fetch calls. once finished they communicate with the background.js via postMessage to extend the eventCollection which causes the rules to execute again.

atm i think thats what fucks up the whole chain reaction of rule execution. its bit of a mess that we need to clean up and implement in a nicer way. its quite complex even without async rules 😄

maybe we should sit down or have a call to define what kind of async actions you need to find a proper solution for this. still not sure what solution would fit your needs best.

what do you think?

[franzenzenhofer]

there are two kinds of async

the one are in the event collection. i needed this because there was no way to do the fetch from the rules.

if we get the fetch from the rules then we could get rid of this kind of async. (or implement the eventCollection that rules for this navigation in this tab does not happen again?)

then there is the other kind of async from the rules, which also can - and should - result in re-ordering (but not re-execution)

[franzenzenhofer]

what i have seen that the "old rule results stuck" happens sometimes (but not other times) after navigating with the back button, but this does not mean that it shows up always with using the back button, mostly if happens on very unoptimized pages and then going to other webproperties and then it turns up again. some kind of garbadge collection might be the answer.

[neuling]

@franzenzenhofer doing a cleanup of all the eventCollection and async stuff trying to implement it in a clean(er) way.

is there a reason why you fire those two events twice (on document_idle as well as on document_end)?

chrome.runtime.sendMessage({ event: 'chrome_load_times', data: { snapshot: window.chrome.loadTimes(), location: document.location } });

chrome.runtime.sendMessage({ event: 'window_performance', data: { snapshot: window.performance, location: document.location } });

will it still do the job if we just fire it on document_end?

[franzenzenhofer]

no, should only be there once.

should fire at the latest possible event.

[neuling]

boom! mega update 🎉 first implementation of async rules + fetch method ignoring CORS. pls check out the branch async-rules to test it.

it's not completely rewritten. i kept some parts of your code but fixed most of the result and eventCollection stuff.

TL;DR: the interface for async rules is pretty much the same like you did except EVERY rule now needs to run 'callback/done' when done. async as well as sync rules.

// standard rule
function(page, done) {
  done(this.createResult('INFO', `Hello from a standard-rule …`, 'info'));
}

// async rule
function(page, done) {
  setTimeout(() => {
    done(this.createResult('INFO', `Hello from a async-rule…`, 'info'));
  }, 1000);
}

until a async-rule is finished, a placeholder entry is rendered inside the results list (like your previous pending message)

a fetch api is now also available for rules.

function(page, done) {
  this.fetch('https://orf.at', { responseFormat: 'text' }, (response) => {
    done(this.createResult('FETCH', `response from orf.at: ${response.length}`, 'info'));
  });
}

because i need to communicate between iframes where i can't pass whole objects around, you need to define what type of response you need responseFormat.

another example:

function(page, done) {
  setTimeout(() => {
    this.fetch('https://jsonplaceholder.typicode.com/posts/1', { responseFormat: 'json' }, (response) => {
      done(this.createResult('FETCH', `json response: ${JSON.stringify(response)}`, 'info'));
    });
  }, 1000);
}

and another one using POST instead of GET

function(page, done) {
  this.fetch('https://orf.at', { responseFormat: 'text' , method: 'POST' }, (response) => {
    done(this.createResult('FETCH', `response from orf.at POST: ${response.length}`, 'info'));
  });
}

it would be great if you could try to implement your speed-test and 404pages with the new async rules and fetch api and let me know if something is missing or does not work as expected.

[neuling]

@franzenzenhofer oh, i forgot to mention that you probably need to migrate all other rules to the new api (always call callback/done) otherwise nothing happens.

if you want me to do this task let me know.

example migration:

function(page) {
  var dom = page.getStaticDom();
  var idom = page.getIdleDom();
  var st = dom.querySelector('title');
  var it = idom.querySelector('title');

  if (st && it && st.innerText && it.innerText) {
    if (st.innerText.trim() !== it.innerText.trim())
    {
      return this.createResult('HEAD', "Static and Idle Titles to not match!"+this.partialCodeLink('Static DOM title:',st,'Idle DOM title:',it), "error");
    }
    return this.createResult('HEAD', text, 'info', 'static');
  }
  return null;
}

becomes

function(page, done) {
  var dom = page.getStaticDom();
  var idom = page.getIdleDom();
  var st = dom.querySelector('title');
  var it = idom.querySelector('title');

  if (st && it && st.innerText && it.innerText) {
    if (st.innerText.trim() !== it.innerText.trim())
    {
      return done(this.createResult('HEAD', "Static and Idle Titles to not match!"+this.partialCodeLink('Static DOM title:',st,'Idle DOM title:',it), "error"));
    }
    return done(this.createResult('HEAD', text, 'info', 'static'));
  }
}

a return is not required anymore.

[franzenzenhofer]

hi moritz, wir haben die meisten default-rules upgedatet, issue ist, 99% funkt nicht mehr, siehe oben

ich habe alle rules disabled, ausser static-head-meta-description.js static-head-title.js

eine regel wird ausgeführt, die andere scheitert background.js:18047 Uncaught TypeError: callbacks[runId] is not a function at background.js:18047 (anonymous) @ background.js:18047

disable is jetzt die title.js

funkt die meta description rule, aber wieder fehlermeldungen

[franzenzenhofer]

ansonsten mag ich die neue syntax mit done

[neuling]

@franzenzenhofer the problem was caused due to a series of issues (mixture of core-code-errors and rule-errors). will explain in detail:

• there was an error handling the async rule results leading to overwrites inside the result-store.

• calling done() twice led to an error where callbacks[runId] was not defined since it fired already. its not possible to call done() a second or third time. every additional done() call will be ignored to prevent this error.

• every rule needs to call done() at some point to resolve the rule, changing the status from pending/WAIT to something else. a rule that never calls done() will remain as pending/WAIT. is this a problem?

• in the process of refactoring the async-rules i forgot to reimplement to handle rule-errors and output the error message as result. should work now again.

[franzenzenhofer]

@neuling

ad calling done() twice - ok, this was because first we used return .... to stop the function altogether so the other stuff will not get executed, ignoring the following done() is a good workaround, to stop the function altogether, I must implement the rules someting like this to stop executing the rule altogehter

done(....); return null;

ad call done() at least once - it's ok, but sometimes it will be just a blank done i.e. https://github.com/franzenzenhofer/f19n-livetest-chrome-extension/issues/58 we only show something if it's an error, otherwise no results, so basically

done();

must get rid of the wait, but no result entry in the panel will be shown

[neuling]

kk i understand. i will add the special case if done() is called without a result it removes the entry from the panel/result list. right?

[neuling]

@franzenzenhofer ok, actually it works as expected out of the box. returning done() without any result will remove the pending/WAIT entry from the panel.

it works because of the following code inside ResultList.jsx:

…
//gets rid of results without required fields
results = results.filter(r => r.type && r.message && r.label);
…

[neuling]

@franzenzenhofer while testing the new async-rules this never happened to me again. did you get this kind of errors with the new async-rules?

[franzenzenhofer]

nope, never saw it again, fixed

On Tue, May 2, 2017 at 11:22 AM, Moritz Kobrna notifications@github.com wrote:

@franzenzenhofer https://github.com/franzenzenhofer while testing the new async-rules this never happened to me again. did you get this kind of errors with the new async-rules?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/franzenzenhofer/f19n-livetest-chrome-extension/issues/58#issuecomment-298573254, or mute the thread https://github.com/notifications/unsubscribe-auth/AATudgWHy9XNIkAYbIYjoffCLs9Ud61qks5r1vW7gaJpZM4Mx4tl .