SingleFile freezes when saving with Zotero Connector enabled

[adomasven]

Hi gildas,

We at Zotero have recently updated the Zotero Connector to use MV3. As part of that change, we had to reimplement our page translation architecture to run in an sandboxed-eval iframe. We have had users report that having Zotero Connector enabled freezes saving with SingleFile, and I have confirmed that on the linked medium page. As the user reports, I indeed see some network activity related to our translation sandbox frame, and having inspected what's happening I have noticed that DOMContentLoaded event keeps firing on our frame, as if being refreshed.

I'm not sure if we're doing something wrong here, or if SingleFile is doing something weird with frames it saves (especially frames that belong to other extensions). Moreover, I'm not sure if SingleFile should be attempting to save frames from non https?:// origins, as it might cause issues with any other extension that uses the sandboxed eval frame too.

[gildas-lormeau]

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];
    }
  }
}

[gildas-lormeau]

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.

[KaimingTao]

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

[adomasven]

@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.

[adomasven]

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

[gildas-lormeau]

@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).

[adomasven]

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?

[gildas-lormeau]

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.

[adomasven]

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