AprilSylph / XKit-Rewritten

🧰 The enhancement suite for Tumblr's new web interface
GNU General Public License v3.0
274 stars 44 forks source link

MV3 branch: Stop processing disconnected elements (simple) #1501

Closed marcustyphoon closed 6 days ago

marcustyphoon commented 1 week ago

Asynchronous inject can result in race conditions where there were previously not any. (Well, maybe this isn't what you would think of of as a race condition? Anyway, by the time the code runs, the target may not be attached to the DOM any more, e.g. if you've scrolled past it.)

Presuming we don't want to pursue instant injection, at least at the moment, this should be fairly easy to handle. I would probably just decline to run unburyTimelineObject and never resolve the inject promise, at first thought? That would stall the function awaiting the inject forever rather than making it have to put a conditional on the result. Not sure how garbage collection works in that case, though; we don't want to leave DOM elements un-GC'd because they have event listeners pending.

_Originally posted by @marcustyphoon in https://github.com/AprilSylph/XKit-Rewritten/pull/1466#discussion_r1616413489_

Description

One way to prevent the console being filled with errors whenever you scroll any timeline with the current MV3 branch, without needing to adjust every location we call timelineObject(), is to respond to injection requests for no-longer-attached-by-the-time-we-process elements with a promise that is neither resolved nor rejected. One would otherwise have to add a conditional early return to each invocation of the function.

This diff is very nice, but it does leave around some mutation observers that will never get triggered. Probably fine?

Testing steps

marcustyphoon commented 1 week ago

Oh, fascinating: this isn't 100% reliable.

In Chrome, I am able to click the patio button, then resize the window horizontally during the soft navigate, and sometimes trigger a TypeError: Cannot read properties of null (reading 'before') console error from line 171 here:

https://github.com/AprilSylph/XKit-Rewritten/blob/6a333b3188b09c0c9ffc87a460c9cacc20ed778c/src/features/trim_reblogs.js#L158-L172

Weirdly, if I log the timeline element at this point, it:

So, a) the inject code didn't remove the script element, so it probably didn't run, so the promise shouldn't have resolved, so why is this line of code running at all? And separately, b) why is that line of code failing if the entire post element is detached but seems fine?

marcustyphoon commented 1 week ago

Ah, I see. When the injected script element loads and executes, the post is still attached, but the button is not. When in doubt, log everything, I guess.

image

  const cachedParents = [];
  let element = editIcon;
  while (element && element !== postElement) {
    cachedParents.push(element);
    element = element.parentElement;
  }

  const { trail = [], content = [] } = await timelineObject(postElement);
  const items = trail.length + (content.length ? 1 : 0);

  if (!editIcon.isConnected) {
    console.log('---');
    for (const element of cachedParents) {
      console.log(
        {
          isConnected: element.isConnected,
          'postElement.contains': postElement.contains(element)
        },
        element
      );
    }
    console.log('---');
  }