TimelineObjectMemoized utility: Retry on errors

[marcustyphoon]

In a development branch, I ran into a scenario where the timelineObject utility failed (the querySelector in unburyTimelineObject matched no elements; I have not managed to reproduce it or find any way in which that could happen given the code I am running, but in any case). This saved a rejected promise into the timelineObject cache, breaking many extensions until a hard page refresh.

A catch handler invalidating the relevant cache id would cause this problem, if it were to occur to a user (maybe it's caused by a rare race condition where Tumblr's code unmounts stuff before an rAF), to have a less pronounced effect.

This is mostly a reminder to myself to look into it. Ideally I'll figure out what happened and if it could realistically happen in production.

[marcustyphoon]

Oh, you can absolutely trigger this bug. If you click to navigate somewhere, realize it's the wrong link, then click somewhere else while the first navigation's javascript is running, you will get both sets of DOM updates before onBeforeRepaint fires, and the first set of posts won't be on the screen anymore.

I'll probably take a look through the codebase to check for problems this might cause in the next few days. Will probably just need a few optional chaining question marks here and there. Pretty minor, I assume.

[marcustyphoon]

...Hm. I'm trying to think if it would make sense to filter added nodes to those still in the DOM in the mutation handler itself:

  const addedNodes = mutationsPool
    .splice(0)
    .flatMap(({ addedNodes }) => [...addedNodes])
    .filter(addedNode => addedNode instanceof Element && addedNode.isConnected);

This would prevent pageModifications handler code from trying to fire on elements that have been added-and-then-removed, which seems... mostly good, as far as the assumptions baked into these handlers being valid goes. But I don't feel completely confident that there isn't a scenario where this behavior is helpful, like where Tumblr loads some placeholder element and replaces it with something else, and one of our handlers is detecting the first one and then running some general code. Probably not, but...

(If not, I definitely think it makes sense to put this check in filterPostElements.)

Edit: Yeah, there are quite a lot of minor elements in this category from Tumblr's code: image elements, SVGs, progress bars, tooltips, etc. Presumably it would be in our best interest for none of our code to rely on any of these never-seen elements, I would assume, in which case filtering them seems to make sense... but I can't think of a way to be sure that wouldn't introduce bugs besides testing it a bunch.

current test code, not refined, just for the curious

const onBeforeRepaint = () => {
  repaintQueued = false;

  let addedNodes = mutationsPool
    .splice(0)
    .flatMap(({ addedNodes }) => [...addedNodes])
    .filter(addedNode => addedNode instanceof Element /* && addedNode.isConnected */);

  addedNodes.forEach(addedNode => {
    if (addedNode.isConnected) return;
    const selectors = [...pageModifications.listeners.values()];
    const relevant = selectors.some(selector => {
      if (selector === '*') return false; // scroll to bottom is probably fine
      if (selector === 'img[alt]') return false; // visible alt text has null check and does nothing if unmounted
      const result = addedNode.matches(selector) || [...addedNode.querySelectorAll(selector)].length;
      if (result) console.log(selector);
      return result;
    });

    if (relevant) {
      (async () => {
        console.log('unconnected node: ', addedNode);
        throw new Error('unconnected node'); // triggers notification (I run all of my PRs)
      })(addedNode);
    }
  });
  addedNodes = addedNodes.filter(addedNode => addedNode.isConnected);