SingleFile freezes when saving with Zotero Connector enabled about singlefile HOT 9 CLOSED

Hi @adomasven,

I confirm I can reproduce the issue. I've done a debugging session. The issue is related to the _setFrameAttributes function in the code of the connector, see https://github.com/zotero/zotero-connectors/blob/2325e9d2b20c9f80639063d71997868535935451/src/browserExt/zoteroFrame.js#L73C2-L83

The problem is that this function is called very often, even if the parameters don't change. I was able to circumvent the issue by replacing the implementation with the code below (I've added a few if statements) to prevent attribute values from being updated if they don't change.

_setFrameAttributes(attributes, style) {
  for (let key in attributes) {
    if (this._frame[key] !== attributes[key]) {
      this._frame[key] = attributes[key];
    }
  }
  if (this._frame.getAttribute("id") !== attributes.id) {
    this._frame.setAttribute("id", attributes.id);
  }
  for (let key in style) {
    if (this._frame.style) {
      this._frame.style[key] = style[key];
    }
  }
}

from singlefile.

It looks like the real issue is that the observer is never disconnected here https://github.com/zotero/zotero-connectors/blob/2325e9d2b20c9f80639063d71997868535935451/src/browserExt/zoteroFrame.js#L65-L70. I don't see the exit condition. The other problem is that the infinite loop due to the recursive call is too tight.

The DOMContentLoaded bug is due to the src attribute being constantly updated by _setFrameAttributes.

from singlefile.

Confirmed, when uninstalled the Zotero plugin, SingleFile plugin works ok.

from singlefile.

@gildas-lormeau thanks for taking a look. I guess SingleFile is setting some attributes on all iframes that it's capturing which is causing this to run in the first place? The code you've discovered was recently added to prevent websites from accidentally "tampering" with the Zotero translation sandbox frame as that was causing it to take up empty space on some websites, but in my testing it wasn't causing things to run in an infinite loop. Either way, this should be easily enough resolved on our end.

from singlefile.

Zotero Connector will be fixed in the next release, pending Chrome extension store approval.

from singlefile.

@adomasven Thank you. I confirm that SingleFile adds an attribute containing an id on all frames in order to recognize them when processing the page.

Note that there is still a potential infinite loop here https://github.com/zotero/zotero-connectors/blob/b5cf8a0f326fc78ba4abcfa90b4cc9efd0a3ed57/src/browserExt/zoteroFrame.js#L66C4-L68 because observer.observe() is always called after observer.disconnect(). Ideally I think there should be a condition that would prevent observer.observe()to be called indefinitely (e.g. when everything is done). Otherwise you could maybe use a timeout to allow the CPU to do some work between function calls to the listener (passed to the MutationObserver constructor).

from singlefile.

Yes, that is intentional. The need to even have a mutation observer in the first place is because some websites change our iframes post page-load, and we can't know when, or whether they might decide to change them again (e.g. for navigation without page-load). I'm still not sure why SingleFile changing the id was causing an infinite loop. Is it repeatedly trying to change the ID too or something?

from singlefile.

It's true that when I think about it, I find it hard to understand why the listener was called following the modifications made by _setFrameAttributes since it was disconnected. Maybe it's related to a special treatment of the src attribute that would be asynchronous.

from singlefile.

You might still want to consider not saving extension frames in snapshots, but that should probably be a separate issue. Thanks for the help!

from singlefile.